--stripe-packages missing export errors (sorbet#4925)

* --stripe-packages error for missing export

* Add CLI test

* update tests

* format

* clarify error msg

core/packages/PackageDB.cc

@@ -37,6 +37,12 @@ class NonePackage final : public PackageInfo {
 
     std::optional<core::AutocorrectSuggestion> addImport(const core::GlobalState &gs, const PackageInfo &pkg,
                                                          bool isTestImport) const {
+        ENFORCE(false);
+        return nullopt;
+    }
+
+    vector<MissingExportMatch> findMissingExports(core::Context ctx, core::SymbolRef scope, core::NameRef name) const {
+        ENFORCE(false);
         return {};
     }
 

core/packages/PackageInfo.cc

@@ -9,6 +9,10 @@ bool PackageInfo::exists() const {
     return mangledName().exists();
 }
 
+bool PackageInfo::operator==(const PackageInfo &rhs) const {
+    return mangledName() == rhs.mangledName();
+}
+
 PackageInfo::~PackageInfo() {
     // see https://eli.thegreenplace.net/2010/11/13/pure-virtual-destructors-in-c
 }

core/packages/PackageInfo.h

@@ -1,14 +1,17 @@
 #ifndef SORBET_CORE_PACKAGES_PACKAGEINFO_H
 #define SORBET_CORE_PACKAGES_PACKAGEINFO_H
 
+#include "core/NameRef.h"
+#include "core/SymbolRef.h"
 #include <optional>
 #include <vector>
 
 namespace sorbet::core {
-class GlobalState;
+struct AutocorrectSuggestion;
 class NameRef;
 class Loc;
-struct AutocorrectSuggestion;
+class GlobalState;
+class Context;
 } // namespace sorbet::core
 
 namespace sorbet::core::packages {
@@ -27,12 +30,21 @@ class PackageInfo {
     virtual std::optional<core::AutocorrectSuggestion>
     addExport(const core::GlobalState &gs, const std::vector<core::NameRef> name, bool isPrivateTestExport) const = 0;
 
+    bool operator==(const PackageInfo &rhs) const;
+
     virtual ~PackageInfo() = 0;
     PackageInfo() = default;
     PackageInfo(PackageInfo &) = delete;
     explicit PackageInfo(const PackageInfo &) = default;
     PackageInfo &operator=(PackageInfo &&) = delete;
     PackageInfo &operator=(const PackageInfo &) = delete;
+
+    struct MissingExportMatch {
+        core::SymbolRef symbol;
+        core::NameRef srcPkg;
+    };
+    virtual std::vector<MissingExportMatch> findMissingExports(core::Context ctx, core::SymbolRef scope,
+                                                               core::NameRef name) const = 0;
 };
 } // namespace sorbet::core::packages
 #endif

packager/packager.cc

@@ -218,6 +218,47 @@ class PackageInfoImpl final : public sorbet::core::packages::PackageInfo {
         return make_unique<PackageInfoImpl>(*this);
     }
 
+    bool matchesInternalName(core::NameRef nr) const {
+        return nr == name.mangledName || nr == privateMangledName;
+    }
+
+    core::SymbolRef internalModule(const core::GlobalState &gs, bool test) const {
+        return core::Symbols::root()
+            .data(gs)
+            ->findMemberNoDealias(gs, test ? core::Names::Constants::PackageTests()
+                                           : core::Names::Constants::PackageRegistry())
+            .data(gs)
+            ->findMemberNoDealias(gs, privateMangledName);
+    }
+
+    vector<MissingExportMatch> findMissingExports(core::Context ctx, core::SymbolRef scope, core::NameRef name) const {
+        vector<MissingExportMatch> res;
+        for (auto &imported : importedPackageNames) {
+            auto &info = ctx.state.packageDB().getPackageInfo(imported.name.mangledName);
+            if (!info.exists()) {
+                continue;
+            }
+
+            core::SymbolRef sym = PackageInfoImpl::from(info).findPrivateSymbol(ctx, scope, false);
+            if (sym.exists()) {
+                sym = sym.data(ctx)->findMember(ctx, name);
+                if (sym.exists()) {
+                    res.emplace_back(MissingExportMatch{sym, imported.name.mangledName});
+                }
+            }
+            if (core::packages::PackageDB::isTestFile(ctx, ctx.file.data(ctx))) {
+                sym = PackageInfoImpl::from(info).findPrivateSymbol(ctx, scope, true);
+                if (sym.exists()) {
+                    sym = sym.data(ctx)->findMember(ctx, name);
+                    if (sym.exists()) {
+                        res.emplace_back(MissingExportMatch{sym, imported.name.mangledName});
+                    }
+                }
+            }
+        }
+        return res;
+    }
+
     PackageInfoImpl() = default;
     explicit PackageInfoImpl(const PackageInfoImpl &) = default;
     PackageInfoImpl &operator=(const PackageInfoImpl &) = delete;
@@ -315,6 +356,21 @@ class PackageInfoImpl final : public sorbet::core::packages::PackageInfo {
             {{insertionLoc, fmt::format("\n  {} {}", isPrivateTestExport ? "export_for_test" : "export", strName)}});
         return {suggestion};
     }
+
+private:
+    // Recursively walk up a symbol's scope from the package's internal module.
+    core::SymbolRef findPrivateSymbol(const core::GlobalState &gs, core::SymbolRef sym, bool test) const {
+        if (!sym.exists() || sym == core::Symbols::root()) {
+            return core::SymbolRef();
+        } else if (sym.data(gs)->name.isPackagerName(gs)) {
+            return internalModule(gs, test);
+        }
+        auto owner = findPrivateSymbol(gs, sym.data(gs)->owner, test);
+        if (owner.exists()) {
+            return owner.data(gs)->findMember(gs, sym.data(gs)->name);
+        }
+        return owner;
+    }
 };
 
 void checkPackageName(core::Context ctx, ast::UnresolvedConstantLit *constLit) {

resolver/SuggestPackage.cc

@@ -48,6 +48,19 @@ class PackageContext final {
         return matches;
     }
 
+    void addMissingExportSuggestions(core::ErrorBuilder &e, core::packages::PackageInfo::MissingExportMatch match) {
+        vector<core::ErrorLine> lines;
+        auto &srcPkg = db().getPackageInfo(match.srcPkg);
+        lines.emplace_back(core::ErrorLine::from(
+            srcPkg.definitionLoc(), "Do you need to `{} {}` in package `{}`?", core::Names::export_().show(ctx),
+            match.symbol.show(ctx),
+            fmt::map_join(srcPkg.fullName(), "::", [&](auto nr) -> string { return nr.show(ctx); })));
+        lines.emplace_back(core::ErrorLine::from(match.symbol.data(ctx)->loc(),
+                                                 "Constant `{}` is defined here:", match.symbol.show(ctx)));
+        // TODO(nroman-stripe) Add automatic fixers
+        e.addErrorSection(core::ErrorSection(lines));
+    }
+
     void addMissingImportSuggestions(core::ErrorBuilder &e, PackageMatch &match) {
         vector<core::ErrorLine> lines;
         auto &otherPkg = db().getPackageInfo(match.mangledName);
@@ -97,7 +110,15 @@ class PackageContext final {
 } // namespace
 
 bool SuggestPackage::tryPackageCorrections(core::Context ctx, core::ErrorBuilder &e,
-                                           const ast::ConstantLit::ResolutionScopes &scopes, core::NameRef name) {
+                                           const ast::ConstantLit::ResolutionScopes &scopes,
+                                           ast::UnresolvedConstantLit &unresolved) {
+    // Search strategy:
+    // 1. Look for un-exported names in packages *this package already imports* that defined the
+    //    unresolved literal.
+    // 2. Look for packages that we did not import that match the prefix of the unresolved constant
+    //    literal.
+    // Both of above look for EXACT matches only. If neither of these find anything we fall back to
+    // the resolver's default behavior (Symbol::findMemberFuzzyMatch).
     if (ctx.state.packageDB().empty()) {
         return false;
     }
@@ -107,9 +128,21 @@ bool SuggestPackage::tryPackageCorrections(core::Context ctx, core::ErrorBuilder
         return false; // This error is not in packaged code. Nothing to do here.
     }
 
-    // TODO(nroman-stripe) First search for missing exports.
+    if (ast::cast_tree<ast::ConstantLit>(unresolved.scope) != nullptr) {
+        auto missingExports = pkgCtx.currentPkg.findMissingExports(
+            ctx, ast::cast_tree_nonnull<ast::ConstantLit>(unresolved.scope).symbol, unresolved.cnst);
+        if (missingExports.size() > 5) {
+            missingExports.resize(5);
+        }
+        if (!missingExports.empty()) {
+            for (auto match : missingExports) {
+                pkgCtx.addMissingExportSuggestions(e, match);
+            }
+            return true;
+        }
+    }
 
-    vector<PackageMatch> missingImports = pkgCtx.findPossibleMissingImports(scopes, name);
+    vector<PackageMatch> missingImports = pkgCtx.findPossibleMissingImports(scopes, unresolved.cnst);
     if (missingImports.empty()) {
         return false;
     }

resolver/SuggestPackage.h

@@ -7,8 +7,10 @@ namespace sorbet::resolver {
 
 class SuggestPackage final {
 public:
+    // Returns true iff package-specific error messages/corrections were found.
     static bool tryPackageCorrections(core::Context ctx, core::ErrorBuilder &e,
-                                      const ast::ConstantLit::ResolutionScopes &scopes, core::NameRef name);
+                                      const ast::ConstantLit::ResolutionScopes &scopes,
+                                      ast::UnresolvedConstantLit &unresolved);
 
     SuggestPackage() = delete;
 };

resolver/resolver.cc

@@ -353,7 +353,7 @@ class ResolveConstantsWalk {
                                    "  scripts/bin/remote-script sorbet/shim_generation/autogen.rb");
                 } else if (suggestDidYouMean && suggestScope.exists() && suggestScope.isClassOrModule()) {
                     bool madePackageSuggestions =
-                        SuggestPackage::tryPackageCorrections(ctx, e, *job.out->resolutionScopes, original.cnst);
+                        SuggestPackage::tryPackageCorrections(ctx, e, *job.out->resolutionScopes, original);
                     if (madePackageSuggestions) {
                         return;
                     }

test/cli/package-error-missing-export/__package.rb

@@ -0,0 +1,7 @@
+# frozen_string_literal: true
+# typed: strict
+# enable-packager: true
+
+class Foo::MyPackage < PackageSpec
+  import Foo::Bar::OtherPackage
+end

test/cli/package-error-missing-export/foo_class.rb

@@ -0,0 +1,16 @@
+# typed: strict
+
+module Foo
+  module MyPackage::Buuzz
+    class FooClass
+      Foo::Bar::OtherPackage::OtherClass
+      Bar::OtherPackage::OtherClass
+
+      # Following are all not exported:
+      Foo::Bar::OtherPackage::NotExported
+      Bar::OtherPackage::NotExported
+      Foo::Bar::OtherPackage::Inner::AlsoNotExported
+      Bar::OtherPackage::Inner::AlsoNotExported
+    end
+  end
+end

test/cli/package-error-missing-export/other/__package.rb

@@ -0,0 +1,5 @@
+# typed: strict
+
+class Foo::Bar::OtherPackage < PackageSpec
+  export Foo::Bar::OtherPackage::OtherClass
+end

test/cli/package-error-missing-export/other/other_class.rb

@@ -0,0 +1,12 @@
+# typed: strict
+
+module Foo::Bar::OtherPackage
+  class OtherClass
+  end
+
+  class NotExported
+  end
+
+  class Inner::AlsoNotExported
+  end
+end

test/cli/package-error-missing-export/package-error-missing-export.out

@@ -0,0 +1,44 @@
+foo_class.rb:10: Unable to resolve constant `NotExported` https://srb.help/5002
+    10 |      Foo::Bar::OtherPackage::NotExported
+              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+    other/__package.rb:3: Do you need to `export Foo::Bar::OtherPackage::NotExported` in package `Foo::Bar::OtherPackage`?
+     3 |class Foo::Bar::OtherPackage < PackageSpec
+     4 |  export Foo::Bar::OtherPackage::OtherClass
+     5 |end
+    other/other_class.rb:7: Constant `Foo::Bar::OtherPackage::NotExported` is defined here:
+     7 |  class NotExported
+          ^^^^^^^^^^^^^^^^^
+
+foo_class.rb:11: Unable to resolve constant `NotExported` https://srb.help/5002
+    11 |      Bar::OtherPackage::NotExported
+              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+    other/__package.rb:3: Do you need to `export Foo::Bar::OtherPackage::NotExported` in package `Foo::Bar::OtherPackage`?
+     3 |class Foo::Bar::OtherPackage < PackageSpec
+     4 |  export Foo::Bar::OtherPackage::OtherClass
+     5 |end
+    other/other_class.rb:7: Constant `Foo::Bar::OtherPackage::NotExported` is defined here:
+     7 |  class NotExported
+          ^^^^^^^^^^^^^^^^^
+
+foo_class.rb:12: Unable to resolve constant `Inner` https://srb.help/5002
+    12 |      Foo::Bar::OtherPackage::Inner::AlsoNotExported
+              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+    other/__package.rb:3: Do you need to `export Foo::Bar::OtherPackage::Inner` in package `Foo::Bar::OtherPackage`?
+     3 |class Foo::Bar::OtherPackage < PackageSpec
+     4 |  export Foo::Bar::OtherPackage::OtherClass
+     5 |end
+    other/other_class.rb:10: Constant `Foo::Bar::OtherPackage::Inner` is defined here:
+    10 |  class Inner::AlsoNotExported
+                ^^^^^
+
+foo_class.rb:13: Unable to resolve constant `Inner` https://srb.help/5002
+    13 |      Bar::OtherPackage::Inner::AlsoNotExported
+              ^^^^^^^^^^^^^^^^^^^^^^^^
+    other/__package.rb:3: Do you need to `export Foo::Bar::OtherPackage::Inner` in package `Foo::Bar::OtherPackage`?
+     3 |class Foo::Bar::OtherPackage < PackageSpec
+     4 |  export Foo::Bar::OtherPackage::OtherClass
+     5 |end
+    other/other_class.rb:10: Constant `Foo::Bar::OtherPackage::Inner` is defined here:
+    10 |  class Inner::AlsoNotExported
+                ^^^^^
+Errors: 4

test/cli/package-error-missing-export/package-error-missing-export.sh

@@ -0,0 +1,3 @@
+cd test/cli/package-error-missing-export || exit 1
+
+../../../main/sorbet --silence-dev-message --stripe-packages --max-threads=0 . 2>&1

test/cli/package-test-simple/package-test-simple.out

@@ -1,8 +1,11 @@
 main_lib/lib.rb:8: Unable to resolve constant `TestOnly` https://srb.help/5002
      8 |  Project::TestOnly::SomeHelper.new
           ^^^^^^^^^^^^^^^^^
-    test_only/__package.rb:5: Do you need to `import` package `Project::TestOnly`?
+    test_only/__package.rb:5: Do you need to `export Project::TestOnly` in package `Project::TestOnly`?
      5 |class Project::TestOnly < PackageSpec
      6 |  export Project::TestOnly::SomeHelper
      7 |end
+    test_only/some_helper.rb:4: Constant `Project::TestOnly` is defined here:
+     4 |class Project::TestOnly::SomeHelper
+              ^^^^^^^^^^^^^^^^^
 Errors: 1

test/cli/simple-package/simple-package.out

@@ -10,11 +10,14 @@ foo/foo.rb:13: Unable to resolve constant `BardClass` https://srb.help/5002
 foo/foo.rb:14: Unable to resolve constant `UnexportedClass` https://srb.help/5002
     14 |    Project::Bar::UnexportedClass
             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
-  Autocorrect: Use `-a` to autocorrect
-    foo/foo.rb:14: Replace with `Project::Bar::UnexportedClass`
-    14 |    Project::Bar::UnexportedClass
-            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
-    bar/bar.rb:14: Did you mean: `Project::Bar::UnexportedClass`?
+    bar/__package.rb:5: Do you need to `export Project::Bar::UnexportedClass` in package `Project::Bar`?
+     5 |class Project::Bar < PackageSpec
+     6 |  import Project::Foo
+     7 |
+     8 |  export Project::Bar::BarClass
+     9 |  export Project::Bar::BarMethods
+    10 |end
+    bar/bar.rb:14: Constant `Project::Bar::UnexportedClass` is defined here:
     14 |  class UnexportedClass; end
           ^^^^^^^^^^^^^^^^^^^^^
 Errors: 2