Improve handling of nonsense rename attempts

haskell-language-server.cabal

@@ -526,6 +526,9 @@ test-suite hls-rename-plugin-tests
     , hls-plugin-api
     , haskell-language-server:hls-rename-plugin
     , hls-test-utils             == 2.7.0.0
+    , lens
+    , lsp-types
+    , text
 
 -----------------------------
 -- retrie plugin

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

@@ -57,43 +57,49 @@ import           Language.LSP.Server
 instance Hashable (Mod a) where hash n = hash (unMod n)
 
 descriptor :: Recorder (WithPriority E.Log) -> PluginId -> PluginDescriptor IdeState
-descriptor recorder pluginId = mkExactprintPluginDescriptor recorder $ (defaultPluginDescriptor pluginId "Provides renaming of Haskell identifiers")
-    { pluginHandlers = mkPluginHandler SMethod_TextDocumentRename renameProvider
-    , pluginConfigDescriptor = defaultConfigDescriptor
-        { configCustomConfig = mkCustomConfig properties }
-    }
+descriptor recorder pluginId = mkExactprintPluginDescriptor recorder $
+    (defaultPluginDescriptor pluginId "Provides renaming of Haskell identifiers")
+        { pluginHandlers = mkPluginHandler SMethod_TextDocumentRename renameProvider
+        , pluginConfigDescriptor = defaultConfigDescriptor
+            { configCustomConfig = mkCustomConfig properties }
+        }
 
 renameProvider :: PluginMethodHandler IdeState Method_TextDocumentRename
 renameProvider state pluginId (RenameParams _prog (TextDocumentIdentifier uri) pos newNameText) = do
-        nfp <- getNormalizedFilePathE 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) . length) <$>
-            mapM (uncurry (getNamesAtPos state) <=< locToFilePos) directRefs
-        let oldNames = filter matchesDirect indirectOldNames ++ directOldNames
-            matchesDirect n = occNameFS (nameOccName n) `elem` directFS
-              where
-                directFS = map (occNameFS. nameOccName) directOldNames
-        refs <- HS.fromList . concat <$> mapM (refsAtName state nfp) oldNames
-
-        -- Validate rename
-        crossModuleEnabled <- liftIO $ runAction "rename: config" state $ usePropertyAction #crossModule pluginId properties
-        unless crossModuleEnabled $ failWhenImportOrExport state nfp refs oldNames
-        when (any isBuiltInSyntax oldNames) $ throwError $ PluginInternalError "Invalid rename of built-in syntax"
-
-        -- Perform rename
-        let newName = mkTcOcc $ T.unpack newNameText
-            filesRefs = collectWith locToUri refs
-            getFileEdit (uri, locations) = do
-              verTxtDocId <- lift $ getVersionedTextDoc (TextDocumentIdentifier uri)
-              getSrcEdit state verTxtDocId (replaceRefs newName locations)
-        fileEdits <- mapM getFileEdit filesRefs
-        pure $ InL $ fold fileEdits
+    nfp <- getNormalizedFilePathE 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) . length) <$>
+        mapM (uncurry (getNamesAtPos state) <=< locToFilePos) directRefs
+    let oldNames = filter matchesDirect indirectOldNames ++ directOldNames
+           where
+             matchesDirect n = occNameFS (nameOccName n) `elem` directFS
+             directFS = map (occNameFS . nameOccName) directOldNames
+
+    case oldNames of
+        -- There was no symbol at given position (e.g. rename triggered within a comment)
+        [] -> throwError $ PluginInvalidParams "No symbol to rename at given position"
+        _  -> do
+            refs <- HS.fromList . concat <$> mapM (refsAtName state nfp) oldNames
+
+            -- Validate rename
+            crossModuleEnabled <- liftIO $ runAction "rename: config" state $ usePropertyAction #crossModule pluginId properties
+            unless crossModuleEnabled $ failWhenImportOrExport state nfp refs oldNames
+            when (any isBuiltInSyntax oldNames) $ throwError $ PluginInternalError "Invalid rename of built-in syntax"
+
+            -- Perform rename
+            let newName = mkTcOcc $ T.unpack newNameText
+                filesRefs = collectWith locToUri refs
+                getFileEdit (uri, locations) = do
+                    verTxtDocId <- lift $ getVersionedTextDoc (TextDocumentIdentifier uri)
+                    getSrcEdit state verTxtDocId (replaceRefs newName locations)
+            fileEdits <- mapM getFileEdit filesRefs
+            pure $ InL $ fold fileEdits
 
 -- | Limit renaming across modules.
 failWhenImportOrExport ::

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

@@ -2,10 +2,13 @@
 
 module Main (main) where
 
+import           Control.Lens               ((^.))
 import           Data.Aeson
-import qualified Data.Map          as M
+import qualified Data.Map                   as M
+import           Data.Text                  (Text)
 import           Ide.Plugin.Config
-import qualified Ide.Plugin.Rename as Rename
+import qualified Ide.Plugin.Rename          as Rename
+import qualified Language.LSP.Protocol.Lens as L
 import           System.FilePath
 import           Test.Hls
 
@@ -64,11 +67,26 @@ tests = testGroup "Rename"
         rename doc (Position 2 17) "BinaryTree"
     , goldenWithRename "Type variable" "TypeVariable" $ \doc ->
         rename doc (Position 0 13) "b"
+    , goldenWithRename "Rename within comment" "Comment" $ \doc -> do
+        let expectedError = ResponseError
+                (InR ErrorCodes_InvalidParams)
+                "rename: Invalid Params: No symbol to rename at given position"
+                Nothing
+        renameExpectError expectedError doc (Position 0 10) "ImpossibleRename"
     ]
 
 goldenWithRename :: TestName-> FilePath -> (TextDocumentIdentifier -> Session ()) -> TestTree
 goldenWithRename title path act =
-    goldenWithHaskellDoc (def { plugins = M.fromList [("rename", def { plcConfig = "crossModule" .= True })] }) renamePlugin title testDataDir path "expected" "hs" act
+    goldenWithHaskellDoc (def { plugins = M.fromList [("rename", def { plcConfig = "crossModule" .= True })] })
+       renamePlugin title testDataDir path "expected" "hs" act
+
+renameExpectError :: ResponseError -> TextDocumentIdentifier -> Position -> Text -> Session ()
+renameExpectError expectedError doc pos newName = do
+  let params = RenameParams Nothing doc pos newName
+  rsp <- request SMethod_TextDocumentRename params
+  case rsp ^. L.result of
+    Right _ -> liftIO $ assertFailure $ "Was expecting " <> show expectedError <> ", got success"
+    Left actualError -> liftIO $ assertEqual "ResponseError" expectedError actualError
 
 testDataDir :: FilePath
 testDataDir = "plugins" </> "hls-rename-plugin" </> "test" </> "testdata"

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

@@ -0,0 +1 @@
+{- IShouldNotBeRenaemable -}

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

@@ -0,0 +1 @@
+{- IShouldNotBeRenaemable -}