8000 Don't insert a duplicate import when applying package autocorrects by neilparikh · Pull Request #8765 · sorbet/sorbet · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Don't insert a duplicate import when applying package autocorrects #8765

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

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Don't insert a duplicate import when applying package autocorrects #8765

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
04f6bf4
simplify some code
neilparikh Apr 10, 2025
c333dbc
add a CLI test
neilparikh Apr 21, 2025
6511595
add a LSP test
neilparikh Apr 14, 2025
c80abe4
track missing imports
neilparikh Apr 24, 2025
f21d80e
aggregate imports
neilparikh Apr 24, 2025
55b329f
use the tracked missing imports to compute the final edit
neilparikh Apr 11, 2025
767cd2b
use the tracked missing imports to compute apply all autocorrect in lsp
neilparikh Apr 24, 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
11 changes: 8 additions & 3 deletions core/AutocorrectSuggestion.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,13 +13,18 @@ struct AutocorrectSuggestion {
std::string replacement;
};

const std::string title;
std::string title;
std::vector<Edit> edits;

bool isDidYouMean;

AutocorrectSuggestion(std::string title, std::vector<Edit> edits, bool isDidYouMean = false)
: title(title), edits(edits), isDidYouMean(isDidYouMean) {}
bool skipWhenAggregated;

AutocorrectSuggestion() : title(""), edits({}), isDidYouMean(false), skipWhenAggregated(false) {}
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor nit: I would be inclined to declare the (bool-only) fields as bool isDidYouMean = false; in the class definition and then this constructor could just be AutocorrectSuggestion() = default;.


AutocorrectSuggestion(std::string title, std::vector<Edit> edits, bool isDidYouMean = false,
bool skipWhenAggregated = false)
: title(title), edits(edits), isDidYouMean(isDidYouMean), skipWhenAggregated(skipWhenAggregated) {}

// Reads all the files to be edited, and then accumulates all the edits that need to be applied
// to those files into a resulting string with all edits applied. Does not write those back out
Expand Down
14 changes: 13 additions & 1 deletion core/ErrorFlusherStdout.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
#include "core/ErrorFlusherStdout.h"
#include "common/FileSystem.h"
#include "core/GlobalState.h"
#include "core/lsp/QueryResponse.h"

using namespace std;
Expand All @@ -25,6 +26,9 @@ void ErrorFlusherStdout::flushErrors(spdlog::logger &logger, const GlobalState &
fmt::format_to(std::back_inserter(out), "{}", error->text.value_or(""));

for (auto &autocorrect : error->error->autocorrects) {
if (autocorrect.skipWhenAggregated) {
continue;
}
autocorrects.emplace_back(move(autocorrect));
}
}
Expand Down Expand Up @@ -57,7 +61,15 @@ void ErrorFlusherStdout::flushErrorCount(spdlog::logger &logger, int count) {
}

void ErrorFlusherStdout::flushAutocorrects(const GlobalState &gs, FileSystem &fs) {
auto toWrite = AutocorrectSuggestion::apply(gs, fs, this->autocorrects);
if (gs.packageDB().enabled()) {
for (auto &pkgName : gs.packageDB().packages()) {
auto &pkg = gs.packageDB().getPackageInfo(pkgName);
ENFORCE(pkg.exists());
auto autocorrect = pkg.aggregateMissingImports();
autocorrects.emplace_back(move(autocorrect));
}
}
auto toWrite = AutocorrectSuggestion::apply(gs, fs, autocorrects);
for (auto &[file, contents] : toWrite) {
fs.writeFile(string(file.data(gs).path()), contents);
}
Expand Down
20 changes: 20 additions & 0 deletions core/packages/PackageDB.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,26 @@ class NonePackage final : public PackageInfo {
return nullopt;
}

void trackMissingImport(const core::FileRef file, const core::packages::MangledName toImport,
const core::packages::ImportType importType,
const core::AutocorrectSuggestion autocorrect) {
notImplemented();
}

void untrackMissingImportsFor(const core::FileRef file) {
notImplemented();
}

core::AutocorrectSuggestion aggregateMissingImports() const {
notImplemented();
return core::AutocorrectSuggestion{"notImplemented", {}};
}

core::AutocorrectSuggestion aggregateMissingImports(const core::FileRef file) const {
notImplemented();
return core::AutocorrectSuggestion{"notImplemented", {}};
}

~NonePackage() {}

private:
Expand Down
20 changes: 20 additions & 0 deletions core/packages/PackageInfo.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,26 @@ class PackageInfo {
virtual std::optional<std::string> pathTo(const core::GlobalState &gs,
const core::packages::MangledName dest) const = 0;

// Track that this package is missing an import of type `importType` for `toImport` in `file`, along with an
// autocorrect that adds the import to this package's __package.rb. We use this to de-duplicate imports added by
// autocorrects; multiple calls to trackMissingImport for the same un-imported package will still lead to only the
// package being imported once in the autocorrect generated by aggregateMissingImports
// Called in VisibilityChecker when a unimported constant is encountered.
virtual void trackMissingImport(const core::FileRef file, const core::packages::MangledName toImport,
const core::packages::ImportType importType,
const core::AutocorrectSuggestion autocorrect) = 0;

// Remove knowledge that this package is missing an import of type `importType` for `toImport` in `file`.
// We do this so that when VisibilityChecker is re-run over `file`, we can delete stale information.
virtual void untrackMissingImportsFor(const core::FileRef file) = 0;

// Create on autocorrect that contains edits to insert all the missing imports for this package.
virtual core::AutocorrectSuggestion aggregateMissingImports() const = 0;

// Create on autocorrect that contains edits to insert all the missing imports for this package that are referenced
// in `file`.
virtual core::AutocorrectSuggestion aggregateMissingImports(const core::FileRef file) const = 0;

// autocorrects
virtual std::optional<core::AutocorrectSuggestion> addImport(const core::GlobalState &gs, const PackageInfo &pkg,
bool isTestImport) const = 0;
Expand Down
11 changes: 11 additions & 0 deletions main/lsp/requests/code_action.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,6 +139,9 @@ unique_ptr<ResponseMessage> CodeActionTask::runRequest(LSPTypecheckerDelegate &t
// Collect all autocorrects regardless of range to compile into a "source" autocorrect whose scope is
// the whole file.
for (auto &autocorrect : error->autocorrects) {
if (autocorrect.skipWhenAggregated) {
continue;
}
allEdits.insert(allEdits.end(), autocorrect.edits.begin(), autocorrect.edits.end());
}

Expand All @@ -165,6 +168,14 @@ unique_ptr<ResponseMessage> CodeActionTask::runRequest(LSPTypecheckerDelegate &t
}
}

if (gs.packageDB().enabled()) {
auto &package = gs.packageDB().getPackageForFile(gs, file);
if (package.exists()) {
auto autocorrect = package.aggregateMissingImports(file);
allEdits.insert(allEdits.end(), autocorrect.edits.begin(), autocorrect.edits.end());
}
}

// TODO(sushain): investigate where duplicates might happen and whether there is a better fix
// Remove any actions with the same header regardless of their actions since users cannot make an informed decision
// between two seemingly identical actions.
Expand Down
91 changes: 70 additions & 21 deletions packager/VisibilityChecker.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -364,6 +364,8 @@ class VisibilityCheckerPass final {
public:
const core::packages::PackageInfo &package;
const bool insideTestFile;
UnorderedMap<std::pair<core::packages::MangledName, core::packages::ImportType>, core::AutocorrectSuggestion>
missingImports;

VisibilityCheckerPass(core::Context ctx, const core::packages::PackageInfo &package)
: package{package}, insideTestFile{ctx.file.data(ctx).isPackagedTest()} {}
Expand Down Expand Up @@ -438,13 +440,13 @@ class VisibilityCheckerPass final {
return;
}

auto importType = this->package.importsPackage(otherPackage);
auto wasNotImported = !importType.has_value();
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) {
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 @@ -466,13 +468,22 @@ class VisibilityCheckerPass final {
path.has_value();
}
if (!causesCycle && !layeringViolation && !strictDependenciesTooLow) {
if (wasNotImported) {
// We failed to import the package that defines the symbol
auto newImportType =
isTestImport ? core::packages::ImportType::Test : core::packages::ImportType::Normal;
std::optional<core::AutocorrectSuggestion> autocorrect =
this->package.addImport(ctx, pkg, isTestImport);
// TODO(neil): find out why this is sometimes not true and enable this ENFORCE
/* ENFORCE(autocorrect.has_value()); */
Comment on lines +475 to +476
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we doing this TODO before committing?

if (autocorrect.has_value()) {
missingImports.insert({{pkg.mangledName(), newImportType}, autocorrect.value()});
}
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");
if (auto exp = this->package.addImport(ctx, pkg, isTestImport)) {
e.addAutocorrect(std::move(exp.value()));
if (autocorrect.has_value()) {
core::AutocorrectSuggestion autocorrectCopy(autocorrect.value());
e.addAutocorrect(std::move(autocorrectCopy));
if (!db.errorHint().empty()) {
e.addErrorNote("{}", db.errorHint());
}
Expand All @@ -485,11 +496,16 @@ 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));
auto &pkg = ctx.state.packageDB().getPackageInfo(otherPackage);
if (auto exp = this->package.addImport(ctx, pkg, false)) {
e.addAutocorrect(std::move(exp.value()));
if (autocorrect.has_value()) {
core::AutocorrectSuggestion autocorrectCopy(autocorrect.value());
e.addAutocorrect(std::move(autocorrectCopy));
if (!db.errorHint().empty()) {
e.addErrorNote("{}", db.errorHint());
}
}
e.addErrorLine(pkg.declLoc(), "Defined here");
}
Expand Down Expand Up @@ -561,7 +577,7 @@ 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 (!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 All @@ -581,35 +597,68 @@ class VisibilityCheckerPass final {
}
}

static std::vector<ast::ParsedFile> run(const core::GlobalState &gs, WorkerPool &workers,
static std::vector<ast::ParsedFile> run(core::GlobalState &nonConstGs, WorkerPool &workers,
std::vector<ast::ParsedFile> files) {
const core::GlobalState &gs = nonConstGs;
Timer timeit(gs.tracer(), "visibility_checker.check_visibility");
auto taskq = std::make_shared<ConcurrentBoundedQueue<size_t>>(files.size());
auto resultq = std::make_shared<BlockingBoundedQueue<
std::pair<core::FileRef, UnorderedMap<std::pair<core::packages::MangledName, core::packages::ImportType>,
core::AutocorrectSuggestion>>>>(files.size());
absl::BlockingCounter barrier(std::max(workers.size(), 1));

for (size_t i = 0; i < files.size(); ++i) {
taskq->push(i, 1);
}

workers.multiplexJob("VisibilityChecker", [&gs, &files, &barrier, taskq]() {
workers.multiplexJob("VisibilityChecker", [&gs, &files, &barrier, taskq, resultq]() {
size_t idx;
for (auto result = taskq->try_pop(idx); !result.done(); result = taskq->try_pop(idx)) {
ast::ParsedFile &f = files[idx];
if (!f.file.data(gs).isPackage(gs)) {
auto pkgName = gs.packageDB().getPackageNameForFile(f.file);
if (pkgName.exists()) {
core::Context ctx{gs, core::Symbols::root(), f.file};
VisibilityCheckerPass pass{ctx, gs.packageDB().getPackageInfo(pkgName)};
ast::TreeWalk::apply(ctx, pass, f.tree);
}
if (f.file.data(gs).isPackage(gs)) {
resultq->push({f.file, {}}, 1);
continue;
}

auto pkgName = gs.packageDB().getPackageNameForFile(f.file);
if (!pkgName.exists()) {
resultq->push({f.file, {}}, 1);
continue;
}

core::Context ctx{gs, core::Symbols::root(), f.file};
VisibilityCheckerPass pass{ctx, gs.packageDB().getPackageInfo(pkgName)};
ast::TreeWalk::apply(ctx, pass, f.tree);
resultq->push({f.file, std::move(pass.missingImports)}, 1);
}

barrier.DecrementCount();
});

barrier.Wait();

std::pair<core::FileRef, UnorderedMap<std::pair<core::packages::MangledName, core::packages::ImportType>,
Comment on lines +632 to +640
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this will have to be rebased across #8852.

Even after that's done, we should make the previous loop here use multiplexJob so that the aggregation loop below can proceed in parallel with the processing of the visibility checking and not wait for all the visibility checks to be done.

core::AutocorrectSuggestion>>
threadResult;
for (auto result = resultq->wait_pop_timed(threadResult, WorkerPool::BLOCK_INTERVAL(), gs.tracer());
!result.done();
result = resultq->wait_pop_timed(threadResult, WorkerPool::BLOCK_INTERVAL(), gs.tracer())) {
if (result.gotItem()) {
auto file = threadResult.first;
auto pkgName = gs.packageDB().getPackageNameForFile(file);
if (!pkgName.exists()) {
continue;
}
auto nonConstPackageInfo = nonConstGs.packageDB().getPackageInfoNonConst(pkgName);
nonConstPackageInfo->untrackMissingImportsFor(file);
for (auto [key, autocorrect] : threadResult.second) {
auto toImport = key.first;
auto importType = key.second;
nonConstPackageInfo->trackMissingImport(file, toImport, importType, autocorrect);
}
}
}

return files;
}
};
Expand Down
Loading
0