diff --git a/core/packages/PackageDB.cc b/core/packages/PackageDB.cc index c3faa3c101c..c7a15f114a4 100644 --- a/core/packages/PackageDB.cc +++ b/core/packages/PackageDB.cc @@ -37,6 +37,12 @@ class NonePackage final : public PackageInfo { std::optional addImport(const core::GlobalState &gs, const PackageInfo &pkg, bool isTestImport) const { + ENFORCE(false); + return nullopt; + } + + vector findMissingExports(core::Context ctx, core::SymbolRef scope, core::NameRef name) const { + ENFORCE(false); return {}; } diff --git a/core/packages/PackageInfo.cc b/core/packages/PackageInfo.cc index 3c2a4d552b9..6c4df937f0e 100644 --- a/core/packages/PackageInfo.cc +++ b/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 } diff --git a/core/packages/PackageInfo.h b/core/packages/PackageInfo.h index edea3ea4af2..0ece6553fe5 100644 --- a/core/packages/PackageInfo.h +++ b/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 #include 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 addExport(const core::GlobalState &gs, const std::vector 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 findMissingExports(core::Context ctx, core::SymbolRef scope, + core::NameRef name) const = 0; }; } // namespace sorbet::core::packages #endif diff --git a/packager/packager.cc b/packager/packager.cc index 2f4e0ccc28a..6c0cff951b4 100644 --- a/packager/packager.cc +++ b/packager/packager.cc @@ -218,6 +218,47 @@ class PackageInfoImpl final : public sorbet::core::packages::PackageInfo { return make_unique(*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 findMissingExports(core::Context ctx, core::SymbolRef scope, core::NameRef name) const { + vector 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) { diff --git a/resolver/SuggestPackage.cc b/resolver/SuggestPackage.cc index bf0cf682ce7..8f5c5aea470 100644 --- a/resolver/SuggestPackage.cc +++ b/resolver/SuggestPackage.cc @@ -48,6 +48,19 @@ class PackageContext final { return matches; } + void addMissingExportSuggestions(core::ErrorBuilder &e, core::packages::PackageInfo::MissingExportMatch match) { + vector 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 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(unresolved.scope) != nullptr) { + auto missingExports = pkgCtx.currentPkg.findMissingExports( + ctx, ast::cast_tree_nonnull(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 missingImports = pkgCtx.findPossibleMissingImports(scopes, name); + vector missingImports = pkgCtx.findPossibleMissingImports(scopes, unresolved.cnst); if (missingImports.empty()) { return false; } diff --git a/resolver/SuggestPackage.h b/resolver/SuggestPackage.h index 0f85d429bda..71fd2c06c77 100644 --- a/resolver/SuggestPackage.h +++ b/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; }; diff --git a/resolver/resolver.cc b/resolver/resolver.cc index a737fe1bc55..51a163a626c 100644 --- a/resolver/resolver.cc +++ b/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; } diff --git a/test/cli/package-error-missing-export/__package.rb b/test/cli/package-error-missing-export/__package.rb new file mode 100644 index 00000000000..b80a3da715b --- /dev/null +++ b/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 diff --git a/test/cli/package-error-missing-export/foo_class.rb b/test/cli/package-error-missing-export/foo_class.rb new file mode 100644 index 00000000000..d06300b1041 --- /dev/null +++ b/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 diff --git a/test/cli/package-error-missing-export/other/__package.rb b/test/cli/package-error-missing-export/other/__package.rb new file mode 100644 index 00000000000..65c2a80c116 --- /dev/null +++ b/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 diff --git a/test/cli/package-error-missing-export/other/other_class.rb b/test/cli/package-error-missing-export/other/other_class.rb new file mode 100644 index 00000000000..56dbf4de972 --- /dev/null +++ b/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 diff --git a/test/cli/package-error-missing-export/package-error-missing-export.out b/test/cli/package-error-missing-export/package-error-missing-export.out new file mode 100644 index 00000000000..8f6e3921164 --- /dev/null +++ b/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 diff --git a/test/cli/package-error-missing-export/package-error-missing-export.sh b/test/cli/package-error-missing-export/package-error-missing-export.sh new file mode 100755 index 00000000000..72d81574127 --- /dev/null +++ b/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 diff --git a/test/cli/package-test-simple/package-test-simple.out b/test/cli/package-test-simple/package-test-simple.out index 35586a32918..b2b74485139 100644 --- a/test/cli/package-test-simple/package-test-simple.out +++ b/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 diff --git a/test/cli/simple-package/simple-package.out b/test/cli/simple-package/simple-package.out index cf73349e4dc..8d0b3de5d09 100644 --- a/test/cli/simple-package/simple-package.out +++ b/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