Fix stale completions after changing an import alias

[BurningLutz]

Description

Refresh qualified completions when an import alias is changed without restarting HLS.

Previously, changing:

import A as CM

to:

import A as C

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

GetModSummaryWithoutTimestamps used msrFingerprint as its early-cutoff fingerprint. That fingerprint represents dependency-relevant module information, such as imported
module and package names, but does not include full import declarations.

Changing only an import alias therefore left the fingerprint unchanged, preventing downstream NonLocalCompletions from being recomputed.

Changes

• Add computeImportsFingerprint for full parsed import declarations, including aliases and import lists.
• Combine the imports fingerprint with msrFingerprint in the GetModSummaryWithoutTimestamps early cutoff.
• Add a regression test that changes an alias from CM to C in an open document, and verify that the new alias receives the imported module completions and the previous alias is no longer treated as active.

Testing

Ran the completion and completion-resolve test selection:

cabal test ghcide-tests \
  --test-option=-p \
  --test-option='/completion/' \
  --test-option=--hide-progress

All 49 selected tests passed.

[fendor]

Why are we only using the computeImportsFingerprint here and not in the GetModSummary rule?

[BurningLutz]

Why are we only using the computeImportsFingerprint here and not in the GetModSummary rule?

@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?

[fendor]

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?

[BurningLutz]

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.

[fendor]

Right, as we are fingerprinting imports, it makes some sense to me to additionally fingerprint the import aliases

[BurningLutz]

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.

[fendor]

If the module header changes, there is a good expectation that we need to recompile a module. That's not to say dependencies should be recompiled, there should be rules that short-circuit if the public interface doesn't change.

I think shallowFingers may benefit from using more fine-grained rules that can short circuit earlier if the public interface of a ModSummary doesn't change in any meaningful way. For example, using GetLocatedImports instead of GetModSummaryWithoutTimestamps (ignoring some implementation details).

[BurningLutz]

Just to be sure I understand correctly, are you suggesting:

1. we include import aliases when computing msrFingerprint to build a more complete semantic, but then it will lead to DependencyInformation invalidation when an import alias changes, so:
2. we make shallowFingers take advantage of more fine-grained rules such as GetLocatedImports to short circuit more precisely.

If so, I'm happy to work it out.

[fendor]

Yes, that's what I am suggesting, but I am not 100% sure (2) is going to work as trivially as I am making it out to be. Might need some work to get the msKey or NodeKey_Module. Maybe return it from GetLocatedImports? 🤔

[BurningLutz]

OK, let me try it out.

[BurningLutz]

I've done the first part, the new fingerPrintImports now compute from parsed imports which contains alias information.

The trickier part is the dependencyInfoForFiles. I think we can directly compute shallowFingers from nodeKeys since we already have it, but what about the global fingerprint of dependencyInfoForFiles? It's currently computed from msrFingerprint, after we change msrFingerprint's implementation, dependencyInfoForFiles will be cache invalidated by a single alias change. If we still want to deal with it, then things get complicated.

What do you think? @fendor

[fendor]

LGTM!

[fendor]

Yeah, this looks right to me!

but what about the global fingerprint of dependencyInfoForFiles?

As mentioned in the discussion above, if we changed dependencyInfoForFiles to use GetLocatedImports, we could use the Located ModuleName for fingerprinting.
Then the rule will short-circuit if the imports themselves haven't changed.
Or we move the early cut-off to GetLocatedImports altogether 🤔

However, it is also matter of how often this scenario really occurs... I don't think it would be a catastrophe to be slightly pessimistic and dirty rules when it is not needed.

[BurningLutz]

As mentioned in the discussion above, if we changed dependencyInfoForFiles to use GetLocatedImports, we could use the Located ModuleName for fingerprinting. Then the rule will short-circuit if the imports themselves haven't changed. Or we move the early cut-off to GetLocatedImports altogether 🤔

That's what I've been struggling with for the past few days. I'm so unfamiliar with the DependencyInformation that I cannot easily figure out what should be considered when computing the global fingerprint.

From my naive understanding, the global fingerprint should correctly reflect changes in DependencyInformation: if some "essential information" inside it changes, the fingerprint should also change. But there are lots of information involved in computing a DependencyInformation: the raw deps info, the shallow fingers, the module graph, the boot id map, etc.

I've been trying very hard to figure out what are the true determinants of a DependencyInformation, but so far with no luck.