Skip to content

Don't suggest import an unnecessary data constructor. #1984

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Jun 28, 2021
Merged

Don't suggest import an unnecessary data constructor. #1984

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
79dd2d9
Don't suggest import an unnecessary data constructor.
peterwicksstringfield Jun 18, 2021
2865e19
Merge branch 'master' into dont_import_extraneous_constructors_1932
mergify[bot] Jun 28, 2021
9ffd408
Merge branch 'master' into dont_import_extraneous_constructors_1932
mergify[bot] Jun 28, 2021
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
11 changes: 8 additions & 3 deletions ghcide/src/Development/IDE/Plugin/CodeAction.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -804,6 +804,10 @@ suggestExtendImport exportsMap (L _ HsModule {hsmodImports}) Diagnostic{_range=_
= mod_srcspan >>= uncurry (suggestions hsmodImports binding)
| otherwise = []
where
canUseDatacon = case extractNotInScopeName _message of
Just NotInScopeTypeConstructorOrClass{} -> False
_ -> True

suggestions decls binding mod srcspan
| range <- case [ x | (x,"") <- readSrcSpan (T.unpack srcspan)] of
[s] -> let x = realSrcSpanToRange s
Expand All @@ -823,7 +827,7 @@ suggestExtendImport exportsMap (L _ HsModule {hsmodImports}) Diagnostic{_range=_
-- Only for the situation that data constructor name is same as type constructor name,
-- let ident with parent be in front of the one without.
, sortedMatch <- sortBy (\ident1 ident2 -> parent ident2 `compare` parent ident1) (Set.toList match)
, idents <- filter (\ident -> moduleNameText ident == mod) sortedMatch
, idents <- filter (\ident -> moduleNameText ident == mod && (canUseDatacon || not (isDatacon ident))) sortedMatch
, (not . null) idents -- Ensure fallback while `idents` is empty
, ident <- head idents
= Just ident
Expand Down Expand Up @@ -1318,8 +1322,9 @@ hideImplicitPreludeSymbol :: T.Text -> NewImport
hideImplicitPreludeSymbol symbol = newUnqualImport "Prelude" symbol True

canUseIdent :: NotInScope -> IdentInfo -> Bool
canUseIdent NotInScopeDataConstructor{} = isDatacon
canUseIdent _ = const True
canUseIdent NotInScopeDataConstructor{} = isDatacon
canUseIdent NotInScopeTypeConstructorOrClass{} = not . isDatacon
canUseIdent _ = const True

data NotInScope
= NotInScopeDataConstructor T.Text
Expand Down
27 changes: 24 additions & 3 deletions ghcide/test/exe/Main.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -730,7 +730,7 @@ codeActionTests = testGroup "code actions"
, typeWildCardActionTests
, removeImportTests
, extendImportTests
, suggesImportClassMethodTests
, suggestImportClassMethodTests
, suggestImportTests
, suggestHideShadowTests
, suggestImportDisambiguationTests
Expand Down Expand Up @@ -1436,6 +1436,25 @@ extendImportTests = testGroup "extend import actions"
, "f :: Foo"
, "f = Foo 1"
])
, testSession "type constructor name same as data constructor name, data constructor extraneous" $ template
[("ModuleA.hs", T.unlines
[ "module ModuleA where"
, "data Foo = Foo"
])]
("ModuleB.hs", T.unlines
[ "module ModuleB where"
, "import ModuleA()"
, "f :: Foo"
, "f = undefined"
])
(Range (Position 2 4) (Position 2 6))
["Add Foo to the import list of ModuleA"]
(T.unlines
[ "module ModuleB where"
, "import ModuleA(Foo)"
, "f :: Foo"
, "f = undefined"
])
]
where
codeActionTitle CodeAction{_title=x} = x
Expand Down Expand Up @@ -1486,8 +1505,8 @@ extendImportTestsRegEx = testGroup "regex parsing"
template message expected = do
liftIO $ matchRegExMultipleImports message @=? expected

suggesImportClassMethodTests :: TestTree
suggesImportClassMethodTests =
suggestImportClassMethodTests :: TestTree
suggestImportClassMethodTests =
testGroup
"suggest import class methods"
[ testGroup
Expand Down Expand Up @@ -1566,6 +1585,8 @@ suggestImportTests = testGroup "suggest import actions"
, test False [] "f = quickCheck" [] "import Test.QuickCheck (quickCheck)"
-- don't omit the parent data type of a constructor
, test False [] "f ExitSuccess = ()" [] "import System.Exit (ExitSuccess)"
-- don't suggest data constructor when we only need the type
, test False [] "f :: Bar" [] "import Bar (Bar(Bar))"
]
, testGroup "want suggestion"
[ wantWait [] "f = foo" [] "import Foo (foo)"
Expand Down