Skip to content

Exclude expected completions in a couple of places #273

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
shahmishal merged 2 commits into from
Nov 25, 2025
Merged

Exclude expected completions in a couple of places #273

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
939af6a
Ignore matching `file` and `line` argument labels
hamishknight Nov 24, 2025
1548bf0
Exclude expected completions for underscored attributes
hamishknight Nov 24, 2025
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
9 changes: 9 additions & 0 deletions SourceKitStressTester/Sources/StressTester/ActionGenerators.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -503,6 +503,15 @@ private struct ActionToken {
return nil
}
}

// Avoid matching for underscored attributes since we hide completions for
// those.
if let attr = parent.ancestorOrSelf(mapping: { $0.as(AttributeSyntax.self) }),
let ident = attr.attributeName.as(IdentifierTypeSyntax.self),
ident.name.text.hasPrefix("_") {
return nil
}

if let parent = parent.as(DeclReferenceExprSyntax.self), parent.baseName == token {
if let refArgs = parent.argumentNames {
let name = SwiftName(base: token.text, labels: refArgs.arguments.map{ $0.name.text })
Expand Down
30 changes: 19 additions & 11 deletions SourceKitStressTester/Sources/StressTester/SourceKitDocument.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -718,8 +718,22 @@ public struct CompletionMatcher {
private func matchesCall(paramLabels: [String]) -> Bool {
var remainingArgLabels = expected.name.argLabels[...]

func skippingIgnorableArgLabels(skipEmpty: Bool = false) -> ArraySlice<String> {
var remainingArgLabels = remainingArgLabels
if skipEmpty || remainingArgLabels.count < expected.name.argLabels.count {
// If previous param was matched, allow consuming unlabeled args to
// handle variadic cases.
remainingArgLabels = remainingArgLabels.drop { $0.isEmpty }
}
// Ignore `file` and `line` since we don't include `#file` and `#line`
// default args for completion.
// FIXME: We ought to have this be configurable and enable it for the stress
// tester.
Comment on lines +730 to +731
Copy link
Member

@ahoppen ahoppen Nov 24, 2025

Choose a reason for hiding this comment

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

Do you remember how the compiler skips these. IIRC it’s looking for #file or #line default argument values. Which unfortunately we can’t do here, so maybe this is fine.

Copy link
Contributor Author

@hamishknight hamishknight Nov 24, 2025

Choose a reason for hiding this comment

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

Yeah it's done here, IMO it would be nice to have a mode where it still includes them just for the stress tester since we otherwise can't handle them properly in the general case. #file and #line were the only ones that the new revision of swift-power-assert ran into though

Copy link
Member

@ahoppen ahoppen Nov 24, 2025

Choose a reason for hiding this comment

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

We could add a sourcekitd option for it, I wouldn’t have any objections to it.

return remainingArgLabels.drop { $0 == "file" || $0 == "line" }
}

guard !paramLabels.isEmpty else {
return remainingArgLabels.allSatisfy { $0.isEmpty }
return skippingIgnorableArgLabels(skipEmpty: true).isEmpty
}
for nextParamLabel in paramLabels {
if nextParamLabel.isEmpty {
Expand All @@ -732,12 +746,9 @@ public struct CompletionMatcher {
continue
}
} else {
// Has param label
if remainingArgLabels.count < expected.name.argLabels.count {
// A previous param was matched, so assume it was variadic and consume
// any leading unlabelled args so the next arg is labelled
remainingArgLabels = remainingArgLabels.drop{ $0.isEmpty }
}
// Has param label, skip any ignorable labels before matching.
remainingArgLabels = skippingIgnorableArgLabels()

guard let nextArgLabel = remainingArgLabels.first else {
// Assume any unprocessed parameters are defaulted
return true
Expand All @@ -750,9 +761,6 @@ public struct CompletionMatcher {
// Else assume this param was defaulted and skip it.
}
}
// If at least one arglabel was matched, allow for it being variadic
let hadMatch = remainingArgLabels.count < expected.name.argLabels.count
return remainingArgLabels.isEmpty || hadMatch &&
remainingArgLabels.allSatisfy { $0.isEmpty }
return skippingIgnorableArgLabels().isEmpty
}
}
18 changes: 16 additions & 2 deletions SourceKitStressTester/Tests/StressTesterToolTests/ActionGeneratorTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,11 @@ class ActionGeneratorTests: XCTestCase {

// FIXME: completion doesn't suggest anonymous closure params (e.g. $0)
(#line, "$0.first", [/*"$0",*/ "first"]),
(#line, "[1,2,4].filter{ $0 == 4 }", ["call:filter"/*, "$0"*/])
(#line, "[1,2,4].filter{ $0 == 4 }", ["call:filter"/*, "$0"*/]),

// We don't include completions for underscored attributes.
(#line, "@_underscored(a: Foo)", []),
(#line, "@_spi(Foo)", []),
]

for test in cases {
Expand Down Expand Up @@ -153,7 +157,17 @@ class ActionGeneratorTests: XCTestCase {
(match: .pattern, of: "first(_:z:)", against: "first(x:y:)", ignoreArgs: false, result: false),
(match: .pattern, of: "first(_:)", against: "first", ignoreArgs: false, result: false),
(match: .pattern, of: "first(_:)", against: "first()", ignoreArgs: false, result: true),
(match: .pattern, of: "first(_:_:)", against: "first()", ignoreArgs: false, result: true)
(match: .pattern, of: "first(_:_:)", against: "first()", ignoreArgs: false, result: true),

// We don't do matching for `file` and `line`.
(match: .call, of: "foo(x:file:line:)", against: "foo(x:)", ignoreArgs: false, result: true),
(match: .call, of: "foo(x:file:line:)", against: "foo(x:file:line:)", ignoreArgs: false, result: true),
(match: .call, of: "foo(x:file:line:y:)", against: "foo(x:y:)", ignoreArgs: false, result: true),
(match: .call, of: "foo(file:line:y:)", against: "foo(file:line:x:y:z:)", ignoreArgs: false, result: true),
(match: .call, of: "foo(x:file:line:y:)", against: "foo(x:y:z:)", ignoreArgs: false, result: true),
(match: .call, of: "foo(file:line:y:)", against: "foo(x:y:)", ignoreArgs: false, result: true),
(match: .call, of: "foo(file:line:)", against: "foo(_:)", ignoreArgs: false, result: true),
(match: .call, of: "foo(file:line:)", against: "foo()", ignoreArgs: false, result: true),
]

for test in cases {
Expand Down