8000 Don't suggest an export if it's a modularity error to use that package by neilparikh · Pull Request #8835 · sorbet/sorbet · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Don't suggest an export if it's a modularity error to use that package #8835

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
May 8, 2025
Merged

Don't suggest an export if it's a modularity error to use that package #8835

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
3819409
simplify some code
neilparikh Apr 10, 2025
1bc635a
Don't suggest an export if it's a modularity error to use that package
neilparikh May 5, 2025
4c21346
new folder for main package
neilparikh May 6, 2025
f1a0eb8
add test case
neilparikh May 6, 2025
f502624
move addErrorLine up
neilparikh May 7, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
88 changes: 42 additions & 46 deletions packager/VisibilityChecker.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,9 @@
#include "absl/synchronization/blocking_counter.h"
#include "ast/treemap/treemap.h"
#include "common/concurrency/ConcurrentQueue.h"
#include "common/sort/sort.h"
#include "common/strings/formatting.h"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ooc, were these includes in service of assorted rbi-gen stuff? or do you think they became unused before we deleted the rbi-gen stuff?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably before the rbi-gen stuff. For example, this commit got rid of the call to fast_sort. I assume something similar for the other includes.

#include "core/Context.h"
#include "core/errors/packager.h"
#include "core/errors/resolver.h"
#include <algorithm>
#include <iterator>

using namespace std;

Expand Down Expand Up @@ -407,46 +403,14 @@ class VisibilityCheckerPass final {
} else if (litSymbol.isFieldOrStaticField()) {
isExported = litSymbol.asFieldRef().data(ctx)->flags.isExported;
}

// Did we use a constant that wasn't exported?
if (!isExported && !db.allowRelaxedPackagerChecksFor(this->package.mangledName())) {
if (auto e = ctx.beginError(lit.loc(), core::errors::Packager::UsedPackagePrivateName)) {
auto &pkg = ctx.state.packageDB().getPackageInfo(otherPackage);
e.setHeader("`{}` resolves but is not exported from `{}`", litSymbol.show(ctx), pkg.show(ctx));
auto definedHereLoc = litSymbol.loc(ctx);
if (definedHereLoc.file().data(ctx).isRBI()) {
e.addErrorSection(core::ErrorSection(
core::ErrorColors::format(
"Consider marking this RBI file `{}` if it is meant to declare unpackaged constants",
"# packaged: false"),
{core::ErrorLine(definedHereLoc, "")}));
} else {
e.addErrorLine(definedHereLoc, "Defined here");
}

auto symToExport = litSymbol;
auto enumClass = getEnumClassForEnumValue(ctx.state, symToExport);
if (enumClass.exists()) {
symToExport = enumClass;
}
if (auto exp = pkg.addExport(ctx, symToExport)) {
e.addAutocorrect(std::move(exp.value()));
}
if (!db.errorHint().empty()) {
e.addErrorNote("{}", db.errorHint());
}
}

return;
}

auto importType = this->package.importsPackage(otherPackage);
auto wasNotImported = !importType.has_value();
isExported = isExported || db.allowRelaxedPackagerChecksFor(this->package.mangledName());
auto currentImportType = this->package.importsPackage(otherPackage);
auto wasImported = currentImportType.has_value();
auto importedAsTest =
importType.has_value() && importType.value() == core::packages::ImportType::Test && !this->insideTestFile;
if (wasNotImported || importedAsTest) {
wasImported && currentImportType.value() == core::packages::ImportType::Test && !this->insideTestFile;
if (!wasImported || importedAsTest || !isExported) {
auto &pkg = ctx.state.packageDB().getPackageInfo(otherPackage);
bool isTestImport = otherFile.data(ctx).isPackagedTest() || ctx.file.data(ctx).isPackagedTest();
bool isTestImport = otherFile.data(ctx).isPackagedTest() || this->insideTestFile;
auto strictDepsLevel = this->package.strictDependenciesLevel();
auto importStrictDepsLevel = pkg.strictDependenciesLevel();
bool layeringViolation = false;
Expand All @@ -468,8 +432,34 @@ class VisibilityCheckerPass final {
path.has_value();
}
if (!causesCycle && !layeringViolation && !strictDependenciesTooLow) {
if (wasNotImported) {
// We failed to import the package that defines the symbol
if (!isExported) {
Copy link
Collaborator Author
@neilparikh neilparikh May 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here, !isExported and !wasImported are treated as exclusive, but it's actually possible for both to be true. In a future PR, I'll handle the !isExported && !wasImported case.

if (auto e = ctx.beginError(lit.loc(), core::errors::Packager::UsedPackagePrivateName)) {
auto &pkg = ctx.state.packageDB().getPackageInfo(otherPackage);
e.setHeader("`{}` resolves but is not exported from `{}`", litSymbol.show(ctx), pkg.show(ctx));
auto definedHereLoc = litSymbol.loc(ctx);
if (definedHereLoc.file().data(ctx).isRBI()) {
e.addErrorSection(core::ErrorSection(
core::ErrorColors::format("Consider marking this RBI file `{}` if it is meant to "
"declare unpackaged constants",
"# packaged: false"),
{core::ErrorLine(definedHereLoc, "")}));
} else {
e.addErrorLine(definedHereLoc, "Defined here");
}

auto symToExport = litSymbol;
auto enumClass = getEnumClassForEnumValue(ctx.state, symToExport);
if (enumClass.exists()) {
symToExport = enumClass;
}
if (auto exp = pkg.addExport(ctx, symToExport)) {
e.addAutocorrect(std::move(exp.value()));
}
if (!db.errorHint().empty()) {
e.addErrorNote("{}", db.errorHint());
}
}
} else if (!wasImported) {
if (auto e = ctx.beginError(lit.loc(), core::errors::Packager::MissingImport)) {
e.setHeader("`{}` resolves but its package is not imported", lit.symbol().show(ctx));
e.addErrorLine(pkg.declLoc(), "Exported from package here");
Expand All @@ -487,13 +477,17 @@ class VisibilityCheckerPass final {
}
}
} else if (importedAsTest) {
ENFORCE(!isTestImport);
if (auto e = ctx.beginError(lit.loc(), core::errors::Packager::UsedTestOnlyName)) {
e.setHeader("Used `{}` constant `{}` in non-test file", "test_import", litSymbol.show(ctx));
e.addErrorLine(pkg.declLoc(), "Defined here");
auto &pkg = ctx.state.packageDB().getPackageInfo(otherPackage);
if (auto exp = this->package.addImport(ctx, pkg, false)) {
e.addAutocorrect(std::move(exp.value()));
if (!db.errorHint().empty()) {
e.addErrorNote("{}", db.errorHint());
}
Comment on lines +487 to +489
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We typically put the addErrorNote last, would you mind moving it for consistency?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, moved the addErrorLine call up.

}
e.addErrorLine(pkg.declLoc(), "Defined here");
}
} else {
ENFORCE(false);
Expand Down Expand Up @@ -563,7 +557,9 @@ class VisibilityCheckerPass final {
ENFORCE(false, "At most three reasons should be present");
}
e.setHeader("`{}` cannot be referenced here because {}", lit.symbol().show(ctx), reason);
if (wasNotImported) {
if (!isExported) {
e.addErrorNote("`{}` is not exported", lit.symbol().show(ctx));
} else if (!wasImported) {
e.addErrorNote("`{}`'s package is not imported", lit.symbol().show(ctx));
} else if (importedAsTest) {
e.addErrorNote("`{}`'s package is imported as `{}`", lit.symbol().show(ctx), "test_import");
Expand Down
6 changes: 3 additions & 3 deletions test/cli/package-autocorrect-missing-import/test.out
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,16 +34,16 @@ use_other_package/foo.rb:8: `Foo::Bar::OtherPackage::OtherClass` resolves but it
use_other_package/foo.rb:9: Used `test_import` constant `Foo::Bar::OtherPackageTest::OtherClass` in non-test file https://srb.help/3720
9 | Foo::Bar::OtherPackageTest::OtherClass
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
other_test/__package.rb:3: Defined here
3 |class Foo::Bar::OtherPackageTest < PackageSpec
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Autocorrect: Done
use_other_package/__package.rb:7: Inserted `import Foo::Bar::OtherPackageTest`
7 | strict_dependencies 'layered'
^
use_other_package/__package.rb:8: Deleted
8 | test_import Foo::Bar::OtherPackageTest
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
other_test/__package.rb:3: Defined here
3 |class Foo::Bar::OtherPackageTest < PackageSpec
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

use_other_package/foo.rb:15: `Foo::Bar::OtherPackage::OtherClass` resolves but its package is not imported https://srb.help/3718
15 | Foo::Bar::OtherPackage::OtherClass # resolves via root
Expand Down
11 changes: 11 additions & 0 deletions test/cli/package-error-missing-export/app_false_cycle_package/__package.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
# frozen_string_literal: true
# typed: strict
# enable-packager: true

class Foo::Bar::AppFalseCyclePackage < PackageSpec
layer 'app'
strict_dependencies 'false'

import Foo::Bar::AppFalseCyclePackage::SubPackage
import Foo::MyPackage
end
4 changes: 4 additions & 0 deletions test/cli/package-error-missing-export/app_false_cycle_package/other_class.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
# typed: strict

class Foo::Bar::AppFalseCyclePackage::OtherClass
end
11 changes: 11 additions & 0 deletions test/cli/package-error-missing-export/app_false_cycle_package/sub_package/__package.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
# frozen_string_literal: true
# typed: strict
# enable-packager: true


class Foo::Bar::AppFalseCyclePackage::SubPackage < PackageSpec
layer 'lib'
strict_dependencies 'false'

import Foo::Bar::AppFalseCyclePackage
end
28 changes: 24 additions & 4 deletions test/cli/package-error-missing-export/test.out
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
foo_class.rb:10: `Foo::Bar::OtherPackage::NotExported` resolves but is not exported from `Foo::Bar::OtherPackage` https://srb.help/3717
use_other/foo_class.rb:10: `Foo::Bar::OtherPackage::NotExported` resolves but is not exported from `Foo::Bar::OtherPackage` https://srb.help/3717
10 | Foo::Bar::OtherPackage::NotExported
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
other/other_class.rb:7: Defined here
Expand All @@ -11,7 +11,7 @@ foo_class.rb:10: `Foo::Bar::OtherPackage::NotExported` resolves but is not expor
Note:
Try running generate-packages.sh

foo_class.rb:11: `Foo::Bar::OtherPackage::NotExported` resolves but is not exported from `Foo::Bar::OtherPackage` https://srb.help/3717
use_other/foo_class.rb:11: `Foo::Bar::OtherPackage::NotExported` resolves but is not exported from `Foo::Bar::OtherPackage` https://srb.help/3717
11 | Bar::OtherPackage::NotExported
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
other/other_class.rb:7: Defined here
Expand All @@ -24,7 +24,7 @@ foo_class.rb:11: `Foo::Bar::OtherPackage::NotExported` resolves but is not expor
Note:
Try running generate-packages.sh

foo_class.rb:12: `Foo::Bar::OtherPackage::Inner::AlsoNotExported` resolves but is not exported from `Foo::Bar::OtherPackage` https://srb.help/3717
use_other/foo_class.rb:12: `Foo::Bar::OtherPackage::Inner::AlsoNotExported` resolves but is not exported from `Foo::Bar::OtherPackage` https://srb.help/3717
12 | Foo::Bar::OtherPackage::Inner::AlsoNotExported
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
other/other_class.rb:10: Defined here
Expand All @@ -37,7 +37,7 @@ foo_class.rb:12: `Foo::Bar::OtherPackage::Inner::AlsoNotExported` resolves but i
Note:
Try running generate-packages.sh

foo_class.rb:13: `Foo::Bar::OtherPackage::Inner::AlsoNotExported` resolves but is not exported from `Foo::Bar::OtherPackage` https://srb.help/3717
use_other/foo_class.rb:13: `Foo::Bar::OtherPackage::Inner::AlsoNotExported` resolves but is not exported from `Foo::Bar::OtherPackage` https://srb.help/3717
13 | Bar::OtherPackage::Inner::AlsoNotExported
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
other/other_class.rb:10: Defined here
Expand All @@ -50,3 +50,23 @@ foo_class.rb:13: `Foo::Bar::OtherPackage::Inner::AlsoNotExported` resolves but i
Note:
Try running generate-packages.sh
Errors: 4
use_app_false_cycle_package/foo.rb:6: `Foo::Bar::AppFalseCyclePackage::OtherClass` cannot be referenced here because importing its package would put `Foo::MyPackage` into a cycle, importing its package would cause a layering violation, and its `strict_dependencies` is not strict enough https://srb.help/3727
6 | Foo::Bar::AppFalseCyclePackage::OtherClass
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
use_app_false_cycle_package/__package.rb:7: `Foo::MyPackage` is `strict_dependencies 'layered_dag'`, which disallows cycles
7 | strict_dependencies 'layered_dag'
^^^^^^^^^^^^^
Note:
Path from `Foo::Bar::AppFalseCyclePackage` to `Foo::MyPackage`:
`Foo::Bar::AppFalseCyclePackage` →
`Foo::MyPackage`

app_false_cycle_package/__package.rb:6: Package `Foo::Bar::AppFalseCyclePackage` must be at most layer `lib` (to match package `Foo::MyPackage`) but is currently layer `app`
6 | layer 'app'
^^^^^
app_false_cycle_package/__package.rb:7: `Foo::Bar::AppFalseCyclePackage` must be at least `strict_dependencies 'layered'` but is currently `strict_dependencies 'false'`
7 | strict_dependencies 'false'
^^^^^^^
Note:
`Foo::Bar::AppFalseCyclePackage::OtherClass` is not exported
Errors: 1
7 changes: 6 additions & 1 deletion test/cli/package-error-missing-export/test.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,4 +2,9 @@ cd test/cli/package-error-missing-export || exit 1

../../../main/sorbet --silence-dev-message --stripe-packages \
--stripe-packages-hint-message="Try running generate-packages.sh" \
--max-threads=0 . 2>&1
--max-threads=0 other use_other 2>&1

../../../main/sorbet --silence-dev-message --stripe-packages \
--stripe-packages-hint-message="Try running generate-packages.sh" \
--packager-layers=lib,app \
--max-threads=0 app_false_cycle_package use_app_false_cycle_package 2>&1
8 changes: 8 additions & 0 deletions test/cli/package-error-missing-export/use_app_false_cycle_package/__package.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
# frozen_string_literal: true
# typed: strict
# enable-packager: true

class Foo::MyPackage < PackageSpec
layer 'lib'
strict_dependencies 'layered_dag'
end
9 changes: 9 additions & 0 deletions test/cli/package-error-missing-export/use_app_false_cycle_package/foo.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
# typed: strict

module Foo
module MyPackage
class FooClass
Foo::Bar::AppFalseCyclePackage::OtherClass
end
end
end
0 ...package-error-missing-export/__package.rb → ...ror-missing-export/use_other/__package.rb
View file
Open in desktop
File renamed without changes.
0 ...package-error-missing-export/foo_class.rb → ...ror-missing-export/use_other/foo_class.rb
View file
Open in desktop
File renamed without changes.
8 changes: 5 additions & 3 deletions test/cli/package-test-simple/test.out
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,16 +1,18 @@
main_lib/lib.rb:8: Used `test_import` constant `Project::TestOnly::SomeHelper` in non-test file https://srb.help/3720
8 | Project::TestOnly::SomeHelper.new
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
test_only/__package.rb:5: Defined here
5 |class Project::TestOnly < PackageSpec
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Autocorrect: Use `-a` to autocorrect
main_lib/__package.rb:5: Insert `import Project::TestOnly`
5 |class Project::MainLib < PackageSpec
^
main_lib/__package.rb:7: Delete
7 | test_import Project::TestOnly
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
test_only/__package.rb:5: Defined here
5 |class Project::TestOnly < PackageSpec
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Note:
RUN SCRIPT HINT

main_lib/__package.rb:8: Invalid expression in package: Arguments to functions must be literals https://srb.help/3710
8 | export_for_test Project::MainLib::Lib
Expand Down
2 changes: 1 addition & 1 deletion tools/scripts/format_cxx.sh
View file
Open in desktop 6AF0
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ shift $((OPTIND - 1))

cd "$(dirname "$0")/../.."

./bazel build //tools:clang-format &> /dev/null
./bazel build --config=dbg //tools:clang-format &> /dev/null

if [ "$#" -ne 0 ]; then
cxx_src=("$@")
Expand Down
0