Repository navigation
Add code action to remove redundant qualified in imports - #5145
BHARATH0153 wants to merge 6 commits into
Conversation
a608d8b to
1b3c6ab
Compare
|
Hi, thank you for your contribution! I think it would be generally good if you discussed your implementation approach on the ticket before actually working on it.
That's not true at all, in most cases we have old DynFlags around from the last time it parsed! I have also a question regarding the implementation, you seem to manually splice out the |
|
@fendor Thanks for the review! You're right about DynFlags. I updated the implementation to pull them from As for the manual splicing, since the module fails to parse with "Multiple occurrences of 'qualified'", there's usually no reliable parsed AST to run exactprint rewrites against. A small, whitespace-aware text edit on the specific line seemed like the most robust and minimal approach for this particular parser error. I did consider an AST-based approach, but in the unparseable case it wasn't straightforward. If there's an existing pattern for fixing syntax errors like this in HLS, I'd be happy to refactor to follow it! Also noted about discussing the approach on the ticket first, I'll do that going forward. Thanks! |
b684dc9 to
a3d1ba7
Compare
fendor
left a comment
fendor
left a comment
There was a problem hiding this comment.
I think the approach should be reconsidered, there is lots of parsing and ad-hoc logic happening.
Trying to parse any kind of haskell syntax ad-hoc is a bad idea, in my mind.
If we can't use the AST directly, we should be doing as little as possible.
Given that the source range points exactly at the qualified, shouldn't this just be a matter of generating a text edit that replaces the diagnostic range with ""?
| importQualifiedPostFromPragmas :: T.Text -> Bool | ||
| importQualifiedPostFromPragmas contents = |
There was a problem hiding this comment.
I don't think we should implement this fall back behaviour, I think it is fine to not offer anything if we can't find the DynFlags. This ad-hoc parsing usually just leads to more tears down the line.
| redundantQualifiedEdit contents = do | ||
| let Position startLine _ = _start _range | ||
| line <- listToMaybe $ drop (fromIntegral startLine) $ T.lines contents | ||
| (preQual, postQual) <- case qualifiedOccurrences line of | ||
| a : b : _ -> Just (a, b) | ||
| _ -> Nothing | ||
| let (colStart, colEnd) | ||
| | usePost = preQual | ||
| | otherwise = postQual | ||
| baseRange = Range (Position startLine (fromIntegral colStart)) (Position startLine (fromIntegral colEnd)) | ||
| trailing = T.takeWhile isSpace (T.drop colEnd line) | ||
| leading = T.takeWhileEnd isSpace (T.take colStart line) | ||
| editRange | ||
| | usePost = baseRange { _end = Position startLine (fromIntegral (colEnd + T.length trailing)) } | ||
| | otherwise = baseRange { _start = Position startLine (fromIntegral (colStart - T.length leading)) } | ||
| pure $ TextEdit editRange "" |
There was a problem hiding this comment.
This might need a comment what this is trying to do. Seems very complicated.
| | "Multiple occurrences of 'qualified'" `T.isInfixOf` _message | ||
| , Just contents <- contents' | ||
| , Just edit <- redundantQualifiedEdit contents |
There was a problem hiding this comment.
Could we use the structured diagnostic in FileDiagnostic instead of parsing the message?
Also, the diagnostic has a source range, isn't that sufficient to calculate the edit?
| -- The module could not be parsed and no DynFlags were available, so | ||
| -- fall back to the LANGUAGE/OPTIONS_GHC pragmas in the file itself. | ||
| Nothing -> maybe False importQualifiedPostFromPragmas contents' |
There was a problem hiding this comment.
I am in all honesty pretty much against this ad-hoc parsing of line pragmas. If there are no DynFlags, just assume a default.
Closes #5121.
Malformed imports such as
produce the parser error
Multiple occurrences of 'qualified'. This adds aRemove redundant 'qualified'code action that drops one of the twooccurrences:
qualifiedwhenImportQualifiedPostis enabled, producingimport Data.List qualified;qualifiedotherwise, producingimport qualified Data.List.Because the module fails to parse, no
DynFlagsare available for the file, sothe action decides which occurrence to remove by inspecting the diagnostics in
the requested range: when
ImportQualifiedPostis off GHC additionally reportsthe postpositive
qualifiedas not enabled. This exposes the in-rangediagnostics to code actions through a new
caaDiagnosticsfield and[FileDiagnostic] -> rinstance.