Skip to content
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

Import suggestion for missing newtype constructor, all types constructor and indirect overloadedrecorddot fields #4516

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -1840,6 +1840,34 @@ extractNotInScopeName x
= Just $ NotInScopeDataConstructor name
| Just [name] <- matchRegexUnifySpaces x "ot in scope: type constructor or class [^‘]*‘([^’]*)’"
= Just $ NotInScopeTypeConstructorOrClass name
| Just [name] <- matchRegexUnifySpaces x "The data constructors of ‘([^ ]+)’ are not all in scope"
= Just $ NotInScopeDataConstructor name
| Just [name] <- matchRegexUnifySpaces x "of newtype ‘([^’]*)’ is not in scope"
= Just $ NotInScopeThing name
-- Match for HasField "foo" Bar String in the context where, e.g. x.foo is
-- used, and x :: Bar.
--
-- This usually mean that the field is not in scope and the correct fix is to
-- import (Bar(foo)) or (Bar(..)).
--
-- However, it is more reliable to match for the type name instead of the field
-- name, and most of the time you'll want to import the complete type with all
-- their fields instead of the specific field.
--
-- The regex is convoluted because it accounts for:
--
-- - Qualified (or not) `HasField`
-- - The type bar is always qualified. If it is unqualified, it means that the
-- parent module is already imported, and in this context it uses an hint
-- already available in the GHC error message. However this regex accounts for
-- qualified or not, it does not cost much and should be more robust if the
-- hint changes in the future
-- - Next regex will account for polymorphic types, which appears as `HasField
-- "foo" (Bar Int)...`, e.g. see the parenthesis
| Just [_module, name] <- matchRegexUnifySpaces x "No instance for [‘(].*HasField \"[^\"]+\" ([^ (.]+\\.)*([^ (.]+).*[’)]"
= Just $ NotInScopeThing name
| Just [_module, name] <- matchRegexUnifySpaces x "No instance for [‘(].*HasField \"[^\"]+\" \\(([^ .]+\\.)*([^ .]+)[^)]*\\).*[’)]"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems complex.
perhapes more unit test on these patterns. Thanks
I mean just plain unit test for matchRegexUnifySpaces x "No instance for [‘(].*HasField \"[^\"]+\" ([^ (.]+\\.)*([^ (.]+).*[’)]" and matchRegexUnifySpaces x "No instance for [‘(].*HasField \"[^\"]+\" \\(([^ .]+\\.)*([^ .]+)[^)]*\\).*[’)]"

= Just $ NotInScopeThing name
| Just [name] <- matchRegexUnifySpaces x "ot in scope: \\(([^‘ ]+)\\)"
= Just $ NotInScopeThing name
| Just [name] <- matchRegexUnifySpaces x "ot in scope: ([^‘ ]+)"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,9 @@ matchRegex message regex = case message =~~ regex of
Nothing -> Nothing

-- | 'matchRegex' combined with 'unifySpaces'
--
-- >>> matchRegexUnifySpaces "hello I'm a cow" "he(ll)o"
-- Just ["ll"]
matchRegexUnifySpaces :: T.Text -> T.Text -> Maybe [T.Text]
matchRegexUnifySpaces message = matchRegex (unifySpaces message)

Expand Down
87 changes: 86 additions & 1 deletion plugins/hls-refactor-plugin/test/Main.hs
Original file line number Diff line number Diff line change
Expand Up @@ -300,6 +300,8 @@ codeActionTests = testGroup "code actions"
, suggestImportClassMethodTests
, suggestImportTests
, suggestAddRecordFieldImportTests
, suggestAddCoerceMissingConstructorImportTests
, suggestAddGenericMissingConstructorImportTests
, suggestHideShadowTests
, fixConstructorImportTests
, fixModuleImportTypoTests
Expand Down Expand Up @@ -1849,8 +1851,14 @@ suggestImportTests = testGroup "suggest import actions"
suggestAddRecordFieldImportTests :: TestTree
suggestAddRecordFieldImportTests = testGroup "suggest imports of record fields when using OverloadedRecordDot"
[ testGroup "The field is suggested when an instance resolution failure occurs"
[ ignoreForGhcVersions [GHC94, GHC96] "Extension not present <9.2, and the assist is derived from the help message in >=9.4" theTest
([ ignoreForGhcVersions [GHC94, GHC96] "Extension not present <9.2, and the assist is derived from the help message in >=9.4" theTest
]
++ [
theTestIndirect qualifiedGhcRecords polymorphicType
|
qualifiedGhcRecords <- [False, True]
, polymorphicType <- [False, True]
])
]
where
theTest = testSessionWithExtraFiles "hover" def $ \dir -> do
Expand All @@ -1871,6 +1879,83 @@ suggestAddRecordFieldImportTests = testGroup "suggest imports of record fields w
contentAfterAction <- documentContents doc
liftIO $ after @=? contentAfterAction

theTestIndirect qualifiedGhcRecords polymorphicType = testGroup
((if qualifiedGhcRecords then "qualified-" else "unqualified-")
<> ("HasField " :: String)
<>
(if polymorphicType then "polymorphic-" else "monomorphic-")
<> "type ")
. (\x -> [x]) $ testSessionWithExtraFiles "hover" def $ \dir -> do
-- Hopefully enable project indexing?
configureCheckProject True

let
before = T.unlines ["{-# LANGUAGE OverloadedRecordDot #-}", "module A where", if qualifiedGhcRecords then "" else "import GHC.Records", "import C (bar)", "spam = bar.foo"]
after = T.unlines ["{-# LANGUAGE OverloadedRecordDot #-}", "module A where", if qualifiedGhcRecords then "" else "import GHC.Records", "import C (bar)", "import B (Foo(..))", "spam = bar.foo"]
cradle = "cradle: {direct: {arguments: [-hide-all-packages, -package, base, -package, text, -package-env, -, A, B, C]}}"
liftIO $ writeFileUTF8 (dir </> "hie.yaml") cradle
liftIO $ writeFileUTF8 (dir </> "B.hs") $ unlines ["module B where", if polymorphicType then "data Foo x = Foo { foo :: x }" else "data Foo = Foo { foo :: Int }"]
liftIO $ writeFileUTF8 (dir </> "C.hs") $ unlines ["module C where", "import B", "bar = Foo 10" ]
doc <- createDoc "Test.hs" "haskell" before
waitForProgressDone
_ <- waitForDiagnostics
let defLine = 4
range = Range (Position defLine 0) (Position defLine maxBound)
actions <- getCodeActions doc range
action <- pickActionWithTitle "import B (Foo(..))" actions
executeCodeAction action
contentAfterAction <- documentContents doc
liftIO $ after @=? contentAfterAction

suggestAddCoerceMissingConstructorImportTests :: TestTree
suggestAddCoerceMissingConstructorImportTests = testGroup "suggest imports of newtype constructor when using coerce"
[ testGroup "The newtype constructor is suggested when a matching representation error"
[ theTest
]
]
where
theTest = testSessionWithExtraFiles "hover" def $ \dir -> do
configureCheckProject False
let before = T.unlines ["module A where", "import Data.Coerce (coerce)", "import Data.Semigroup (Sum)", "bar = coerce (10 :: Int) :: Sum Int"]
after = T.unlines ["module A where", "import Data.Coerce (coerce)", "import Data.Semigroup (Sum)", "import Data.Semigroup (Sum(..))", "bar = coerce (10 :: Int) :: Sum Int"]
cradle = "cradle: {direct: {arguments: [-hide-all-packages, -package, base, -package, text, -package-env, -, A]}}"
liftIO $ writeFileUTF8 (dir </> "hie.yaml") cradle
doc <- createDoc "Test.hs" "haskell" before
waitForProgressDone
_ <- waitForDiagnostics
let defLine = 3
range = Range (Position defLine 0) (Position defLine maxBound)
actions <- getCodeActions doc range
action <- pickActionWithTitle "import Data.Semigroup (Sum(..))" actions
executeCodeAction action
contentAfterAction <- documentContents doc
liftIO $ after @=? contentAfterAction

suggestAddGenericMissingConstructorImportTests :: TestTree
suggestAddGenericMissingConstructorImportTests = testGroup "suggest imports of type constructors when using generic deriving"
[ testGroup "The type constructors are suggested when not in scope"
[ theTest
]
]
where
theTest = testSessionWithExtraFiles "hover" def $ \dir -> do
configureCheckProject False
let
before = T.unlines ["module A where", "import GHC.Generics", "import Data.Semigroup (Sum)", "deriving instance Generic (Sum Int)"]
after = T.unlines ["module A where", "import GHC.Generics", "import Data.Semigroup (Sum)", "import Data.Semigroup (Sum(..))", "deriving instance Generic (Sum Int)"]
cradle = "cradle: {direct: {arguments: [-hide-all-packages, -package, base, -package, text, -package-env, -, A]}}"
liftIO $ writeFileUTF8 (dir </> "hie.yaml") cradle
doc <- createDoc "Test.hs" "haskell" before
waitForProgressDone
_ <- waitForDiagnostics
let defLine = 3
range = Range (Position defLine 0) (Position defLine maxBound)
actions <- getCodeActions doc range
action <- pickActionWithTitle "import Data.Semigroup (Sum(..))" actions
executeCodeAction action
contentAfterAction <- documentContents doc
liftIO $ after @=? contentAfterAction


suggestImportDisambiguationTests :: TestTree
suggestImportDisambiguationTests = testGroup "suggest import disambiguation actions"
Expand Down
Loading