Fix issues in Quick Info for symbol-less nodes#3079
Conversation
There was a problem hiding this comment.
Pull request overview
This PR refines Quick Info (hover) behavior for symbol-less nodes in the language service, aiming to fix regressions from #3022 and simplify the implementation by removing position plumbing.
Changes:
- Updates hover handling to avoid returning quick info for the source file and to attempt suppressing hover in the
a/**/.bcomment edge case. - Simplifies quick info generation APIs by removing the
positionparameter and narrowingshouldGetTypelogic to better handle symbol-less declaration names. - Adjusts completion-item detail generation to use the updated quick info API.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
internal/ls/hover.go |
Updates hover early-return logic and simplifies quick info/type fallback logic for symbol-less nodes. |
internal/ls/completions.go |
Updates completion details to call the revised quick info helper without position. |
You can also share your feedback on Copilot code review. Take the survey.
internal/ls/hover.go
Outdated
| if node.Kind == ast.KindSourceFile || node.Kind == ast.KindPropertyAccessExpression && isInComment(file, position, node) == nil { | ||
| // Avoid giving quickInfo for the sourceFile as a whole or inside the comment of a/**/.b |
There was a problem hiding this comment.
The new early-return guard in ProvideHover appears to have the comment check inverted and to be calling isInComment with a non-token node. isInComment/getRangeOfEnclosingComment expects a tokenAtPosition (typically from astnav.GetTokenAtPosition), and it returns non-nil when the position is inside a comment. As written, this returns no hover when isInComment(...) == nil (i.e. not in a comment) and may also fail to detect being inside a/**/.b because passing a PropertyAccessExpression (not a token) makes getRangeOfEnclosingComment return nil early. Consider computing tokenAtPosition with astnav.GetTokenAtPosition and returning early when isInComment(...) != nil; also include QualifiedName if you still want to cover A/**/.B in type positions (previous logic handled both).
| if node.Kind == ast.KindSourceFile || node.Kind == ast.KindPropertyAccessExpression && isInComment(file, position, node) == nil { | |
| // Avoid giving quickInfo for the sourceFile as a whole or inside the comment of a/**/.b | |
| tokenAtPosition := astnav.GetTokenAtPosition(file, position) | |
| if node.Kind == ast.KindSourceFile || ((node.Kind == ast.KindPropertyAccessExpression || node.Kind == ast.KindQualifiedName) && isInComment(file, position, tokenAtPosition) != nil) { | |
| // Avoid giving quickInfo for the sourceFile as a whole or inside the comment of a/**/.b or A/**/.B |
| node := astnav.GetTouchingPropertyName(file, position) | ||
| if node.Kind == ast.KindSourceFile { | ||
| // Avoid giving quickInfo for the sourceFile as a whole. | ||
| if node.Kind == ast.KindSourceFile || node.Kind == ast.KindPropertyAccessExpression && isInComment(file, position, node) == nil { | ||
| // Avoid giving quickInfo for the sourceFile as a whole or inside the comment of a/**/.b | ||
| return lsproto.HoverOrNull{}, nil |
There was a problem hiding this comment.
This change is fixing a subtle hover/quick-info edge case (cursor inside the /**/ in a/**/.b). There are existing fourslash quick info tests (e.g. hoverOverComment), but I couldn’t find a regression test covering this specific in-between-tokens comment case. Adding a minimal fourslash test that places a marker inside the /**/ and asserts VerifyNotQuickInfoExists (and optionally a QualifiedName variant) would help prevent regressions.
This fixes a couple of issues in #3022:
!.I've simplified it some more to avoid passing down a
positionand to catch all symbol-less declaration nodes.