Renaming of indirect references (RecordFieldPuns) (#3013)

* test: add tests for record puns

* feat: rename indirect references
refactor: remove "safe" from function names

* test: ignore record field tests for ghc92 (#2915)

* test: ignore record field tests for ghc90 (#2915)

* fix: update record field test ignore message

* expand comment about indirect reference renaming

* fix: find all punned references

* test: ignore record field pun test for ghc > 9

* docs: mention test in indirect pun explaination

* link issue for ignored record field rename tests

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>

Author: OliverMadine

plugins/hls-rename-plugin/src/Ide/Plugin/Rename.hs

@@ -24,7 +24,7 @@ import           Data.Generics
 import           Data.Hashable
 import           Data.HashSet                          (HashSet)
 import qualified Data.HashSet                          as HS
-import           Data.List.Extra
+import           Data.List.Extra                       hiding (length)
 import qualified Data.Map                              as M
 import           Data.Maybe
 import           Data.Mod.Word
@@ -42,7 +42,6 @@ import           Development.IDE.GHC.ExactPrint
 import           Development.IDE.Spans.AtPoint
 import           Development.IDE.Types.Location
 import           HieDb.Query
-import           Ide.Plugin.Config
 import           Ide.Plugin.Properties
 import           Ide.PluginUtils
 import           Ide.Types
@@ -65,16 +64,28 @@ descriptor pluginId = (defaultPluginDescriptor pluginId)
 renameProvider :: PluginMethodHandler IdeState TextDocumentRename
 renameProvider state pluginId (RenameParams (TextDocumentIdentifier uri) pos _prog newNameText) =
     pluginResponse $ do
-        nfp <- safeUriToNfp uri
-        oldName <- getNameAtPos state nfp pos
-        refLocs <- refsAtName state nfp oldName
+        nfp <- handleUriToNfp uri
+        directOldNames <- getNamesAtPos state nfp pos
+        directRefs <- concat <$> mapM (refsAtName state nfp) directOldNames
+
+        {- References in HieDB are not necessarily transitive. With `NamedFieldPuns`, we can have
+           indirect references through punned names. To find the transitive closure, we do a pass of
+           the direct references to find the references for any punned names.
+           See the `IndirectPuns` test for an example. -}
+        indirectOldNames <- concat . filter ((>1) . Prelude.length) <$>
+            mapM (uncurry (getNamesAtPos state) . locToFilePos) directRefs
+        let oldNames = indirectOldNames ++ directOldNames
+        refs <- HS.fromList . concat <$> mapM (refsAtName state nfp) oldNames
+
+        -- Validate rename
         crossModuleEnabled <- lift $ usePropertyLsp #crossModule pluginId properties
-        unless crossModuleEnabled $ failWhenImportOrExport state nfp refLocs oldName
-        when (isBuiltInSyntax oldName) $
-            throwE ("Invalid rename of built-in syntax: \"" ++ showName oldName ++ "\"")
+        unless crossModuleEnabled $ failWhenImportOrExport state nfp refs oldNames
+        when (any isBuiltInSyntax oldNames) $ throwE "Invalid rename of built-in syntax"
+
+        -- Perform rename
         let newName = mkTcOcc $ T.unpack newNameText
-            filesRefs = collectWith locToUri refLocs
-            getFileEdit = flip $ getSrcEdit state . renameRefs newName
+            filesRefs = collectWith locToUri refs
+            getFileEdit = flip $ getSrcEdit state . replaceRefs newName
         fileEdits <- mapM (uncurry getFileEdit) filesRefs
         pure $ foldl' (<>) mempty fileEdits
 
@@ -84,16 +95,16 @@ failWhenImportOrExport ::
     IdeState ->
     NormalizedFilePath ->
     HashSet Location ->
-    Name ->
+    [Name] ->
     ExceptT String m ()
-failWhenImportOrExport state nfp refLocs name = do
+failWhenImportOrExport state nfp refLocs names = do
     pm <- handleMaybeM ("No parsed module for: " ++ show nfp) $ liftIO $ runAction
         "Rename.GetParsedModule"
         state
         (use GetParsedModule nfp)
     let hsMod = unLoc $ pm_parsed_source pm
     case (unLoc <$> hsmodName hsMod, hsmodExports hsMod) of
-        (mbModName, _) | not $ nameIsLocalOrFrom (replaceModName name mbModName) name
+        (mbModName, _) | not $ any (\n -> nameIsLocalOrFrom (replaceModName n mbModName) n) names
             -> throwE "Renaming of an imported name is unsupported"
         (_, Just (L _ exports)) | any ((`HS.member` refLocs) . unsafeSrcSpanToLoc . getLoc) exports
             -> throwE "Renaming of an exported name is unsupported"
@@ -112,7 +123,7 @@ getSrcEdit ::
     ExceptT String m WorkspaceEdit
 getSrcEdit state updatePs uri = do
     ccs <- lift getClientCapabilities
-    nfp <- safeUriToNfp uri
+    nfp <- handleUriToNfp uri
     annAst <- handleMaybeM ("No parsed source for: " ++ show nfp) $ liftIO $ runAction
         "Rename.GetAnnotatedParsedSource"
         state
@@ -128,13 +139,13 @@ getSrcEdit state updatePs uri = do
     pure $ diffText ccs (uri, src) res IncludeDeletions
 
 -- | Replace names at every given `Location` (in a given `ParsedSource`) with a given new name.
-renameRefs ::
+replaceRefs ::
     OccName ->
     HashSet Location ->
     ParsedSource ->
     ParsedSource
 #if MIN_VERSION_ghc(9,2,1)
-renameRefs newName refs = everywhere $
+replaceRefs newName refs = everywhere $
     -- there has to be a better way...
     mkT (replaceLoc @AnnListItem) `extT`
     -- replaceLoc @AnnList `extT` -- not needed
@@ -149,14 +160,13 @@ renameRefs newName refs = everywhere $
             | isRef (locA srcSpan) = L srcSpan $ replace oldRdrName
         replaceLoc lOldRdrName = lOldRdrName
 #else
-renameRefs newName refs = everywhere $ mkT replaceLoc
+replaceRefs newName refs = everywhere $ mkT replaceLoc
     where
         replaceLoc :: Located RdrName -> Located RdrName
         replaceLoc (L srcSpan oldRdrName)
             | isRef srcSpan = L srcSpan $ replace oldRdrName
         replaceLoc lOldRdrName = lOldRdrName
 #endif
-
         replace :: RdrName -> RdrName
         replace (Qual modName _) = Qual modName newName
         replace _                = Unqual newName
@@ -173,10 +183,10 @@ refsAtName ::
     IdeState ->
     NormalizedFilePath ->
     Name ->
-    ExceptT String m (HashSet Location)
+    ExceptT String m [Location]
 refsAtName state nfp name = do
     ShakeExtras{withHieDb} <- liftIO $ runAction "Rename.HieDb" state getShakeExtras
-    ast <- safeGetHieAst state nfp
+    ast <- handleGetHieAst state nfp
     dbRefs <- case nameModule_maybe name of
         Nothing -> pure []
         Just mod -> liftIO $ mapMaybe rowToLoc <$> withHieDb (\hieDb ->
@@ -188,32 +198,32 @@ refsAtName state nfp name = do
                 (Just $ moduleUnit mod)
                 [fromNormalizedFilePath nfp]
             )
-    pure $ HS.fromList $ getNameLocs name ast ++ dbRefs
+    pure $ nameLocs name ast ++ dbRefs
 
-getNameLocs :: Name -> (HieAstResult, PositionMapping) -> [Location]
-getNameLocs name (HAR _ _ rm _ _, pm) =
+nameLocs :: Name -> (HieAstResult, PositionMapping) -> [Location]
+nameLocs name (HAR _ _ rm _ _, pm) =
     mapMaybe (toCurrentLocation pm . realSrcSpanToLocation . fst)
              (concat $ M.lookup (Right name) rm)
 
 ---------------------------------------------------------------------------------------------------
 -- Util
 
-getNameAtPos :: IdeState -> NormalizedFilePath -> Position -> ExceptT String (LspT Config IO) Name
-getNameAtPos state nfp pos = do
-    (HAR{hieAst}, pm) <- safeGetHieAst state nfp
-    handleMaybe ("No name at " ++ showPos pos) $ listToMaybe $ getNamesAtPoint hieAst pos pm
+getNamesAtPos :: MonadIO m => IdeState -> NormalizedFilePath -> Position -> ExceptT String m [Name]
+getNamesAtPos state nfp pos = do
+    (HAR{hieAst}, pm) <- handleGetHieAst state nfp
+    pure $ getNamesAtPoint hieAst pos pm
 
-safeGetHieAst ::
+handleGetHieAst ::
     MonadIO m =>
     IdeState ->
     NormalizedFilePath ->
     ExceptT String m (HieAstResult, PositionMapping)
-safeGetHieAst state nfp = handleMaybeM
+handleGetHieAst state nfp = handleMaybeM
     ("No AST for file: " ++ show nfp)
     (liftIO $ runAction "Rename.GetHieAst" state $ useWithStale GetHieAst nfp)
 
-safeUriToNfp :: (Monad m) => Uri -> ExceptT String m NormalizedFilePath
-safeUriToNfp uri = handleMaybe
+handleUriToNfp :: (Monad m) => Uri -> ExceptT String m NormalizedFilePath
+handleUriToNfp uri = handleMaybe
     ("No filepath for uri: " ++ show uri)
     (toNormalizedFilePath <$> uriToFilePath uri)
 
@@ -230,15 +240,17 @@ nfpToUri = filePathToUri . fromNormalizedFilePath
 showName :: Name -> String
 showName = occNameString . getOccName
 
-showPos :: Position -> String
-showPos Position{_line, _character} = "line: " ++ show _line ++ " - character: " ++ show _character
-
 unsafeSrcSpanToLoc :: SrcSpan -> Location
 unsafeSrcSpanToLoc srcSpan =
     case srcSpanToLocation srcSpan of
         Nothing       -> error "Invalid conversion from UnhelpfulSpan to Location"
         Just location -> location
 
+locToFilePos :: Location -> (NormalizedFilePath, Position)
+locToFilePos (Location uri (Range pos _)) = (nfp, pos)
+    where
+        Just nfp = (uriToNormalizedFilePath . toNormalizedUri) uri
+
 replaceModName :: Name -> Maybe ModuleName -> Module
 replaceModName name mbModName =
     mkModule (moduleUnit $ nameModule name) (fromMaybe (mkModuleName "Main") mbModName)

plugins/hls-rename-plugin/test/Main.hs

@@ -15,12 +15,19 @@ main = defaultTestRunner tests
 renamePlugin :: PluginDescriptor IdeState
 renamePlugin = Rename.descriptor "rename"
 
+-- See https://github.com/wz1000/HieDb/issues/45
+recordConstructorIssue :: String
+recordConstructorIssue = "HIE references for record fields incorrect with GHC versions >= 9"
+
 tests :: TestTree
 tests = testGroup "Rename"
     [ goldenWithRename "Data constructor" "DataConstructor" $ \doc ->
         rename doc (Position 0 15) "Op"
     , goldenWithRename "Exported function" "ExportedFunction" $ \doc ->
         rename doc (Position 2 1) "quux"
+    , ignoreForGhcVersions [GHC90, GHC92] recordConstructorIssue $
+      goldenWithRename "Field Puns" "FieldPuns" $ \doc ->
+        rename doc (Position 7 13) "bleh"
     , goldenWithRename "Function argument" "FunctionArgument" $ \doc ->
         rename doc (Position 3 4) "y"
     , goldenWithRename "Function name" "FunctionName" $ \doc ->
@@ -33,6 +40,9 @@ tests = testGroup "Rename"
         rename doc (Position 3 8) "baz"
     , goldenWithRename "Import hiding" "ImportHiding" $ \doc ->
         rename doc (Position 0 22) "hiddenFoo"
+    , ignoreForGhcVersions [GHC90, GHC92] recordConstructorIssue $
+      goldenWithRename "Indirect Puns" "IndirectPuns" $ \doc ->
+        rename doc (Position 4 23) "blah"
     , goldenWithRename "Let expression" "LetExpression" $ \doc ->
         rename doc (Position 5 11) "foobar"
     , goldenWithRename "Qualified as" "QualifiedAs" $ \doc ->
@@ -43,7 +53,8 @@ tests = testGroup "Rename"
         rename doc (Position 3 12) "baz"
     , goldenWithRename "Realigns do block indentation" "RealignDo" $ \doc ->
         rename doc (Position 0 2) "fooBarQuux"
-    , goldenWithRename "Record field" "RecordField" $ \doc ->
+    , ignoreForGhcVersions [GHC90, GHC92] recordConstructorIssue $
+      goldenWithRename "Record field" "RecordField" $ \doc ->
         rename doc (Position 6 9) "number"
     , goldenWithRename "Shadowed name" "ShadowedName" $ \doc ->
         rename doc (Position 1 1) "baz"

plugins/hls-rename-plugin/test/testdata/FieldPuns.expected.hs

@@ -0,0 +1,8 @@
+{-# LANGUAGE NamedFieldPuns #-}
+
+module FieldPun () where
+
+newtype Foo = Foo { bleh :: Int }
+
+unFoo :: Foo -> Int
+unFoo Foo{bleh} = bleh

plugins/hls-rename-plugin/test/testdata/FieldPuns.hs

@@ -0,0 +1,8 @@
+{-# LANGUAGE NamedFieldPuns #-}
+
+module FieldPun () where
+
+newtype Foo = Foo { field :: Int }
+
+unFoo :: Foo -> Int
+unFoo Foo{field} = field

plugins/hls-rename-plugin/test/testdata/IndirectPuns.expected.hs

@@ -0,0 +1,8 @@
+{-# LANGUAGE NamedFieldPuns #-}
+
+module IndirectPuns () where
+
+newtype Foo = Foo { blah :: Int }
+
+unFoo :: Foo -> Int
+unFoo Foo{blah} = blah

plugins/hls-rename-plugin/test/testdata/IndirectPuns.hs

@@ -0,0 +1,8 @@
+{-# LANGUAGE NamedFieldPuns #-}
+
+module IndirectPuns () where
+
+newtype Foo = Foo { field :: Int }
+
+unFoo :: Foo -> Int
+unFoo Foo{field} = field

plugins/hls-rename-plugin/test/testdata/hie.yaml

@@ -3,13 +3,15 @@ cradle:
     arguments:
       - "DataConstructor"
       - "ExportedFunction"
+      - "FieldPuns"
       - "Foo"
       - "FunctionArgument"
       - "FunctionName"
       - "Gadt"
       - "HiddenFunction"
       - "ImportHiding"
       - "ImportedFunction"
+      - "IndirectPuns"
       - "LetExpression"
       - "QualifiedAs"
       - "QualifiedFunction"