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 remove constraint #1578

Merged
merged 8 commits into from
May 2, 2021
Merged

Fix remove constraint #1578

merged 8 commits into from
May 2, 2021

Conversation

kderme
Copy link
Contributor

@kderme kderme commented Mar 15, 2021

This pr refactors the code actions which remove redundant constraints from function signatures.

remove-constraint

The new approach traverses the ast to find the related Sig and uses the ExactPrint functionality to create a new signature. The old approach did some text manipulation of the module, which was not very reliable. This caused a number of issues which are now fixed:

  • code actions for signatures which are not top level were wrong:
f :: Int -> ()
f = g
  where
    g :: Eq a => a -> ()
  -- abcdefghijklmn
    g _ = ()

resulted in

f :: Int -> ()
f = g
  where
    g :: Eq a => a -> ()
  -- ijklmn
    g _ = ()
  • signatures with an explicit forall were not fixed at all
g :: forall a. Show a => a -> ()
g _ = ()

created no visible change

  • signatures with spaces created no code action
g ::  Show a => a -> ()
g _ = ()

Tests are failing (hence the draft) because ExactPrint creates an unnecessary space. I'm still trying to understand why and exactly how TransformT works

I think the pr also helps fixing #1300, but doesn't really fix it.

@kderme kderme force-pushed the fix-remove-constraint branch from e0c69e4 to f88a5f8 Compare March 15, 2021 18:43
@kderme kderme force-pushed the fix-remove-constraint branch from f88a5f8 to 58e95fc Compare March 15, 2021 19:11
@kderme
Copy link
Contributor Author

kderme commented Mar 15, 2021

Test failures. Not sure where this space comes from 🤔

    remove redundant function constraints
      Remove redundant constraint `Eq a` from the context of the type signature for `foo`:                FAIL (1.64s)
        test/exe/Main.hs:2681:
        expected: "{-# OPTIONS_GHC -Wredundant-constraints #-}\nmodule Testing where\n\nfoo :: a -> a\nfoo = id\n"
         but got: "{-# OPTIONS_GHC -Wredundant-constraints #-}\nmodule Testing where\n\nfoo ::  a -> a\nfoo = id\n"
      Remove redundant constraints `(Eq a, Monoid a)` from the context of the type signature for `foo`:   FAIL (1.82s)
        test/exe/Main.hs:2681:
        expected: "{-# OPTIONS_GHC -Wredundant-constraints #-}\nmodule Testing where\n\nfoo :: a -> a\nfoo = id\n"
         but got: "{-# OPTIONS_GHC -Wredundant-constraints #-}\nmodule Testing where\n\nfoo ::  a -> a\nfoo = id\n"

@kderme kderme marked this pull request as ready for review March 18, 2021 07:58
berberman
berberman previously approved these changes Mar 18, 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.

LGTM, and tests are great! Thank you!

ghcide/src/Development/IDE/Plugin/CodeAction.hs Outdated Show resolved Hide resolved
@jneira jneira requested a review from berberman March 23, 2021 09:21
@jneira jneira dismissed berberman’s stale review March 23, 2021 09:22

Requested changes thereafter

@jneira jneira added the status: needs info Not actionable, because there's missing information label Apr 13, 2021
@jneira jneira removed the status: needs info Not actionable, because there's missing information label May 1, 2021
@berberman berberman added the merge me Label to trigger pull request merge label May 2, 2021
@mergify mergify bot merged commit 35927c8 into haskell:master May 2, 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.

4 participants