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

Suggest adding pragmas for parse errors too #1165

Merged
merged 4 commits into from
Jan 9, 2021

Conversation

mrBliss
Copy link
Contributor

@mrBliss mrBliss commented Jan 7, 2021

Only errors produced by the type checker were checked for mentions of a pragma
that could be enabled. Many parse errors suggest enabling a pragma:

  • @ -> TypeApplications
  • forall -> RankNTypes. Although ScopedTypeVariables would be a better
    suggestion, IMO.
  • ...

Generate suggestions for these too.

Only errors produced by the type checker were checked for mentions of a pragma
that could be enabled. Many parse errors suggest enabling a pragma:

* `@` -> `TypeApplications`
* `forall` -> `RankNTypes`. Although `ScopedTypeVariables` would be a better
  suggestion, IMO.
* ...

Generate suggestions for these too.
@jneira jneira added the type: enhancement New feature or request label Jan 7, 2021
Copy link
Collaborator

@berberman berberman 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 to me. But there is one thing needs for attention: If a moudle name contains a pragma name, such as test/testdata/addPragmas/TypeApplications.hs, some diagnostics yield by typecheck would trigger findPragma, creating code actions which suggest adding the unrelated pragma.

@mrBliss
Copy link
Contributor Author

mrBliss commented Jan 8, 2021

Looks good to me. But there is one thing needs for attention: If a moudle name contains a pragma name, such as test/testdata/addPragmas/TypeApplications.hs, some diagnostics yield by typecheck would trigger findPragma, creating code actions which suggest adding the unrelated pragma.

What you are saying is true, but that was already the case before this PR.

  • Ideally, we'd have errors-as-values proposal ghc-proposals/ghc-proposals#306 so that we'd only have to search for pragma names in the right field(s?) of the structured GHC error, instead of having to search the entire error message. Even better, the structured GHC error could contain a field with suggested pragmas 🙂.

  • In the meantime, there is another alternative: instead of matching just the pragma names, which might trigger false positives, we could use some regexps like below (copied from my Emacs config):

    (or (and " -X\\([A-Z][A-Za-z]+\\)"
             (not "\\([A-Z][A-Za-z]+\\) is deprecated"))
        "[Uu]se \\([A-Z][A-Za-z]+\\)"
        "Using \\([A-Z][A-Za-z]+\\) might"
        "You need \\([A-Z][A-Za-z]+\\)"
        "Try \\(?:enabling \\)?\\([A-Z][A-Za-z]+\\)"
        "intended to use \\([A-Z][A-Za-z]+\\)"
        "Enable \\(?:the \\)?\\([A-Z][A-Za-z]+\\)"
        "need \\([A-Z][A-Za-z]+\\) turned on"
        "extension '\\([A-Z][A-Za-z]+\\)'")

    (I'm sure the regexp can be optimised, it's just something that grew organically.) However, this is rather ad-hoc and will get out of date when an error message is added/changed in GHC.

  • Question: what do we value most:

    1. All possibly applicable pragmas are suggested, so that the user can always at least add the missing pragma. This might come at the cost of false positives, i.e., the user would have to ignore an incorrect suggestion (like the module name match you pointed out).
    2. Only applicable pragmas are suggested, so that the user is never confused by an incorrect suggestion. This might come at the cost of false negatives, i.e., the user would not always be presented with a suggestion to add the pragma mentioned in the error message.

    (This is completeness vs. soundness.)

    I value (i) over (ii), and I think most pragmatic (pardon the pun) users would agree. I think that already many suggestions (unrelated to pragmas) are made that are not always helpful.

    Before this PR, there were false negatives and false positives. This PR shrinks the number of false negatives. I think the false positives for typechecking errors are unaffected, but there will be more false positives for parse errors (previously, there were no suggestions for parse errors at all).

@mrBliss mrBliss requested a review from pepeiborra January 8, 2021 12:42
@pepeiborra pepeiborra added the merge me Label to trigger pull request merge label Jan 9, 2021
@pepeiborra
Copy link
Collaborator

Awesome, thank you so much!

@mergify mergify bot merged commit 2e78baa into haskell:master Jan 9, 2021
@mrBliss mrBliss deleted the suggest-parse-pragmas branch January 11, 2021 08:32
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 type: enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants