Check types of missing definition in add import quick suggestions

[ilyakooo0]

Current behavior:

Even though the type of toLower is known from the context (Char -> Char), "quick fix" purposes to import functions that, despite having the same name, are known not to be correct type-wise (Data.Text.Lazy (toLower) for example).

Proposed behavior:

When the type of the undefined function is fully known, filter quick fixes by type.

[jneira]

This would be a really great improvement and i dare to say it should no be too difficult. Also, i think i read that suggestion in another issue but i cant find it out right now.

[pepeiborra]

There are two quick fixes involved here: suggest new import and suggest extend import. The code for these quick fixes is at:

haskell-language-server/ghcide/src/Development/IDE/Plugin/CodeAction.hs

Lines 796 to 805 in 0b3bb10

	suggestExtendImport :: ExportsMap -> ParsedSource -> Diagnostic -> [(T.Text, CodeActionKind, Rewrite)]
	suggestExtendImport exportsMap (L _ HsModule {hsmodImports}) Diagnostic{_range=_range,..}
	| Just [binding, mod, srcspan] <-
	matchRegexUnifySpaces _message
	"Perhaps you want to add ‘([^’]*)’ to the import list in the import of ‘([^’]*)’ *\\((.*)\\).$"
	= suggestions hsmodImports binding mod srcspan
	| Just (binding, mod_srcspan) <-
	matchRegExMultipleImports _message
	= mod_srcspan >>= uncurry (suggestions hsmodImports binding)
	| otherwise = []

and at

haskell-language-server/ghcide/src/Development/IDE/Plugin/CodeAction.hs

Lines 1241 to 1250 in 0b3bb10

	constructNewImportSuggestions
	:: ExportsMap -> (Maybe T.Text, NotInScope) -> Maybe [T.Text] -> [(CodeActionKind, NewImport)]
	constructNewImportSuggestions exportsMap (qual, thingMissing) notTheseModules = nubOrdOn snd
	[ suggestion
	| Just name <- [T.stripPrefix (maybe "" (<> ".") qual) $ notInScope thingMissing]
	, identInfo <- maybe [] Set.toList $ Map.lookup name (getExportsMap exportsMap)
	, canUseIdent thingMissing identInfo
	, moduleNameText identInfo `notElem` fromMaybe [] notTheseModules
	, suggestion <- renderNewImport identInfo
	]

In order to implement this, you will need to:

1. retrieve the type of the identifier I that is out of scope,
2. the type of the import candidate C
3. check if they are compatible

I'm not really sure what's the best way to retrieve I, possibly parsing the error message?

Retrieving C should be easy - just look through the tcg_type_env in the TcGblEnv.

To check if they are compatible, I would do this with tcUnifyTy.

[Javran]

The error message seems like a stable one to match against (data constructor has a slightly different message):
https://github.com/ghc/ghc/blob/4c87a3d1d14f9e28c8aa0f6062e9c4201f469ad7/compiler/GHC/Tc/Errors.hs#L1210-L1211
plus many GHC testsuite relies on this exact string.

I'll try with approach outlined in #754 (comment) and see how it goes.

[July541]

@pepeiborra Sorry to disturb you, I took a look at this and found the type of identifier can get from TcGblEnv by TcModuleResult, but candidates seem not. I have trouble connecting ExportsMap to TcModuleResult, do you have any suggestions?

[pepeiborra]

@July541 have you seen my recent comment in #2081 about this? It describes an easier approach, albeit too slow.

To answer your question: To retrieve the type of the candidate you would need to use the GHC api, similar to what ghci :info does. I'm not on my PC but you can easily find the code for :info by grepping for infoCmd in the ghc repository.

[jneira]

I think we could try the direct ghc api approach until typed holes are usable, if using the API does not introduce too much complexity

[July541]

To answer your question: To retrieve the type of the candidate you would need to use the GHC api, similar to what ghci :info does. I'm not on my PC but you can easily find the code for :info by grepping for infoCmd in the ghc repository.

Thank you, I'll try it.

@July541 have you seen my recent comment in #2081 about this? It describes an easier approach, albeit too slow.

Yes, I think we can move a little step at the beginning.

[isovector]

https://hackage.haskell.org/package/ghc-8.10.2/docs/GHC.html#v:lookupName might be helpful for getting the candidate types.

[July541]

Woohoo, thanks for your guide, it seems pretty nice!!!

[July541]

Do we have a function like OccName -> ModuleName -> Unit -> Name? I'm blocked by extracting type info from HieDb.

HieDb throws out a well-done result, restore the type seems not easy.

haskell-language-server/ghcide/src/Development/IDE/Types/Exports.hs

Lines 117 to 132 in dab8d00

	createExportsMapHieDb :: HieDb -> IO ExportsMap
	createExportsMapHieDb hiedb = do
	mods <- getAllIndexedMods hiedb
	idents <- forM (filter (nonInternalModules . modInfoName . hieModInfo) mods) $ \m -> do
	let mn = modInfoName $ hieModInfo m
	mText = pack $ moduleNameString mn
	fmap (wrap . unwrap mText) <$> getExportsForModule hiedb mn
	return $ ExportsMap $ Map.fromListWith (<>) (concat idents)
	where
	wrap identInfo = (rendered identInfo, Set.fromList [identInfo])
	-- unwrap :: ExportRow -> IdentInfo
	unwrap m ExportRow{..} = IdentInfo exportName n p exportIsDatacon m
	where
	n = pack (occNameString exportName)
	p = pack . occNameString <$> exportParent

https://github.com/wz1000/HieDb/blob/0e660aea0e2cf7a64c125b396b0229fcf679e6f7/src/HieDb/Types.hs#L221-L230

[jneira]

This continue being unimplememted and it is so annoying that i am tempted to label it as a bug

@July541 do you still planning continue working on that? do you have some work which could help other to continue this?

[jneira]

If at least we would have #2436 it would make sense to propose definitions with other types, as you could apply Change type signature in a second step. But there are contexts where adding the proposed definition supposes manual code changes for sure. Maybe for some use cases you still want the definition and change the expression afterwards. So not sure if we should filter them unconditionally. Maybe a config option would be needed.

but if we have several definitions with the same name and different types, they should be filtered unconditionally, imo

[July541]

As my latest comment mentioned, the difficult part is retrieve the type of the identifier I that is out of scope, as the identifier is from HieDB, restoring its type is not easy.

I'll have a look this weekend.

[July541]

Don't have a proper solution:(, feel free to pick this if anyone has ideas