fix(refactor): do not offer 'Add Argument' for qualified names (#4852)

Author: ijatinydv

plugins/hls-refactor-plugin/src/Development/IDE/Plugin/Plugins/AddArgument.hs

@@ -4,6 +4,7 @@ module Development.IDE.Plugin.Plugins.AddArgument (plugin) where
 import           Control.Monad                             (join)
 import           Control.Monad.Trans.Class                 (lift)
 import           Data.Bifunctor                            (Bifunctor (..))
+import           Data.Char                                 (isUpper)
 import           Data.Either.Extra                         (maybeToEither)
 import qualified Data.Text                                 as T
 import           Development.IDE.GHC.Compat
@@ -68,11 +69,20 @@ type HsArrow pass = HsMultAnn pass
 --      In this case a new argument would have to add its type between b and c in the signature.
 plugin :: ParsedModule -> Diagnostic -> Either PluginError [(T.Text, [TextEdit])]
 plugin parsedModule Diagnostic {_message, _range}
-  | Just (name, typ) <- matchVariableNotInScope message = addArgumentAction parsedModule _range name typ
+  | Just (name, typ) <- matchVariableNotInScope message
+  , not (isQualifiedName name) = addArgumentAction parsedModule _range name typ
   | Just (name, typ) <- matchFoundHoleIncludeUnderscore message = addArgumentAction parsedModule _range name (Just typ)
   | otherwise = pure []
   where
     message = unifySpaces _message
+    -- Qualified names (e.g. NE.toList) always start with an uppercase module
+    -- qualifier. Since "Variable not in scope" only reports variables and
+    -- operators, an unqualified name will never start with an uppercase letter.
+    -- Therefore, checking for an uppercase first character reliably identifies
+    -- qualified names, which can never be valid function argument patterns.
+    isQualifiedName name = case T.uncons name of
+      Just (c, _) -> isUpper c
+      Nothing     -> False
 
 -- Given a name for the new binding, add a new pattern to the match in the last position,
 -- returning how many patterns there were in this match prior to the transformation:

plugins/hls-refactor-plugin/test/Test/AddArgument.hs

@@ -3,10 +3,13 @@
 {-# LANGUAGE DataKinds             #-}
 {-# LANGUAGE DuplicateRecordFields #-}
 {-# LANGUAGE GADTs                 #-}
+{-# LANGUAGE LambdaCase            #-}
 {-# LANGUAGE OverloadedStrings     #-}
 
 module Test.AddArgument (tests) where
 
+import           Data.List                         (find)
+import           Data.Maybe                        (isJust)
 import qualified Data.Text                         as T
 import           Development.IDE.Types.Location
 import           Language.LSP.Protocol.Types       hiding
@@ -42,7 +45,8 @@ tests =
       mkGoldenAddArgTest "AddArgWithLambda" (r 1 0 1 50),
       mkGoldenAddArgTest "MultiSigFirst" (r 2 0 2 50),
       mkGoldenAddArgTest "MultiSigLast" (r 2 0 2 50),
-      mkGoldenAddArgTest "MultiSigMiddle" (r 2 0 2 50)
+      mkGoldenAddArgTest "MultiSigMiddle" (r 2 0 2 50),
+      mkNoAddArgForQualifiedNameTest
     ]
   where
     r x y x' y' = Range (Position x y) (Position x' y')
@@ -71,3 +75,30 @@ mkGoldenAddArgTest' testFileName range varName = do
       "expected"
       "hs"
       action
+
+-- | Verify that the "Add argument" code action is NOT offered for qualified names (e.g. NE.toList).
+-- We also verify that an import suggestion IS offered, to confirm we're testing the right diagnostic.
+mkNoAddArgForQualifiedNameTest :: TestTree
+mkNoAddArgForQualifiedNameTest =
+    testCase "No add argument for qualified names" $ runSessionWithServerInTmpDir def
+      (  mkPluginTestDescriptor Refactor.iePluginDescriptor "ghcide-code-actions-imports-exports"
+      <> mkPluginTestDescriptor Refactor.bindingsPluginDescriptor "ghcide-code-actions-bindings"
+      )
+      (FS.mkVirtualFileTree "plugins/hls-refactor-plugin/test/data/add-arg" (FS.directProject "QualifiedName.hs"))
+      $ do
+        doc <- openDoc "QualifiedName.hs" "haskell"
+        _ <- waitForDiagnostics
+        actions <- getCodeActions doc (Range (Position 5 0) (Position 5 50))
+        -- Verify that an import suggestion exists, confirming the right diagnostic.
+        let importAction = find (\case
+              InR CodeAction {_title = t} -> "Data.List.NonEmpty" `T.isInfixOf` t
+              _                           -> False) actions
+        liftIO $ assertBool
+          "Expected an import suggestion code action for Data.List.NonEmpty"
+          (isJust importAction)
+        -- Verify that the "Add argument" code action is NOT offered
+        let addArgAction = find (\case
+              InR CodeAction {_title = t} -> "Add argument" `T.isPrefixOf` t
+              _                           -> False) actions
+        liftIO $ addArgAction @?= Nothing
+

plugins/hls-refactor-plugin/test/data/add-arg/QualifiedName.hs

@@ -0,0 +1,6 @@
+module QualifiedName where
+
+import Data.List.NonEmpty (NonEmpty(..))
+
+foo :: NonEmpty a -> [a]
+foo xs = NE.toList xs