Skip to content
This repository has been archived by the owner on Jan 2, 2021. It is now read-only.

Code action: remove redundant constraints for type signature #692

Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
73 changes: 73 additions & 0 deletions src/Development/IDE/Plugin/CodeAction.hs
Original file line number Diff line number Diff line change
Expand Up @@ -147,6 +147,7 @@ suggestAction dflags packageExports ideOptions parsedModule text diag = concat
, suggestReplaceIdentifier text diag
, suggestSignature True diag
, suggestConstraint text diag
, removeRedundantConstraints text diag
, suggestAddTypeAnnotationToSatisfyContraints text diag
] ++ concat
[ suggestNewDefinition ideOptions pm text diag
Expand Down Expand Up @@ -586,6 +587,78 @@ suggestFunctionConstraint contents Diagnostic{..} missingConstraint
actionTitle constraint typeSignatureName = "Add `" <> constraint
<> "` to the context of the type signature for `" <> typeSignatureName <> "`"

-- | Suggests the removal of a redundant constraint for a type signature.
removeRedundantConstraints :: Maybe T.Text -> Diagnostic -> [(T.Text, [TextEdit])]
removeRedundantConstraints mContents Diagnostic{..}
-- • Redundant constraint: Eq a
-- • In the type signature for:
-- foo :: forall a. Eq a => a -> a
-- • Redundant constraints: (Monoid a, Show a)
-- • In the type signature for:
-- foo :: forall a. (Num a, Monoid a, Eq a, Show a) => a -> Bool
| Just contents <- mContents
-- Account for both "Redundant constraint" and "Redundant constraints".
, True <- "Redundant constraint" `T.isInfixOf` _message
, Just typeSignatureName <- findTypeSignatureName _message
, Just redundantConstraintList <- findRedundantConstraints _message
= let constraints = findConstraints contents typeSignatureName
constraintList = parseConstraints constraints
newConstraints = buildNewConstraints constraintList redundantConstraintList
typeSignatureLine = findTypeSignatureLine contents typeSignatureName
typeSignatureFirstChar = T.length $ typeSignatureName <> " :: "
startOfConstraint = Position typeSignatureLine typeSignatureFirstChar
endOfConstraint = Position typeSignatureLine $
typeSignatureFirstChar + T.length (constraints <> " => ")
range = Range startOfConstraint endOfConstraint
in [(actionTitle redundantConstraintList typeSignatureName, [TextEdit range newConstraints])]
| otherwise = []
where
parseConstraints :: T.Text -> [T.Text]
parseConstraints = stripConstraintsParens >>> T.splitOn ", "

stripConstraintsParens :: T.Text -> T.Text
stripConstraintsParens constraints =
if "(" `T.isPrefixOf` constraints
then constraints & T.drop 1 & T.dropEnd 1 & T.strip
else constraints

findRedundantConstraints :: T.Text -> Maybe [T.Text]
findRedundantConstraints t = t
& T.lines
& head
& T.strip
& (`matchRegex` "Redundant constraints?: (.+)")
<&> (head >>> parseConstraints)

findConstraints :: T.Text -> T.Text -> T.Text
findConstraints contents typeSignatureName = contents
Copy link
Contributor Author

Choose a reason for hiding this comment

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

On a second thought, I've realised that this will fail if the type signature is not formatted as expected, for example with an arbitrary number of spaces or written over multiple lines (see also lines 608 and 611).

While I don't reckon it could brake the code, this is rather poorly handled. I see two ways to improve it:

  • trivial one: make it a bit safer and do nothing if the code is not formatted as expected;
  • trickier: improve the logic in order to handle any format. I'm not sure this would be achievable with regular expressions and Text functions though, so maybe some text parsing should be involved?

What do you think?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Usually do the trivial one now, and then handle the trickier one in a follow up PR if its a problem in practice.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Doing the trivial one now is totally fine but we should do nothing if the formatting doesn’t match our expectations instead of crashing which the last and head calls will do.

& T.splitOn (typeSignatureName <> " ::")
& last
& T.splitOn "=>"
& head
& T.strip

formatConstraints :: [T.Text] -> T.Text
formatConstraints [] = ""
formatConstraints [constraint] = constraint
formatConstraints constraintList = constraintList
& T.intercalate ", "
& \cs -> "(" <> cs <> ")"

formatConstraintsWithArrow :: [T.Text] -> T.Text
formatConstraintsWithArrow [] = ""
formatConstraintsWithArrow cs = cs & formatConstraints & (<> " => ")

buildNewConstraints :: [T.Text] -> [T.Text] -> T.Text
buildNewConstraints constraintList redundantConstraintList =
formatConstraintsWithArrow $ constraintList \\ redundantConstraintList

actionTitle :: [T.Text] -> T.Text -> T.Text
actionTitle constraintList typeSignatureName =
"Remove redundant constraint" <> (if length constraintList == 1 then "" else "s") <> " `"
<> formatConstraints constraintList
<> "` from the context of the type signature for `" <> typeSignatureName <> "`"

-------------------------------------------------------------------------------------------------

suggestNewImport :: PackageExportsMap -> ParsedModule -> Diagnostic -> [(T.Text, [TextEdit])]
Expand Down
52 changes: 51 additions & 1 deletion test/exe/Main.hs
Original file line number Diff line number Diff line change
Expand Up @@ -483,6 +483,7 @@ codeActionTests = testGroup "code actions"
, deleteUnusedDefinitionTests
, addInstanceConstraintTests
, addFunctionConstraintTests
, removeRedundantConstraintsTests
, addTypeAnnotationsToLiteralsTest
]

Expand Down Expand Up @@ -1261,7 +1262,7 @@ addTypeAnnotationsToLiteralsTest = testGroup "add type annotations to literals t
, ""
, "import Debug.Trace"
, ""
, "f a = traceShow \"debug\" a"
, "f a = traceShow \"debug\" a"
])
[ (DsWarning, (6, 6), "Defaulting the following constraint") ]
"Add type annotation ‘[Char]’ to ‘\"debug\"’"
Expand Down Expand Up @@ -1550,6 +1551,55 @@ addFunctionConstraintTests = let
(incompleteConstraintSourceCode2 $ Just "Eq c")
]

removeRedundantConstraintsTests :: TestTree
removeRedundantConstraintsTests = let
header =
[ "{-# OPTIONS_GHC -Wredundant-constraints #-}"
, "module Testing where"
, ""
]

redundantConstraintsCode :: Maybe T.Text -> T.Text
redundantConstraintsCode mConstraint =
let constraint = maybe "" (\c -> "" <> c <> " => ") mConstraint
in T.unlines $ header <>
[ "foo :: " <> constraint <> "a -> a"
, "foo = id"
]

redundantMixedConstraintsCode :: Maybe T.Text -> T.Text
redundantMixedConstraintsCode mConstraint =
let constraint = maybe "(Num a, Eq a)" (\c -> "(Num a, Eq a, " <> c <> ")") mConstraint
in T.unlines $ header <>
[ "foo :: " <> constraint <> " => a -> Bool"
, "foo x = x == 1"
]

check :: T.Text -> T.Text -> T.Text -> TestTree
check actionTitle originalCode expectedCode = testSession (T.unpack actionTitle) $ do
doc <- createDoc "Testing.hs" "haskell" originalCode
_ <- waitForDiagnostics
actionsOrCommands <- getCodeActions doc (Range (Position 4 0) (Position 4 maxBound))
chosenAction <- liftIO $ pickActionWithTitle actionTitle actionsOrCommands
executeCodeAction chosenAction
modifiedCode <- documentContents doc
liftIO $ expectedCode @=? modifiedCode

in testGroup "remove redundant function constraints"
[ check
"Remove redundant constraint `Eq a` from the context of the type signature for `foo`"
(redundantConstraintsCode $ Just "Eq a")
(redundantConstraintsCode Nothing)
, check
"Remove redundant constraints `(Eq a, Monoid a)` from the context of the type signature for `foo`"
(redundantConstraintsCode $ Just "(Eq a, Monoid a)")
(redundantConstraintsCode Nothing)
, check
"Remove redundant constraints `(Monoid a, Show a)` from the context of the type signature for `foo`"
(redundantMixedConstraintsCode $ Just "Monoid a, Show a")
(redundantMixedConstraintsCode Nothing)
]

addSigActionTests :: TestTree
addSigActionTests = let
header = "{-# OPTIONS_GHC -Wmissing-signatures -Wmissing-pattern-synonym-signatures #-}"
Expand Down