Filter generated Core variable names from hovertip documentation

[drone29a]

This addresses #3280 by checking if names associated with a Haskell source file location follow particular prefix patterns used by generated Core variables. The two prefixes used are: $d and $c. If a name is prefixed with either of those strings then it is removed from the list of names used to generate the hovertip documentation content.

This change has been tested with the cases mentioned in the associated issue.

[santiweight]

If it's not too inconvenient: is it possible to have a test for this? Something with Generics is a great candidate, because those are the most problematic in my experience

[fendor]

Looks like a good start, would it be possible to add the two cases you mention in the associated issues as test-cases? Otherwise, regressions or changes in how the internal names look like will go through unchecked.

[fendor]

Would you mind adding a comment, what we consider to be an internal name?

[drone29a]

Will do. I had done this locally right before pushing, but forgot to commit.

[michaelpj]

Do we have any logic like this anywhere else in the code? Is there a more reliable way to identify a generated name? Have you asked in #ghc?

[drone29a]

Good thought, there is a list of GHC OccName prefixes in Development.IDE.Plugin.Completions.Logic.

[drone29a]

Re: tests, is there a good example test or test module I can use as a reference? It looks like there aren't existing tests for the AtPoint module. Should ghcide tests be placed within the top-level test/ directory or the ghcide/test/ directory?

I think we'll want to move the isInternal function into the Development.IDE.Plugin.Completions.Logic module, and perhaps give it a slightly better name. We could also extract the retrieval of names from the atPoint function, make it a top-level function (getNamesAtPoint?), and then test that directly rather than test the result of atPoint.

I'm new to HLS dev, so I'll proceed with whatever the experienced members recommend.

[pepeiborra]

Hey @drone-rites sorry for the late response. The ghcide test suite lives all in ghcide/test/exe/Main.hs and is almost 100% composed of integration tests, you will find almost no unit tests at all. The best place to test this change is:

haskell-language-server/ghcide/test/exe/Main.hs

Lines 996 to 1090 in 6dd5a3b

	findDefinitionAndHoverTests :: TestTree
	findDefinitionAndHoverTests = let
	tst :: (TextDocumentIdentifier -> Position -> Session a, a -> Session [Expect] -> Session ()) -> Position -> String -> Session [Expect] -> String -> TestTree
	tst (get, check) pos sfp targetRange title = testSessionWithExtraFiles "hover" title $ \dir -> do
	-- Dirty the cache to check that definitions work even in the presence of iface files
	liftIO $ runInDir dir $ do
	let fooPath = dir </> "Foo.hs"
	fooSource <- liftIO $ readFileUtf8 fooPath
	fooDoc <- createDoc fooPath "haskell" fooSource
	_ <- getHover fooDoc $ Position 4 3
	closeDoc fooDoc
	doc <- openTestDataDoc (dir </> sfp)
	waitForProgressDone
	found <- get doc pos
	check found targetRange
	checkHover :: Maybe Hover -> Session [Expect] -> Session ()
	checkHover hover expectations = traverse_ check =<< expectations where
	check expected =
	case hover of
	Nothing -> unless (expected == ExpectNoHover) $ liftIO $ assertFailure "no hover found"
	Just Hover{_contents = (HoverContents MarkupContent{_value = standardizeQuotes -> msg})
	,_range = rangeInHover } ->
	case expected of
	ExpectRange expectedRange -> checkHoverRange expectedRange rangeInHover msg
	ExpectHoverRange expectedRange -> checkHoverRange expectedRange rangeInHover msg
	ExpectHoverText snippets -> liftIO $ traverse_ (`assertFoundIn` msg) snippets
	ExpectHoverTextRegex re -> liftIO $ assertBool ("Regex not found in " <> T.unpack msg) (msg =~ re :: Bool)
	ExpectNoHover -> liftIO $ assertFailure $ "Expected no hover but got " <> show hover
	_ -> pure () -- all other expectations not relevant to hover
	_ -> liftIO $ assertFailure $ "test not expecting this kind of hover info" <> show hover
	extractLineColFromHoverMsg :: T.Text -> [T.Text]
	extractLineColFromHoverMsg = T.splitOn ":" . head . T.splitOn "*" . last . T.splitOn (sourceFileName <> ":")
	checkHoverRange :: Range -> Maybe Range -> T.Text -> Session ()
	checkHoverRange expectedRange rangeInHover msg =
	let
	lineCol = extractLineColFromHoverMsg msg
	-- looks like hovers use 1-based numbering while definitions use 0-based
	-- turns out that they are stored 1-based in RealSrcLoc by GHC itself.
	adjust Position{_line = l, _character = c} =
	Position{_line = l + 1, _character = c + 1}
	in
	case map (read . T.unpack) lineCol of
	[l,c] -> liftIO $ adjust (_start expectedRange) @=? Position l c
	_ -> liftIO $ assertFailure $
	"expected: " <> show ("[...]" <> sourceFileName <> ":<LINE>:<COL>**[...]", Just expectedRange) <>
	"\n but got: " <> show (msg, rangeInHover)
	assertFoundIn :: T.Text -> T.Text -> Assertion
	assertFoundIn part whole = assertBool
	(T.unpack $ "failed to find: `" <> part <> "` in hover message:\n" <> whole)
	(part `T.isInfixOf` whole)
	sourceFilePath = T.unpack sourceFileName
	sourceFileName = "GotoHover.hs"
	mkFindTests tests = testGroup "get"
	[ testGroup "definition" $ mapMaybe fst tests
	, testGroup "hover" $ mapMaybe snd tests
	, checkFileCompiles sourceFilePath $
	expectDiagnostics
	[ ( "GotoHover.hs", [(DsError, (62, 7), "Found hole: _")])
	, ( "GotoHover.hs", [(DsError, (65, 8), "Found hole: _")])
	]
	, testGroup "type-definition" typeDefinitionTests
	, testGroup "hover-record-dot-syntax" recordDotSyntaxTests ]
	typeDefinitionTests = [ tst (getTypeDefinitions, checkDefs) aaaL14 sourceFilePath (pure tcData) "Saturated data con"
	, tst (getTypeDefinitions, checkDefs) aL20 sourceFilePath (pure [ExpectNoDefinitions]) "Polymorphic variable"]
	recordDotSyntaxTests
	| ghcVersion >= GHC92 =
	[ tst (getHover, checkHover) (Position 19 24) (T.unpack "RecordDotSyntax.hs") (pure [ExpectHoverText ["x :: MyRecord"]]) "hover over parent"
	, tst (getHover, checkHover) (Position 19 25) (T.unpack "RecordDotSyntax.hs") (pure [ExpectHoverText ["_ :: MyChild"]]) "hover over dot shows child"
	, tst (getHover, checkHover) (Position 19 26) (T.unpack "RecordDotSyntax.hs") (pure [ExpectHoverText ["_ :: MyChild"]]) "hover over child"
	]
	| otherwise = []
	test runDef runHover look expect = testM runDef runHover look (return expect)
	testM runDef runHover look expect title =
	( runDef $ tst def look sourceFilePath expect title
	, runHover $ tst hover look sourceFilePath expect title ) where
	def = (getDefinitions, checkDefs)
	hover = (getHover , checkHover)
	-- search locations expectations on results

[drone29a]

Hi @pepeiborra, thanks for the pointer; perfect timing as I was just refactoring the updates today.

[drone29a]

Added a single, minimal test that captures the case I was originally observing. Type class dictionary variables associated with a type definition were showing up in the hover info when cursor was placed on a comment above the type definition.

[drone29a]

It's worth noting that reusing the occNamePrefixes list from:
https://github.com/drone-rites/haskell-language-server/blob/764f716e5390bdf9cb0fc602823171e8296285cb/ghcide/src/Development/IDE/GHC/CoreFile.hs#L234-L274
contain some entries that are redundant for the purpose of checking if a name is Core-generated. For example, the entries $contag and $co are redundant given the presence of $c. The additional longer prefixes are only needed for correctly stripping prefixes. I decided to keep it simple in the current version of the PR and there will be some redundant checks.

[pepeiborra]

This looks good to me, thank you for the test. I only have one question: can someone remind me why aren't we just filtering all identifiers starting with $, other than $ itself?

[fendor]

LGTM, amazing and necessary improvement :)

[drone29a]

This looks good to me, thank you for the test. I only have one question: can someone remind me why aren't we just filtering all identifiers starting with $, other than $ itself?

I started with a predicate like that locally, but then moved to an explicit short list of $d and $c after recalling there are other valid Haskell operators that match—e.g., $!, $$, and $-$. That short list was expanded once we discovered there was a list of Core variable prefixes already in ghcide.

An alternative predicate that should work is: the identifier has length of at least two characters, starts with "$", and contains an alphanumeric. Maybe there is a specification that describes the naming scheme of generated Core symbols and we could later update this check to follow it.

[michaelpj]

It seems that SrcSpans can be UnhelpfulSpans (https://hackage.haskell.org/package/ghc-9.4.2/docs/GHC-Types-SrcLoc.html#t:SrcSpan) which can indicate that they are generated. There's even a function for it: https://hackage.haskell.org/package/ghc-9.4.2/docs/GHC-Types-SrcLoc.html#v:isGeneratedSrcSpan

Could we replace the name-based logic with some logic that instead uses the SrcSpan of the Name?

[drone29a]

It seems that SrcSpans can be UnhelpfulSpans (https://hackage.haskell.org/package/ghc-9.4.2/docs/GHC-Types-SrcLoc.html#t:SrcSpan) which can indicate that they are generated. There's even a function for it: https://hackage.haskell.org/package/ghc-9.4.2/docs/GHC-Types-SrcLoc.html#v:isGeneratedSrcSpan

Could we replace the name-based logic with some logic that instead uses the SrcSpan of the Name?

Unfortunately, it doesn't look like access to UnhelpfulSpans are provided through an HieAST a. The nodeSpan field in an HieAST a is a Span type which is just an alias for RealSrcSpan. That said, the generated variables are showing up when the point is on a non-whitespace character of a comment, which seems like it would be covered by a RealSrcSpan. We could dive deeper and figure out why generated variables are being linked with comment spans.

[michaelpj]

That does seem odd indeed. A question for @wz1000 I guess.

[michaelpj]

Sorry, I thought this was merged! I think this was basically good to go, although I would appreciate it if @wz1000 could take a look and think about the issue with detecting generated code.

[wz1000]

Sorry, I didn't look at this earlier, but I'm not sure if looking at occname prefixes is the best way to go about this.

I think this line from #1983 might do what you need to out evidence bindings ($dFoo and $fFoo:

        prettyName (Right n, dets)
#if MIN_VERSION_ghc(9,0,1)
          | any isEvidenceUse (identInfo dets) = maybe "" (printOutputable . renderEvidenceTree) (getEvidenceTree _rf n) <> "\n"

https://github.com/haskell/haskell-language-server/pull/1983/files#diff-91f773bae93bbf86035573f4dbffa3035c1689b1bb7e17bde5937bb1271c0131R251

I'm not completely confident that it will remove all the generated names, but you could also have better luck filtering them out if you dropped the generated code like so:

haskell-language-server/plugins/hls-rename-plugin/src/Ide/Plugin/Rename.hs

Lines 234 to 244 in 2b6f603

	removeGenerated :: HieAstResult -> HieAstResult
	removeGenerated HAR{..} = HAR{hieAst = go hieAst,..}
	where
	go :: HieASTs a -> HieASTs a
	go hf =
	#if MIN_VERSION_ghc(9,2,1)
	HieASTs (fmap goAst (getAsts hf))
	goAst (Node nsi sp xs) = Node (SourcedNodeInfo $ M.restrictKeys (getSourcedNodeInfo nsi) (S.singleton SourceInfo)) sp (map goAst xs)
	#else
	hf
	#endif

Maybe these two fixes won't be enough and we would still need to filter out some names based on the prefix (like $sel in the presence of Duplicated record fields), so I don't know the best way forward.

[michaelpj]

@wz1000 what do you think about the idea of preserving the UnhelpfulGenerated SrcSpans through HIE files so we can pick them up again later?

[drone29a]

@wz1000 thanks, I was hoping there'd be a more direct approach. I will try out your suggestions.

[drone29a]

I tested updating prettyLine to use isEvidenceUse like so:

        prettyName (Right n, dets)
#if MIN_VERSION_ghc(9,0,1)
          | any isEvidenceUse (identInfo dets) = "wat...\n"
          | otherwise
#endif
           = T.unlines $
          wrapHaskell (printOutputable n <> maybe "" (" :: " <>) ((prettyType <$> identType dets) <|> maybeKind))
          : maybeToList (pretty (definedAt n) (prettyPackageName n))
          ++ catMaybes [ T.unlines . spanDocToMarkdown <$> lookupNameEnv dm n
                       ]
          where maybeKind = fmap printOutputable $ safeTyThingType =<< lookupNameEnv km n
                pretty Nothing Nothing = Nothing
                pretty (Just define) Nothing = Just $ define <> "\n"
                pretty Nothing (Just pkgName) = Just $ pkgName <> "\n"
                pretty (Just define) (Just pkgName) = Just $ define <> " " <> pkgName <> "\n"

It failed to replace the entries for the evidence bindings with the test string "wat...\n".

I also tried using removeGenerated which I modified to operate directly on HieASTs a and used like so:

atPoint IdeOptions{} (HAR _ hf _ _ kind) (DKMap dm km) env pos = listToMaybe $ pointCommand (removeGenerated hf) pos hoverInfo

It also failed to remove the entries.

@wz1000 any other suggestions I should try? Or did I misunderstand your earlier suggestions? Thanks!

[wz1000]

Ah, try isEvidenceContext which should catch more cases.

[drone29a]

That works, thanks! Sorry for the noise on the PR, somewhere along the lines I merged master into the branch instead of rebasing.

[michaelpj]

Lovely!

[michaelpj]

It doesn't seem to build however, it complains that isEvidenceContext is not in scope?

[wz1000]

Is this needed now?

[drone29a]

This is a refactor to allow the prefixes from the Logic module available to other plugins. Thanks to your suggestion, we now don't need to access to those prefixes from the AtPoint module but other modules might need access to the prefixes in the future. We can revert the refactor, or if it's considered an improvement we can keep it. I'll defer to you and others with more experience on the codebase.

[drone29a]

@michaelpj there was a missing preprocessor check. This commit should fix that: 30c42bb

[michaelpj]

Thanks!