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

Record diagnostics source rule when testing #3301

Merged
merged 3 commits into from
Nov 1, 2022
Merged

Conversation

pepeiborra
Copy link
Collaborator

@pepeiborra pepeiborra commented Oct 23, 2022

I have seen unexpected duplicate diagnostics leading to a failure in the ghcide testsuite: what seems to happen here is that both Typecheck and GetModIfaceFromDisk contribute the same diagnostics. But this should never happen, because we never run both these rules for the same file:

  • GetModIfaceFromDisk is only run for non FOIs via GetModIface
  • Typecheck is only run for FOIs, either via GetModIface or on demand by code actions

This PR adds more testing-only logging to help diagnostics when this happens again next time

Copy link
Collaborator

@fendor fendor left a comment

Choose a reason for hiding this comment

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

Nice addition

Copy link
Collaborator

@michaelpj michaelpj left a comment

Choose a reason for hiding this comment

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

Slightly an abuse of the source field but I guess it's fine. We could put it in the data field? which is entirely implementation-defined. Then we could consider even doing it unconditionally, or having some richer-structured "provenance" structure.

@pepeiborra
Copy link
Collaborator Author

Slightly an abuse of the source field but I guess it's fine. We could put it in the data field? which is entirely implementation-defined. Then we could consider even doing it unconditionally, or having some richer-structured "provenance" structure.

Do you mean the code field? I like the idea, but it doesn't look like diagnostics have a data field

@michaelpj
Copy link
Collaborator

No, code is for the error code, which we might actually get from GHC before long. Diagnostics do have a data field, but not in the version of the lsp library we're using, I guess.

@fendor fendor added the merge me Label to trigger pull request merge label Nov 1, 2022
@mergify mergify bot merged commit 2b94f85 into master Nov 1, 2022
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.

3 participants