Fix stale completions after changing an import alias#5011
Conversation
| ms_hspp_buf = error "use GetModSummary instead of GetModSummaryWithoutTimestamps" | ||
| } | ||
| fp = fingerprintToBS msrFingerprint | ||
| fp = fingerprintToBS $ Util.fingerprintFingerprints [msrFingerprint, computeImportsFingerprint msrImports] |
There was a problem hiding this comment.
Why are we only using the computeImportsFingerprint here and not in the GetModSummary rule?
There was a problem hiding this comment.
Why are we only using the
computeImportsFingerprinthere and not in theGetModSummaryrule?
@fendor I've considered that, but the imports fingerprint is a derived value from msrImports and only used here, in order not to break anything I chose to compute here. :)
Is it expected to compute in the GetModSummary rule?
There was a problem hiding this comment.
It feels non-trivial to see why this works, initially I thought GetModSummary might produce the same fingerprint and thus short-circuit the calculation, rendering this fingerprint unsued. However, now I can tell, this is not the case, GetModSummary will be recomputed in this situation.
I just don't see why the fingerprinting logic should be different here, I think computeImportsFingerprint msrImports should be part of msrFingerprint from the start.
If an import changes, more than only completions need to be recomputed, like we need to typecheck the whole module again!
Actually, looking at the implementation of msrFingerprint, it looks like the imports are already part of the fingerprint. Perhaps there is a bug, or missing value in there?
There was a problem hiding this comment.
Perhaps there is a bug, or missing value in there?
AFAIK, import aliases are missing in the fingerPrintImports of msrFingerprint so changing aliases won't result in a different fingerprint value.
There was a problem hiding this comment.
Right, as we are fingerprinting imports, it makes some sense to me to additionally fingerprint the import aliases
There was a problem hiding this comment.
it makes some sense to me to additionally fingerprint the import aliases
I agree, with a little concern about the usage of msrFingerprint.
At line 684, msrFingerprint is used to compute shallowFingers which is used to compute a DependencyInformation.
So I do think It is a good reason to exclude import aliases from msrFingerprint: changing aliases won't result in a different depenceny info.
This is why I wasn't touching msrFingerprint in the first place.
Description
Refresh qualified completions when an import alias is changed without restarting HLS.
Previously, changing:
to:
could leave non-local completions associated with the old CM qualifier. As a result, completions for C did not prioritize the members imported from A.
Root cause
GetModSummaryWithoutTimestampsusedmsrFingerprintas its early-cutoff fingerprint. That fingerprint represents dependency-relevant module information, such as importedmodule and package names, but does not include full import declarations.
Changing only an import alias therefore left the fingerprint unchanged, preventing downstream
NonLocalCompletionsfrom being recomputed.Changes
computeImportsFingerprintfor full parsed import declarations, including aliases and import lists.msrFingerprintin theGetModSummaryWithoutTimestampsearly cutoff.Testing
Ran the completion and completion-resolve test selection:
All 49 selected tests passed.