Don't insert a duplicate import when applying package autocorrects #8765
Conversation
-a mode
There was a problem hiding this comment.
I think this is the right general idea, but it is the wrong layer for the implementation to live at -- ErrorFlusherStdout should not have any knowledge of the packager errors, for instance. (Note also that we'd have to write separate support for the LSP implementation, which is not desirable.)
I think it's possible to rewrite this such that the worker threads in the VisibilityChecker maintain their own lists of any import errors that have happened, and they can send those accumulated errors off to a queue which gets drained and deduplicates errors before sending them off to the actual error queue in the GlobalState. (I believe that we do this kind of pattern in at least one other place?) I do not think we want to be storing this kind of information in the package DB -- we can maintain this same mapping in whatever structure we have in the VisibilityChecker instead. Happy to explain/explore more over Slack.
packager/VisibilityChecker.cc
Outdated
| 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)}; | ||
| VisibilityCheckerPass pass{ctx, *nonConstGs.packageDB().getPackageInfoNonConst(pkgName)}; |
There was a problem hiding this comment.
It is possible for this kind of pattern -- having non-constant shared data used in worker threads -- to be safe, but it is not safe in this instance. We could have multiple files that share the same package both have import errors, and then separate threads would be modifying the same package without any locking between them.
6de52db to
93edb89
Compare
e0f5469 to
b011b82
Compare
-a modeb011b82 to
767cd2b
Compare
There was a problem hiding this comment.
Sorry about the review delay here, I think this is better than the previous version. Still a few things to fix.
IIUC, this is going to collide with @jez's PackageRef work, so you may want to coordinate -- if you haven't already -- on what the right thing to do there is.
| 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>, |
There was a problem hiding this comment.
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.
| for (auto &[key, value] : trackedMissingImports_) { | ||
| auto importName = key.first; | ||
| auto importType = key.second; | ||
| auto autocorrect = value.second; |
There was a problem hiding this comment.
| auto autocorrect = value.second; | |
| auto &autocorrect = value.second; |
| // test_import. | ||
| continue; | ||
| } | ||
| allEdits.insert(allEdits.end(), autocorrect.edits.begin(), autocorrect.edits.end()); |
There was a problem hiding this comment.
These autocorrect.edits should probably be move iterators?
| std::vector<core::AutocorrectSuggestion::Edit> allEdits; | ||
| for (auto &[_, value] : trackedMissingImports_) { | ||
| auto files = value.first; | ||
| auto autocorrect = value.second; |
There was a problem hiding this comment.
| auto autocorrect = value.second; | |
| auto &autocorrect = value.second; |
| auto files = value.first; | ||
| auto autocorrect = value.second; | ||
| if (files.contains(file)) { | ||
| allEdits.insert(allEdits.end(), autocorrect.edits.begin(), autocorrect.edits.end()); |
There was a problem hiding this comment.
Same comment on move iterators here.
| // TODO(neil): find out why this is sometimes not true and enable this ENFORCE | ||
| /* ENFORCE(autocorrect.has_value()); */ |
There was a problem hiding this comment.
Are we doing this TODO before committing?
| : title(title), edits(edits), isDidYouMean(isDidYouMean) {} | ||
| bool skipWhenAggregated; | ||
|
|
||
| AutocorrectSuggestion() : title(""), edits({}), isDidYouMean(false), skipWhenAggregated(false) {} |
There was a problem hiding this comment.
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;.
| if (files.contains(file)) { | ||
| files.erase(file); | ||
| } |
There was a problem hiding this comment.
Nit: better to do something like:
auto it = files.find(file);
if (it == files.end()) {
continue;
}
files.erase(it);so you don't do unnecessary hash lookups.
When applying autocorrects in
core/ErrorFlusherStdout.cc, we will now ignore the autocorrects that are set on the missing import errors. Instead, we store all the missing imports for a given package inPackageInfo, and then aggregate those to compute the autocorrect. This means that 2 missing import errors for the same missing import will only lead to the import being inserted onceSomething similar is done in
code_action.cc. There, we have a list of the all the edits for autocorrects for the current file. While constructing that list, we will skip missing import errors. Then, we aggregate the autocorrects from thePackageInfo, but only those that apply for the current file.Easier to review commit-by-commit.
Motivation
Don't import the same package more than once.
Test plan
See included automated tests.