Skip to content

[5.9] Add diagnostic for wrong where requirements separation #1530

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
May 19, 2023
Merged

[5.9] Add diagnostic for wrong where requirements separation #1530

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
12 changes: 12 additions & 0 deletions Sources/SwiftParser/Declarations.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -759,9 +759,21 @@ extension Parser {
}

keepGoing = self.consume(if: .comma)
let unexpectedBetweenBodyAndTrailingComma: RawUnexpectedNodesSyntax?

// If there's a comma, keep parsing the list.
// If there's a "&&", diagnose replace with a comma and keep parsing
if let token = self.consumeIfContextualPunctuator("&&") {
keepGoing = self.missingToken(.comma)
unexpectedBetweenBodyAndTrailingComma = RawUnexpectedNodesSyntax([token], arena: self.arena)
} else {
unexpectedBetweenBodyAndTrailingComma = nil
}

elements.append(
RawGenericRequirementSyntax(
body: requirement,
unexpectedBetweenBodyAndTrailingComma,
trailingComma: keepGoing,
arena: self.arena
)
Expand Down
32 changes: 32 additions & 0 deletions Sources/SwiftParserDiagnostics/ParseDiagnosticsGenerator.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -546,6 +546,38 @@ public class ParseDiagnosticsGenerator: SyntaxAnyVisitor {
return handleEffectSpecifiers(node)
}

public override func visit(_ node: GenericRequirementSyntax) -> SyntaxVisitorContinueKind {
if shouldSkip(node) {
return .skipChildren
}

if let unexpected = node.unexpectedBetweenBodyAndTrailingComma,
let token = unexpected.tokens(satisfying: { $0.tokenKind == .binaryOperator("&&") }).first,
let trailingComma = node.trailingComma,
trailingComma.presence == .missing,
let previous = node.unexpectedBetweenBodyAndTrailingComma?.previousToken(viewMode: .sourceAccurate)
{

addDiagnostic(
unexpected,
.expectedCommaInWhereClause,
fixIts: [
FixIt(
message: ReplaceTokensFixIt(replaceTokens: [token], replacement: .commaToken()),
changes: [
.makeMissing(token),
.makePresent(trailingComma),
FixIt.MultiNodeChange(.replaceTrailingTrivia(token: previous, newTrivia: [])),
]
)
],
handledNodes: [unexpected.id, trailingComma.id]
)
}

return .visitChildren
}

public override func visit(_ node: DeinitializerDeclSyntax) -> SyntaxVisitorContinueKind {
if shouldSkip(node) {
return .skipChildren
Expand Down
3 changes: 3 additions & 0 deletions Sources/SwiftParserDiagnostics/ParserDiagnosticMessages.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,6 +131,9 @@ extension DiagnosticMessage where Self == StaticParserError {
public static var expectedAssignmentInsteadOfComparisonOperator: Self {
.init("expected '=' instead of '==' to assign default value for parameter")
}
public static var expectedCommaInWhereClause: Self {
.init("expected ',' to separate the requirements of this 'where' clause")
}
public static var expectedLeftBraceOrIfAfterElse: Self {
.init("expected '{' or 'if' after 'else'")
}
Expand Down
10 changes: 6 additions & 4 deletions Tests/SwiftParserTest/translated/RecoveryTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2174,15 +2174,17 @@ final class RecoveryTests: XCTestCase {
}

func testRecovery177() {
// rdar://38225184
Copy link
Contributor

Choose a reason for hiding this comment

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

This isn't needed any more (I'm fine with just fixing that on main FWIW)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just to be clear.
Should it be removed or? 😁

Copy link
Member

Choose a reason for hiding this comment

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

@bnbarham Just for clarification, this radar number has been carried over from the old parser tests because it was the motivation for the test case. I agree that it has very marginal value by now. I personally don’t have a strong opinion to keeping or removing it.

@kimdv just for your information: The contents of that radar are the same as swiftlang/swift#54086

Copy link
Contributor

Choose a reason for hiding this comment

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

Fair enough, personally I'd much prefer a comment on what it's testing rather than a radar number (or the GitHub issue). For something with tonnes of details, sure, link out to the issue. For this though... 🤷

Anyway, I know you didn't add it - it was mostly a drive by comment. No need to fix in this PR.

assertParse(
"""
// rdar://38225184
extension Collection where Element == Int 1️⃣&& Index == Int {}
""",
diagnostics: [
DiagnosticSpec(message: "unexpected code '&& Index == Int' in extension")
// TODO: Old parser expected error on line 1: expected ',' to separate the requirements of this 'where' clause, Fix-It replacements: 43 - 45 = ','
]
DiagnosticSpec(message: "expected ',' to separate the requirements of this 'where' clause", fixIts: ["replace '&&' with ','"])
],
fixedSource: """
extension Collection where Element == Int, Index == Int {}
"""
)
}

Expand Down