-
-
Notifications
You must be signed in to change notification settings - Fork 389
support add-argument action #3149
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
Changes from all commits
51e0d1c
5206ce2
cae508a
be159c6
3c6fba3
4be40b7
2627118
9388c54
3a0a8f9
6bc2454
57b4edb
123a4f0
9a232d2
01cc14e
f2659a7
136c232
c39b6f3
34e7277
fceae39
0fce830
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -38,6 +38,7 @@ import Data.Ord (comparing) | |
import qualified Data.Set as S | ||
import qualified Data.Text as T | ||
import qualified Data.Text.Utf16.Rope as Rope | ||
import Data.Tuple.Extra (first) | ||
import Development.IDE.Core.Rules | ||
import Development.IDE.Core.RuleTypes | ||
import Development.IDE.Core.Service | ||
|
@@ -63,7 +64,8 @@ import Development.IDE.Types.Logger hiding | |
import Development.IDE.Types.Options | ||
import GHC.Exts (fromList) | ||
import qualified GHC.LanguageExtensions as Lang | ||
import Ide.PluginUtils (subRange) | ||
import Ide.PluginUtils (makeDiffTextEdit, | ||
subRange) | ||
import Ide.Types | ||
import qualified Language.LSP.Server as LSP | ||
import Language.LSP.Types (ApplyWorkspaceEditParams (..), | ||
|
@@ -89,7 +91,13 @@ import Language.LSP.VFS (VirtualFile, | |
import qualified Text.Fuzzy.Parallel as TFP | ||
import Text.Regex.TDFA (mrAfter, | ||
(=~), (=~~)) | ||
#if MIN_VERSION_ghc(9,2,1) | ||
import GHC.Types.SrcLoc (generatedSrcSpan) | ||
import Language.Haskell.GHC.ExactPrint (noAnnSrcSpanDP1, | ||
runTransformT) | ||
#endif | ||
#if MIN_VERSION_ghc(9,2,0) | ||
import Extra (maybeToEither) | ||
import GHC (AddEpAnn (AddEpAnn), | ||
Anchor (anchor_op), | ||
AnchorOperation (..), | ||
|
@@ -168,6 +176,9 @@ bindingsPluginDescriptor recorder plId = mkExactprintPluginDescriptor recorder $ | |
, wrap suggestImplicitParameter | ||
#endif | ||
, wrap suggestNewDefinition | ||
#if MIN_VERSION_ghc(9,2,1) | ||
, wrap suggestAddArgument | ||
#endif | ||
, wrap suggestDeleteUnusedBinding | ||
] | ||
plId | ||
|
@@ -243,7 +254,7 @@ extendImportHandler' ideState ExtendImport {..} | |
Nothing -> newThing | ||
Just p -> p <> "(" <> newThing <> ")" | ||
t <- liftMaybe $ snd <$> newImportToEdit n ps (fromMaybe "" contents) | ||
return (nfp, WorkspaceEdit {_changes=Just (fromList [(doc,List [t])]), _documentChanges=Nothing, _changeAnnotations=Nothing}) | ||
return (nfp, WorkspaceEdit {_changes=Just (GHC.Exts.fromList [(doc,List [t])]), _documentChanges=Nothing, _changeAnnotations=Nothing}) | ||
| otherwise = | ||
mzero | ||
|
||
|
@@ -385,7 +396,7 @@ suggestHideShadow ps fileContents mTcM mHar Diagnostic {_message, _range} | |
Just matched <- allMatchRegexUnifySpaces _message "imported from ‘([^’]+)’ at ([^ ]*)", | ||
mods <- [(modName, s) | [_, modName, s] <- matched], | ||
result <- nubOrdBy (compare `on` fst) $ mods >>= uncurry (suggests identifier), | ||
hideAll <- ("Hide " <> identifier <> " from all occurence imports", concat $ snd <$> result) = | ||
hideAll <- ("Hide " <> identifier <> " from all occurence imports", concatMap snd result) = | ||
result <> [hideAll] | ||
| otherwise = [] | ||
where | ||
|
@@ -881,34 +892,111 @@ suggestReplaceIdentifier contents Diagnostic{_range=_range,..} | |
= [ ("Replace with ‘" <> name <> "’", [mkRenameEdit contents _range name]) | name <- renameSuggestions ] | ||
| otherwise = [] | ||
|
||
matchVariableNotInScope :: T.Text -> Maybe (T.Text, Maybe T.Text) | ||
matchVariableNotInScope message | ||
-- * Variable not in scope: | ||
-- suggestAcion :: Maybe T.Text -> Range -> Range | ||
-- * Variable not in scope: | ||
-- suggestAcion | ||
| Just (name, typ) <- matchVariableNotInScopeTyped message = Just (name, Just typ) | ||
| Just name <- matchVariableNotInScopeUntyped message = Just (name, Nothing) | ||
| otherwise = Nothing | ||
where | ||
matchVariableNotInScopeTyped message | ||
| Just [name, typ] <- matchRegexUnifySpaces message "Variable not in scope: ([^ ]+) :: ([^*•]+)" = | ||
Just (name, typ) | ||
| otherwise = Nothing | ||
matchVariableNotInScopeUntyped message | ||
| Just [name] <- matchRegexUnifySpaces message "Variable not in scope: ([^ ]+)" = | ||
Just name | ||
| otherwise = Nothing | ||
|
||
matchFoundHole :: T.Text -> Maybe (T.Text, T.Text) | ||
matchFoundHole message | ||
| Just [name, typ] <- matchRegexUnifySpaces message "Found hole: _([^ ]+) :: ([^*•]+) Or perhaps" = | ||
Just (name, typ) | ||
| otherwise = Nothing | ||
|
||
matchFoundHoleIncludeUnderscore :: T.Text -> Maybe (T.Text, T.Text) | ||
matchFoundHoleIncludeUnderscore message = first ("_" <>) <$> matchFoundHole message | ||
|
||
suggestNewDefinition :: IdeOptions -> ParsedModule -> Maybe T.Text -> Diagnostic -> [(T.Text, [TextEdit])] | ||
suggestNewDefinition ideOptions parsedModule contents Diagnostic{_message, _range} | ||
-- * Variable not in scope: | ||
-- suggestAcion :: Maybe T.Text -> Range -> Range | ||
| Just [name, typ] <- matchRegexUnifySpaces message "Variable not in scope: ([^ ]+) :: ([^*•]+)" | ||
= newDefinitionAction ideOptions parsedModule _range name typ | ||
| Just [name, typ] <- matchRegexUnifySpaces message "Found hole: _([^ ]+) :: ([^*•]+) Or perhaps" | ||
, [(label, newDefinitionEdits)] <- newDefinitionAction ideOptions parsedModule _range name typ | ||
= [(label, mkRenameEdit contents _range name : newDefinitionEdits)] | ||
| otherwise = [] | ||
where | ||
message = unifySpaces _message | ||
suggestNewDefinition ideOptions parsedModule contents Diagnostic {_message, _range} | ||
| Just (name, typ) <- matchVariableNotInScope message = | ||
newDefinitionAction ideOptions parsedModule _range name typ | ||
| Just (name, typ) <- matchFoundHole message, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think you have a Plan. Something like:
But the Plan is not written down anywhere, and as a reader it's hard to figure out what it is. Maybe worth writing it down somewhere and referring to it? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Revised |
||
[(label, newDefinitionEdits)] <- newDefinitionAction ideOptions parsedModule _range name (Just typ) = | ||
[(label, mkRenameEdit contents _range name : newDefinitionEdits)] | ||
| otherwise = [] | ||
where | ||
message = unifySpaces _message | ||
|
||
newDefinitionAction :: IdeOptions -> ParsedModule -> Range -> T.Text -> T.Text -> [(T.Text, [TextEdit])] | ||
newDefinitionAction IdeOptions{..} parsedModule Range{_start} name typ | ||
| Range _ lastLineP : _ <- | ||
newDefinitionAction :: IdeOptions -> ParsedModule -> Range -> T.Text -> Maybe T.Text -> [(T.Text, [TextEdit])] | ||
newDefinitionAction IdeOptions {..} parsedModule Range {_start} name typ | ||
| Range _ lastLineP : _ <- | ||
[ realSrcSpanToRange sp | ||
| (L (locA -> l@(RealSrcSpan sp _)) _) <- hsmodDecls | ||
, _start `isInsideSrcSpan` l] | ||
, nextLineP <- Position{ _line = _line lastLineP + 1, _character = 0} | ||
= [ ("Define " <> sig | ||
, [TextEdit (Range nextLineP nextLineP) (T.unlines ["", sig, name <> " = _"])] | ||
)] | ||
| otherwise = [] | ||
| (L (locA -> l@(RealSrcSpan sp _)) _) <- hsmodDecls, | ||
_start `isInsideSrcSpan` l | ||
], | ||
nextLineP <- Position {_line = _line lastLineP + 1, _character = 0} = | ||
[ ( "Define " <> sig, | ||
[TextEdit (Range nextLineP nextLineP) (T.unlines ["", sig, name <> " = _"])] | ||
) | ||
] | ||
| otherwise = [] | ||
where | ||
colon = if optNewColonConvention then " : " else " :: " | ||
sig = name <> colon <> T.dropWhileEnd isSpace typ | ||
ParsedModule{pm_parsed_source = L _ HsModule{hsmodDecls}} = parsedModule | ||
sig = name <> colon <> T.dropWhileEnd isSpace (fromMaybe "_" typ) | ||
ParsedModule {pm_parsed_source = L _ HsModule {hsmodDecls}} = parsedModule | ||
|
||
#if MIN_VERSION_ghc(9,2,1) | ||
-- When GHC tells us that a variable is not bound, it will tell us either: | ||
-- - there is an unbound variable with a given type | ||
-- - there is an unbound variable (GHC provides no type suggestion) | ||
-- | ||
-- When we receive either of these errors, we produce a text edit that will add a new argument (as a new pattern in the | ||
-- last position of each LHS of the top-level bindings for this HsDecl). | ||
-- | ||
-- TODO Include logic to also update the type signature of a binding | ||
-- | ||
-- NOTE When adding a new argument to a declaration, the corresponding argument's type in declaration's signature might | ||
-- not be the last type in the signature, such as: | ||
-- foo :: a -> b -> c -> d | ||
-- foo a b = \c -> ... | ||
-- In this case a new argument would have to add its type between b and c in the signature. | ||
suggestAddArgument :: ParsedModule -> Diagnostic -> Either ResponseError [(T.Text, [TextEdit])] | ||
suggestAddArgument parsedModule Diagnostic {_message, _range} | ||
| Just (name, typ) <- matchVariableNotInScope message = addArgumentAction parsedModule _range name typ | ||
| Just (name, typ) <- matchFoundHoleIncludeUnderscore message = addArgumentAction parsedModule _range name (Just typ) | ||
| otherwise = pure [] | ||
where | ||
message = unifySpaces _message | ||
|
||
-- TODO use typ to modify type signature | ||
santiweight marked this conversation as resolved.
Show resolved
Hide resolved
|
||
addArgumentAction :: ParsedModule -> Range -> T.Text -> Maybe T.Text -> Either ResponseError [(T.Text, [TextEdit])] | ||
addArgumentAction (ParsedModule _ parsedSource _ _) range name _typ = | ||
do | ||
let addArgToMatch (L locMatch (Match xMatch ctxMatch pats rhs)) = do | ||
let unqualName = mkRdrUnqual $ mkVarOcc $ T.unpack name | ||
let newPat = L (noAnnSrcSpanDP1 generatedSrcSpan) $ VarPat NoExtField (noLocA unqualName) | ||
pure $ L locMatch (Match xMatch ctxMatch (pats <> [newPat]) rhs) | ||
insertArg = \case | ||
(L locDecl (ValD xVal (FunBind xFunBind idFunBind mg coreFunBind))) -> do | ||
mg' <- modifyMgMatchesT mg addArgToMatch | ||
let decl' = L locDecl (ValD xVal (FunBind xFunBind idFunBind mg' coreFunBind)) | ||
pure [decl'] | ||
decl -> pure [decl] | ||
case runTransformT $ modifySmallestDeclWithM spanContainsRangeOrErr insertArg (makeDeltaAst parsedSource) of | ||
Left err -> Left err | ||
Right (newSource, _, _) -> | ||
let diff = makeDiffTextEdit (T.pack $ exactPrint parsedSource) (T.pack $ exactPrint newSource) | ||
in pure [("Add argument ‘" <> name <> "’ to function", fromLspList diff)] | ||
where | ||
spanContainsRangeOrErr = maybeToEither (responseError "SrcSpan was not valid range") . (`spanContainsRange` range) | ||
#endif | ||
|
||
fromLspList :: List a -> [a] | ||
fromLspList (List a) = a | ||
|
||
suggestFillTypeWildcard :: Diagnostic -> [(T.Text, TextEdit)] | ||
suggestFillTypeWildcard Diagnostic{_range=_range,..} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps an annoying suggestion, but these matching functions are all nice pure functions that could benefit from some direct tests checking that they do definitely match all the cases you care about.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, this module is also quite large, perhaps the add-action stuff could go in a separate module also?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually already did this in a followup MR. Would you be okay with following up with this change (to avoid unnecessary conflicts)?