Skip to content

Don’t offer text edits from inlay hints if they are syntactically invalid #770

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 1 commit into from
Jul 18, 2023
Merged

Don’t offer text edits from inlay hints if they are syntactically invalid #770

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
10 changes: 7 additions & 3 deletions Sources/SourceKitLSP/Swift/SwiftLanguageServer.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1320,13 +1320,17 @@ extension SwiftLanguageServer {
.map { info -> InlayHint in
let position = info.range.upperBound
let label = ": \(info.printedType)"
let textEdits: [TextEdit]?
if info.canBeFollowedByTypeAnnotation {
textEdits = [TextEdit(range: position..<position, newText: label)]
} else {
textEdits = nil
}
return InlayHint(
position: position,
label: .string(label),
kind: .type,
textEdits: [
TextEdit(range: position..<position, newText: label)
]
textEdits: textEdits
)
}

Expand Down
34 changes: 34 additions & 0 deletions Sources/SourceKitLSP/Swift/VariableTypeInfo.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,35 @@
import Dispatch
import LanguageServerProtocol
import SourceKitD
import SwiftSyntax

fileprivate extension TokenSyntax {
/// Returns `false` if it is known that this token can’t be followed by a type
/// annotation.
var canBeFollowedByTypeAnnotation: Bool {
var node = Syntax(self)
LOOP: while let parent = node.parent {
switch parent.kind {
case .caseItem, .closureParam:
Copy link
Contributor

Choose a reason for hiding this comment

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

I was going to ask if this handles the case that the param is in (). But apparently it does, because closureParam is only the non-signature case. x in is a ClosureParamListSyntax where as (x) in is a ClosureParameterClauseSyntax. The former has ClosureParams and the latter ClosureParameters 😅.

Not something that needs fixing here, but worth knowing. I don't have a great name for the former case 🤔.

// case items (inside a switch) and closure parameters can’t have type
// annotations.
return false
case .codeBlockItem, .memberDeclListItem:
// Performance optimization. If we walked the parents up to code block item,
// we can’t enter a case item or closure param anymore. No need walking
// the tree any further.
break LOOP
default:
break
}
node = parent
}

// By default, assume that the token can be followed by a type annotation as
// most locations that produce a variable type info can.
return true
}
}

/// A typed variable as returned by sourcekitd's CollectVariableType.
struct VariableTypeInfo {
Expand All @@ -22,6 +51,9 @@ struct VariableTypeInfo {
var printedType: String
/// Whether the variable has an explicit type annotation in the source file.
var hasExplicitType: Bool
/// Whether we should suggest making an edit to add the type annotation to the
/// source file.
var canBeFollowedByTypeAnnotation: Bool

init?(_ dict: SKDResponseDictionary, in snapshot: DocumentSnapshot) {
let keys = dict.sourcekitd.keys
Expand All @@ -34,10 +66,12 @@ struct VariableTypeInfo {
let hasExplicitType: Bool = dict[keys.variable_type_explicit] else {
return nil
}
let tokenAtOffset = snapshot.tokens.syntaxTree?.token(at: AbsolutePosition(utf8Offset: offset))

self.range = startIndex..<endIndex
self.printedType = printedType
self.hasExplicitType = hasExplicitType
self.canBeFollowedByTypeAnnotation = tokenAtOffset?.canBeFollowedByTypeAnnotation ?? true
Copy link
Contributor

Choose a reason for hiding this comment

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

Would a false default be better here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not really. canBeFollowedByTypeAnnotation is a blacklist-based method that also defaults to true. That’s why I think we should also default to true here for consistency.

}
}

Expand Down
17 changes: 11 additions & 6 deletions Tests/SourceKitLSPTests/InlayHintTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,14 +61,18 @@ final class InlayHintTests: XCTestCase {
}
}

private func makeInlayHint(position: Position, kind: InlayHintKind, label: String) -> InlayHint {
InlayHint(
private func makeInlayHint(position: Position, kind: InlayHintKind, label: String, hasEdit: Bool = true) -> InlayHint {
let textEdits: [TextEdit]?
if hasEdit {
textEdits = [TextEdit(range: position..<position, newText: label)]
} else {
textEdits = nil
}
return InlayHint(
position: position,
label: .string(label),
kind: kind,
textEdits: [
TextEdit(range: position..<position, newText: label)
]
textEdits: textEdits
)
}

Expand Down Expand Up @@ -205,7 +209,8 @@ final class InlayHintTests: XCTestCase {
makeInlayHint(
position: Position(line: 3, utf16index: 31),
kind: .type,
label: ": String"
label: ": String",
hasEdit: false
),
makeInlayHint(
position: Position(line: 4, utf16index: 40),
Expand Down