From fb60821cd7f16dc9771e29d53b99efd168c55881 Mon Sep 17 00:00:00 2001 From: ktf Date: Fri, 9 Feb 2024 11:47:16 -0800 Subject: [PATCH 1/3] Use *only* incoming range to determine which code actions are in scope Rather than doing a full compare with incoming `Diagnostic` objects from the client. This brings the "remove redundant imports/exports" code actions more in line with behavior described in #4056, and has the pleasant side-effect of fixing broken code actions in neovim (#3857). --- .../src/Development/IDE/Plugin/CodeAction.hs | 31 ++++++++++++------- 1 file changed, 19 insertions(+), 12 deletions(-) diff --git a/plugins/hls-refactor-plugin/src/Development/IDE/Plugin/CodeAction.hs b/plugins/hls-refactor-plugin/src/Development/IDE/Plugin/CodeAction.hs index cd96758b39..d6fb1ae409 100644 --- a/plugins/hls-refactor-plugin/src/Development/IDE/Plugin/CodeAction.hs +++ b/plugins/hls-refactor-plugin/src/Development/IDE/Plugin/CodeAction.hs @@ -80,7 +80,8 @@ import GHC.Exts (fromList) import qualified GHC.LanguageExtensions as Lang import Ide.Logger hiding (group) -import Ide.PluginUtils (extractTextInRange, +import Ide.PluginUtils (extendToFullLines, + extractTextInRange, subRange) import Ide.Types import Language.LSP.Protocol.Message (Method (..), @@ -112,16 +113,16 @@ import Text.Regex.TDFA ((=~), (=~~)) -- | Generate code actions. codeAction :: PluginMethodHandler IdeState 'Method_TextDocumentCodeAction -codeAction state _ (CodeActionParams _ _ (TextDocumentIdentifier uri) _range CodeActionContext{_diagnostics= xs}) = do +codeAction state _ (CodeActionParams _ _ (TextDocumentIdentifier uri) range _) = do contents <- lift $ LSP.getVirtualFile $ toNormalizedUri uri liftIO $ do let text = virtualFileText <$> contents mbFile = toNormalizedFilePath' <$> uriToFilePath uri - diag <- atomically $ fmap (\(_, _, d) -> d) . filter (\(p, _, _) -> mbFile == Just p) <$> getDiagnostics state + allDiags <- atomically $ fmap (\(_, _, d) -> d) . filter (\(p, _, _) -> mbFile == Just p) <$> getDiagnostics state (join -> parsedModule) <- runAction "GhcideCodeActions.getParsedModule" state $ getParsedModule `traverse` mbFile let - actions = caRemoveRedundantImports parsedModule text diag xs uri - <> caRemoveInvalidExports parsedModule text diag xs uri + actions = caRemoveRedundantImports parsedModule text allDiags range uri + <> caRemoveInvalidExports parsedModule text allDiags range uri pure $ InL $ actions ------------------------------------------------------------------------------------------------- @@ -441,19 +442,25 @@ suggestRemoveRedundantImport ParsedModule{pm_parsed_source = L _ HsModule{hsmod = [("Remove import", [TextEdit (extendToWholeLineIfPossible contents _range) ""])] | otherwise = [] +diagInRange :: Diagnostic -> Range -> Bool +diagInRange Diagnostic {_range = dr} r = dr `subRange` extendedRange + where + -- Ensures the range captures full lines. Makes it easier to trigger the correct + -- "remove redundant" code actions from anywhere on the offending line. + extendedRange = extendToFullLines r -- Note [Removing imports is preferred] -- It's good to prefer the remove imports code action because an unused import -- is likely to be removed and less likely the warning will be disabled. -- Therefore actions to remove a single or all redundant imports should be -- preferred, so that the client can prioritize them higher. -caRemoveRedundantImports :: Maybe ParsedModule -> Maybe T.Text -> [Diagnostic] -> [Diagnostic] -> Uri -> [Command |? CodeAction] -caRemoveRedundantImports m contents digs ctxDigs uri +caRemoveRedundantImports :: Maybe ParsedModule -> Maybe T.Text -> [Diagnostic] -> Range -> Uri -> [Command |? CodeAction] +caRemoveRedundantImports m contents allDiags contextRange uri | Just pm <- m, - r <- join $ map (\d -> repeat d `zip` suggestRemoveRedundantImport pm contents d) digs, + r <- join $ map (\d -> repeat d `zip` suggestRemoveRedundantImport pm contents d) allDiags, allEdits <- [ e | (_, (_, edits)) <- r, e <- edits], caRemoveAll <- removeAll allEdits, - ctxEdits <- [ x | x@(d, _) <- r, d `elem` ctxDigs], + ctxEdits <- [ x | x@(d, _) <- r, d `diagInRange` contextRange], not $ null ctxEdits, caRemoveCtx <- map (\(d, (title, tedit)) -> removeSingle title tedit d) ctxEdits = caRemoveCtx ++ [caRemoveAll] @@ -477,8 +484,8 @@ caRemoveRedundantImports m contents digs ctxDigs uri _data_ = Nothing _changeAnnotations = Nothing -caRemoveInvalidExports :: Maybe ParsedModule -> Maybe T.Text -> [Diagnostic] -> [Diagnostic] -> Uri -> [Command |? CodeAction] -caRemoveInvalidExports m contents digs ctxDigs uri +caRemoveInvalidExports :: Maybe ParsedModule -> Maybe T.Text -> [Diagnostic] -> Range -> Uri -> [Command |? CodeAction] +caRemoveInvalidExports m contents digs contextRange uri | Just pm <- m, Just txt <- contents, txt' <- indexedByPosition $ T.unpack txt, @@ -488,7 +495,7 @@ caRemoveInvalidExports m contents digs ctxDigs uri allRanges <- nubOrd $ [ range | (_,_,ranges) <- r, range <- ranges], allRanges' <- extend txt' allRanges, Just caRemoveAll <- removeAll allRanges', - ctxEdits <- [ x | x@(_, d, _) <- r, d `elem` ctxDigs], + ctxEdits <- [ x | x@(_, d, _) <- r, d `diagInRange` contextRange], not $ null ctxEdits = caRemoveCtx ++ [caRemoveAll] | otherwise = [] From 4bd1e2dac443d5a159d5ddfd6dc241ae5ab27032 Mon Sep 17 00:00:00 2001 From: ktf Date: Fri, 9 Feb 2024 11:53:26 -0800 Subject: [PATCH 2/3] Remove redundant imports ;) --- .../src/Development/IDE/Plugin/CodeAction.hs | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/plugins/hls-refactor-plugin/src/Development/IDE/Plugin/CodeAction.hs b/plugins/hls-refactor-plugin/src/Development/IDE/Plugin/CodeAction.hs index d6fb1ae409..610ef3169c 100644 --- a/plugins/hls-refactor-plugin/src/Development/IDE/Plugin/CodeAction.hs +++ b/plugins/hls-refactor-plugin/src/Development/IDE/Plugin/CodeAction.hs @@ -40,7 +40,6 @@ import Data.Ord (comparing) import qualified Data.Set as S import qualified Data.Text as T import qualified Data.Text.Encoding as T -import qualified Data.Text.Utf16.Rope as Rope import Development.IDE.Core.Rules import Development.IDE.Core.RuleTypes import Development.IDE.Core.Service @@ -88,7 +87,6 @@ import Language.LSP.Protocol.Message (Method (..), SMethod (..)) import Language.LSP.Protocol.Types (ApplyWorkspaceEditParams (..), CodeAction (..), - CodeActionContext (CodeActionContext, _diagnostics), CodeActionKind (CodeActionKind_QuickFix), CodeActionParams (CodeActionParams), Command, @@ -103,8 +101,7 @@ import Language.LSP.Protocol.Types (ApplyWorkspa type (|?) (InL, InR), uriToFilePath) import qualified Language.LSP.Server as LSP -import Language.LSP.VFS (VirtualFile, - virtualFileText) +import Language.LSP.VFS (virtualFileText) import qualified Text.Fuzzy.Parallel as TFP import qualified Text.Regex.Applicative as RE import Text.Regex.TDFA ((=~), (=~~)) From ee7ba6e4b49be5b611c2d261d2add1a3f5ad6564 Mon Sep 17 00:00:00 2001 From: ktf Date: Mon, 12 Feb 2024 11:37:54 -0800 Subject: [PATCH 3/3] Rename param for clarity --- .../src/Development/IDE/Plugin/CodeAction.hs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/plugins/hls-refactor-plugin/src/Development/IDE/Plugin/CodeAction.hs b/plugins/hls-refactor-plugin/src/Development/IDE/Plugin/CodeAction.hs index 06a4b289b5..716246039d 100644 --- a/plugins/hls-refactor-plugin/src/Development/IDE/Plugin/CodeAction.hs +++ b/plugins/hls-refactor-plugin/src/Development/IDE/Plugin/CodeAction.hs @@ -481,11 +481,11 @@ caRemoveRedundantImports m contents allDiags contextRange uri _changeAnnotations = Nothing caRemoveInvalidExports :: Maybe ParsedModule -> Maybe T.Text -> [Diagnostic] -> Range -> Uri -> [Command |? CodeAction] -caRemoveInvalidExports m contents digs contextRange uri +caRemoveInvalidExports m contents allDiags contextRange uri | Just pm <- m, Just txt <- contents, txt' <- indexedByPosition $ T.unpack txt, - r <- mapMaybe (groupDiag pm) digs, + r <- mapMaybe (groupDiag pm) allDiags, r' <- map (\(t,d,rs) -> (t,d,extend txt' rs)) r, caRemoveCtx <- mapMaybe removeSingle r', allRanges <- nubOrd $ [ range | (_,_,ranges) <- r, range <- ranges],