Skip to content

Improve diagnosis of the missing expression in a for-each loop #1683

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 22, 2023
Merged

Improve diagnosis of the missing expression in a for-each loop #1683

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: 11 additions & 1 deletion Sources/SwiftParserDiagnostics/ParseDiagnosticsGenerator.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -778,7 +778,17 @@ public class ParseDiagnosticsGenerator: SyntaxAnyVisitor {
)
Copy link
Member Author

@TTOzzi TTOzzi May 21, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I also discovered issues with the C-style for loop, but it seemed difficult to resolve them simply 🤯
(testRecovery31?, testRecovery32, testRecovery33, testRecovery38)
It appears that they should be considered as separate issues.

Copy link
Member

Choose a reason for hiding this comment

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

Honestly, I just don’t care about the C-style for loop diagnostic. If you write C-style for loops in Swift. That diagnostic dates back in the old parser from the Swift 1 days when people were migrating from (Objective-)C to Swift and maybe didn’t know how to write Swift-style for loops and I only migrated it to the new parser as a proof-of-concept of a slightly more involved diagnostic.

} else { // If it's not a C-style for loop
if node.sequenceExpr.is(MissingExprSyntax.self) {
addDiagnostic(node.sequenceExpr, .expectedSequenceExpressionInForEachLoop, handledNodes: [node.sequenceExpr.id])
addDiagnostic(
node.sequenceExpr,
.expectedSequenceExpressionInForEachLoop,
fixIts: [
FixIt(
message: InsertTokenFixIt(missingNodes: [Syntax(node.sequenceExpr)]),
changes: [.makePresent(node.sequenceExpr)]
)
],
handledNodes: [node.sequenceExpr.id]
)
}
}

Expand Down
12 changes: 8 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 @@ -604,10 +604,10 @@ final class RecoveryTests: XCTestCase {
""",
diagnostics: [
DiagnosticSpec(locationMarker: "1️⃣", message: "keyword 'for' cannot be used as an identifier here", fixIts: ["if this name is unavoidable, use backticks to escape it"]),
DiagnosticSpec(locationMarker: "2️⃣", message: "expected Sequence expression for for-each loop"),
DiagnosticSpec(locationMarker: "2️⃣", message: "expected Sequence expression for for-each loop", fixIts: ["insert expression"]),
],
fixedSource: """
for `for` in {
for `for` in <#expression#> {
}
"""
)
Expand All @@ -620,8 +620,12 @@ final class RecoveryTests: XCTestCase {
}
""",
diagnostics: [
DiagnosticSpec(message: "expected Sequence expression for for-each loop")
]
DiagnosticSpec(message: "expected Sequence expression for for-each loop", fixIts: ["insert expression"])
],
fixedSource: """
for i in <#expression#> {
}
"""
)
}

Expand Down