Skip to content
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.

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

Fix some import module completions being dropped (and fix flaky test too) #2595

Merged
merged 2 commits into from
Jan 17, 2022

Conversation

eddiemundo
Copy link
Collaborator

@eddiemundo eddiemundo commented Jan 16, 2022

Fixes #2577 and maybe other issues involving import module completions.

The superficial reason why the module completions test was failing is that HashMap's toList ordering is not deterministic between runs. This meant that the list ordering of the target files/modules would occur in different orders.

Of course why should the order of module names affect anything with regards to completions?

The test generates 3 targets, 2 TargetModules and 1 TargetFile. Since we seem to only care about actual modules we convert TargetFiles to empty strings. This list of module names (including the empty string) eventually get passed to the fuzzy matcher which attempts to run the matcher on the names in parallel.

Interestingly different orders can cause the parallel strategy to give different results which should be impossible unless the Strategy changes its input. Therefore the strategy does change its input.

Turns out the function used to chunk vectors had an off-by-one error when doing the Vector chunking, and if the empty string wasn't in the last position some module completions would be dropped.
e.g.

> chunk 3 [1,2,3]
[[1,2], []]

Also interesting is the comment on the chunkVector function seems to have come from eval plugin usage and has the correct output underneath it, yet when I eventually ran the eval plugin for kicks it produced the wrong output 😅

The fix is just adding +1 to the slice length.

Copy link
Member

@jneira jneira left a comment

Choose a reason for hiding this comment

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

Wow, good catch, many thanks for the fix

@jneira jneira added the merge me Label to trigger pull request merge label Jan 17, 2022
@mergify mergify bot merged commit 3024c80 into haskell:master Jan 17, 2022
@pepeiborra
Copy link
Collaborator

pepeiborra commented Jan 17, 2022

Thanks for the fix @eddiemundo awesome stuff. The lack of unit tests in this code (I authored it) is disturbing...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge me Label to trigger pull request merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Flaky test: auto complete project imports (ghcide) (9.2)
3 participants