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

Use exact print for suggest missing constraint code actions #1221

Merged
merged 11 commits into from
Jan 17, 2021

Conversation

pepeiborra
Copy link
Collaborator

This is a big PR with several changes, split in well defined commits for ease of review.

Main changes

1. Cache the annotated AST

If we are going to use ghc-exactprint in code actions, It makes sense to cache the annotated AST in the Shake graph

2. Introduce a Rewrite abstraction

Code actions that intend to preserve user formatting cannot use the existing Development.IDE.GHC.ExactPrint.Graft abstraction, which throws away all format. So instead we introduce a weaker Rewrite abstraction that does fewer things:

  • It doesn't annotate things for you (so it doesn't destroy user format)
  • It doesn't provide a Monoid instance (for now)
  • It doesn't use SYB to perform the replacement
  • It doesn't diff to compute the result

The use case is code actions where you don't have the SrcSpan that you need to edit at hand, and instead you need to traverse the AST manually to locate the declaration to edit. The ergonomics are not as good as Graft though, there's certainly room to improve.

3. Refactor the existing suggest missing constraint code actions to use exact print

This means replacing all the stringy code for editing instance/signature heads with a ghc-exactprint implementation which respects whitespace and other user formatting. As a bonus, I made it handle missing Monad constraints too. /cc @jhrcek who I believe was the original author.

4. Add suggestions for missing implicit parameters

This was very easy to do and I have a sweet spot for implicit parameters :)

The Rewrite abstraction is similar to D.IDE.GHC.ExactPrint.Graft but it
does fewer things more efficiently:

- It doesn't annotate things for you (so it doesn't destroy user format)
- It doesn't provide a Monoid instance (for now)
- It doesn't need a fully parsed source
- It doesn't use SYB to perform the replacement
- It doesn't diff to compute the result

The use case is code actions where you don't have the SrcSpan that you need to
edit at hand, and instead you need to traverse the AST manually to locate the declaration to edit
Tweaking the suggest constraints tests to reflect the increased precision in
whitespace preservation
@pepeiborra pepeiborra force-pushed the implicit-params-exactprint-cleanup branch from c7aaa05 to bf12e63 Compare January 17, 2021 14:16
Copy link
Collaborator

@ndmitchell ndmitchell left a comment

Choose a reason for hiding this comment

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

Looks good! A few little notes, but cool, I look forward to better completions more robustly.

ghcide/src/Development/IDE/Plugin/CodeAction.hs Outdated Show resolved Hide resolved
ghcide/src/Development/IDE/GHC/ExactPrint.hs Show resolved Hide resolved
@@ -739,7 +739,7 @@ suggestConstraint df parsedModule diag@Diagnostic {..}
where
findMissingConstraint :: T.Text -> Maybe T.Text
findMissingConstraint t =
let regex = "(No instance for|Could not deduce) \\((.+)\\) arising from a use of"
let regex = "(No instance for|Could not deduce) \\((.+)\\) arising from" -- a use of / a do statement
Copy link
Collaborator

Choose a reason for hiding this comment

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

why not push this into the regex? feels weird to have it as a comment next to the regex.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I did not push it into the regex because I don't know if there are other suffixes to this sentence that could result in more opportunities for a quick fix.

Why is it weird to have a comment next to a regex? We do it in most regexes in this module

Copy link
Collaborator

Choose a reason for hiding this comment

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

It feels weird because you are saying "after this regex comes one of these two suffixes". But what you really mean is these two values have been observed in the wild, there might be more, who knows.

@pepeiborra pepeiborra added the merge me Label to trigger pull request merge label Jan 17, 2021
@alanz
Copy link
Collaborator

alanz commented Jan 17, 2021

Not related directly to this PR, but I am currently moving ghc-exactprint into the GHC AST, and open to suggestions about what the API should be. Will be opening a discussion about it soon.

@mergify mergify bot merged commit 0403dbf into master Jan 17, 2021
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