-
-
Notifications
You must be signed in to change notification settings - Fork 435
Implement renaming import aliases #4874
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
Open
izuzu-izuzu
wants to merge
33
commits into
haskell:master
Choose a base branch
from
izuzu-izuzu:rename-qualified-alias
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 27 commits
Commits
Show all changes
33 commits
Select commit
Hold shift + click to select a range
da8c02e
Add test cases, data files
9968162
Add `NOTES.md` containing development notes
4504a10
Get parsed AST
a1927d7
Find import alias declaration at point
a0b78ea
Find all use sites matching target import alias
fa8d1fb
Build text edits for alias declaration, use sites
8e28d1e
Build full edit; add handlers for alias renaming
0fa49a3
Remove redundant import; amend function, comments
a72b796
Support renaming when cursor is on alias use site
c07286e
Remove unused test data files
0587f0b
Simplify finding `RdrName` nodes in parsed source
14ffa2f
Amend approach to handle imports with same alias
257c308
Add new helper module for alias renaming
325119a
Fill in function implementations
1e41e96
Add record type storing import alias information
cc3f072
Avoid cyclic dependency involving `getNamesAtPos`
ea70948
Take responsibility for all AI-assisted code
0138f36
Move alias-renaming logic out of `Rename.hs`
bdb7040
Add tests for alias used by multiple modules
acb6f44
Merge branch 'master' into rename-qualified-alias
f802a14
Use recommended GHC API to ensure compatibility
f97e95f
Handle ambiguous alias due to module re-export
b77b40f
Use code-point positions to avoid UTF-16 issues
6539bae
Reorganize, clean up code; improve readability
f23419f
Move exported functions to top
843d7ac
Return alias range at cursor for `prepareRename`
cf4b935
Remove `NOTES.md`
4c4b25e
Merge branch 'master' into rename-qualified-alias
42ad67c
Allow renaming when cursor is at end of alias
9df3d3b
Rename alias in re-exported names
ef4e33b
Demonstrate that `listify` is lazy
c5ca3b1
Revert "Demonstrate that `listify` is lazy"
fd535c2
Resolve PR comments
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Some comments aren't visible on the classic Files Changed page.
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -53,12 +53,14 @@ import Ide.Logger (Pretty (..), | |
| cmapWithPrio) | ||
| import Ide.Plugin.Error | ||
| import Ide.Plugin.Properties | ||
| import qualified Ide.Plugin.Rename.ImportAlias as ImportAlias | ||
| import qualified Ide.Plugin.Rename.ModuleName as ModuleName | ||
| import Ide.PluginUtils | ||
| import Ide.Types | ||
| import qualified Language.LSP.Protocol.Lens as L | ||
| import Language.LSP.Protocol.Message | ||
| import Language.LSP.Protocol.Types | ||
| import qualified Language.LSP.VFS as VFS | ||
|
|
||
| instance Hashable (Mod a) where hash n = hash (unMod n) | ||
|
|
||
|
|
@@ -88,22 +90,63 @@ descriptor recorder pluginId = mkExactprintPluginDescriptor exactPrintRecorder $ | |
| moduleNameRecorder = cmapWithPrio LogModuleName recorder | ||
|
|
||
| prepareRenameProvider :: PluginMethodHandler IdeState Method_TextDocumentPrepareRename | ||
| prepareRenameProvider state _pluginId (PrepareRenameParams (TextDocumentIdentifier uri) pos _progressToken) = do | ||
| prepareRenameProvider state _pluginId (PrepareRenameParams (TextDocumentIdentifier uri) lspPos _progressToken) = do | ||
| nfp <- getNormalizedFilePathE uri | ||
| namesUnderCursor <- getNamesAtPos state nfp pos | ||
| -- When this handler says that rename is invalid, VSCode shows "The element can't be renamed" | ||
| -- and doesn't even allow you to create full rename request. | ||
| -- This handler deliberately approximates "things that definitely can't be renamed" | ||
| -- to mean "there is no Name at given position". | ||
| -- | ||
| -- In particular it allows some cases through (e.g. cross-module renames), | ||
| -- so that the full rename handler can give more informative error about them. | ||
| let renameValid = not $ null namesUnderCursor | ||
| pure $ InL $ PrepareRenameResult $ InR $ InR $ PrepareRenameDefaultBehavior renameValid | ||
| codePointPos <- getCodePointPosition state nfp lspPos | ||
| maybeParsed <- ImportAlias.getParsedModuleStale state nfp | ||
| case maybeParsed of | ||
| Nothing -> throwError $ PluginInternalError | ||
| "The module hasn’t yet been parsed. Please wait for indexing to complete and try again." | ||
| Just parsed -> do | ||
| let hsModule = unLoc $ pm_parsed_source parsed | ||
| imports = hsmodImports hsModule | ||
| hsDecls = hsmodDecls hsModule | ||
| maybeAlias <- ImportAlias.resolveAliasAtPos | ||
| getNamesAtPos state nfp lspPos codePointPos imports hsDecls | ||
| case maybeAlias of | ||
| Just (lspRange, _importAlias) -> | ||
| pure $ InL $ PrepareRenameResult $ InL $ lspRange | ||
| Nothing -> do | ||
| -- When this handler says that rename is invalid, VSCode shows "The element can't be renamed" | ||
| -- and doesn't even allow you to create full rename request. | ||
| -- This handler deliberately approximates "things that definitely can't be renamed" | ||
| -- to mean "there is no Name at given position". | ||
| -- | ||
| -- In particular it allows some cases through (e.g. cross-module renames), | ||
| -- so that the full rename handler can give more informative error about them. | ||
| namesUnderCursor <- getNamesAtPos state nfp lspPos | ||
| let renameValid = not $ null namesUnderCursor | ||
| pure $ InL $ PrepareRenameResult $ InR $ InR $ PrepareRenameDefaultBehavior renameValid | ||
|
|
||
| renameProvider :: PluginMethodHandler IdeState Method_TextDocumentRename | ||
| renameProvider state pluginId (RenameParams _prog (TextDocumentIdentifier uri) pos newNameText) = do | ||
| renameProvider state pluginId (RenameParams _prog (TextDocumentIdentifier uri) lspPos newNameText) = do | ||
| nfp <- getNormalizedFilePathE uri | ||
| codePointPos <- getCodePointPosition state nfp lspPos | ||
| maybeParsed <- ImportAlias.getParsedModuleStale state nfp | ||
|
Collaborator
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. There's an argument to be made that the |
||
| case maybeParsed of | ||
| Nothing -> throwError $ PluginInternalError | ||
| "The module hasn’t yet been parsed. Please wait for indexing to complete and try again." | ||
| Just parsed -> do | ||
| let hsModule = unLoc $ pm_parsed_source parsed | ||
| imports = hsmodImports hsModule | ||
| hsDecls = hsmodDecls hsModule | ||
| maybeAlias <- ImportAlias.resolveAliasAtPos | ||
| getNamesAtPos state nfp lspPos codePointPos imports hsDecls | ||
| case maybeAlias of | ||
| Just (_lspRange, importAlias) -> ImportAlias.aliasBasedRename | ||
| state nfp uri importAlias hsDecls newNameText | ||
| Nothing -> | ||
| nameBasedRename state pluginId nfp lspPos newNameText | ||
|
|
||
| -- | Name-based rename: the original rename logic. | ||
| nameBasedRename :: | ||
| IdeState -> | ||
| PluginId -> | ||
| NormalizedFilePath -> | ||
| Position -> | ||
| T.Text -> | ||
| ExceptT PluginError (HandlerM config) (MessageResult Method_TextDocumentRename) | ||
| nameBasedRename state pluginId nfp pos newNameText = do | ||
| directOldNames <- getNamesAtPos state nfp pos | ||
| directRefs <- concat <$> mapM (refsAtName state nfp) directOldNames | ||
|
|
||
|
|
@@ -182,17 +225,14 @@ replaceRefs :: | |
| HashSet Location -> | ||
| ParsedSource -> | ||
| ParsedSource | ||
| replaceRefs newName refs = everywhere $ | ||
| -- there has to be a better way... | ||
| mkT (replaceLoc @AnnListItem) `extT` | ||
| -- replaceLoc @AnnList `extT` -- not needed | ||
| -- replaceLoc @AnnParen `extT` -- not needed | ||
| -- replaceLoc @AnnPragma `extT` -- not needed | ||
| -- replaceLoc @AnnContext `extT` -- not needed | ||
| -- replaceLoc @NoEpAnns `extT` -- not needed | ||
| replaceLoc @NameAnn | ||
| replaceRefs newName refs = everywhere (mkT replaceLoc) | ||
| where | ||
| replaceLoc :: forall an. LocatedAn an RdrName -> LocatedAn an RdrName | ||
| -- See Note [XRec and SrcSpans in the AST] in Language.Haskell.Syntax.Extension | ||
| -- See Note [XRec and Anno in the AST] in GHC.Parser.Annotation | ||
| -- GHC recommends using 'XRec' (available since 9.4.8 or earlier) to | ||
| -- get the right annotation type for a given target type. | ||
| -- XRec (GhcPass 'Parsed) RdrName = GenLocated (Anno RdrName) RdrName | ||
| replaceLoc :: XRec (GhcPass 'Parsed) RdrName -> XRec (GhcPass 'Parsed) RdrName | ||
| replaceLoc (L srcSpan oldRdrName) | ||
| | isRef (locA srcSpan) = L srcSpan $ replace oldRdrName | ||
| replaceLoc lOldRdrName = lOldRdrName | ||
|
|
@@ -220,6 +260,7 @@ refsAtName state nfp name = do | |
| Nothing -> pure [] | ||
| Just mod -> liftIO $ mapMaybe rowToLoc <$> withHieDb (\hieDb -> | ||
| -- See Note [Generated references] | ||
| -- REVIEW: Is this filter supposed to keep or remove generated references? | ||
| filter (\(refRow HieDb.:. _) -> refIsGenerated refRow) <$> | ||
|
vidit-od marked this conversation as resolved.
|
||
| findReferences | ||
| hieDb | ||
|
|
@@ -239,6 +280,28 @@ nameLocs name (HAR _ _ rm _ _) = | |
| --------------------------------------------------------------------------------------------------- | ||
| -- Util | ||
|
|
||
| -- | Convert an LSP position (based on UTF-16 code units) to a position based on | ||
| -- whole Unicode code points. | ||
| getCodePointPosition :: | ||
| MonadIO m => | ||
| IdeState -> | ||
| NormalizedFilePath -> | ||
| Position -> | ||
| ExceptT PluginError m VFS.CodePointPosition | ||
| getCodePointPosition state nfp pos = do | ||
| virtualFile <- runActionE "rename.getVirtualFile" state | ||
| $ handleMaybeM (PluginInternalError | ||
| ("Virtual file not found: " <> T.pack (show nfp))) | ||
| $ getVirtualFile nfp | ||
| case VFS.positionToCodePointPosition virtualFile pos of | ||
| Nothing -> throwError $ PluginInvalidParams | ||
| "The cursor position is inside a Unicode surrogate pair." | ||
| Just codePointPosition -> pure codePointPosition | ||
|
|
||
| -- TODO: 'getNamesAtPos' passes the LSP 'Position' directly to 'pointCommand', | ||
| -- which treats '_character' as a code-point column. This is incorrect for | ||
| -- files with supplementary-plane Unicode characters before the cursor. | ||
| -- Fixing it requires changes in ghcide, not here. | ||
| getNamesAtPos :: MonadIO m => IdeState -> NormalizedFilePath -> Position -> ExceptT PluginError m [Name] | ||
| getNamesAtPos state nfp pos = do | ||
| HAR{hieAst} <- handleGetHieAst state nfp | ||
|
|
||
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
The previous version used
handleGetHieAst, which has a comment saying that renames should explicitly use non-stale versions to ensure consistency. Doesn't this have that same problem? This is theprepareversion of the request so stale results should be fine I think. How about usinguseWithStaleFast*instead of juststale, as this is a interactive action from what I can tell?On a secondary note, isn't the existing
HieAstenough to get the import module aliases?Looking at the
HieAstof aimport Data.Map.Strict as Mapin this file, gives me the following nodes.hiedb dump of HieDb/Dump.hs