Skip to content

Commit 9a2da83

Browse files
kimdvkimdv
committed
Add missing comma diagnostic if it's inside an array or dictionary
1 parent 9f09285 commit 9a2da83

File tree

4 files changed

+152
-55
lines changed

4 files changed

+152
-55
lines changed

‎Sources/SwiftParser/Expressions.swift‎

Lines changed: 26 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -1497,17 +1497,38 @@ extension Parser {
14971497
var elements = [RawSyntax]()
14981498
do {
14991499
var collectionProgress = LoopProgressCondition()
1500-
COLLECTION_LOOP: while self.hasProgressed(&collectionProgress) {
1500+
var keepGoing: RawTokenSyntax?
1501+
COLLECTION_LOOP: repeat {
15011502
elementKind = self.parseCollectionElement(elementKind)
15021503

1504+
/// Whether expression of an array element or the value of a dictionary
1505+
/// element is missing. If this is the case, we shouldn't recover from
1506+
/// a missing comma since most likely the closing `]` is missing.
1507+
var elementIsMissingExpression: Bool {
1508+
switch elementKind! {
1509+
case .dictionary(_, _, _, let value):
1510+
return value.is(RawMissingExprSyntax.self)
1511+
case .array(let rawExprSyntax):
1512+
return rawExprSyntax.is(RawMissingExprSyntax.self)
1513+
}
1514+
}
1515+
15031516
// Parse the ',' if exists.
1504-
let comma = self.consume(if: .comma)
1517+
if let token = self.consume(if: .comma) {
1518+
keepGoing = token
1519+
} else if !self.at(.rightSquare, .endOfFile) && !self.atStartOfLine && !elementIsMissingExpression && !self.atStartOfDeclaration()
1520+
&& !self.atStartOfStatement(preferExpr: false)
1521+
{
1522+
keepGoing = missingToken(.comma)
1523+
} else {
1524+
keepGoing = nil
1525+
}
15051526

15061527
switch elementKind! {
15071528
case .array(let el):
15081529
let element = RawArrayElementSyntax(
15091530
expression: el,
1510-
trailingComma: comma,
1531+
trailingComma: keepGoing,
15111532
arena: self.arena
15121533
)
15131534
if element.isEmpty {
@@ -1521,7 +1542,7 @@ extension Parser {
15211542
unexpectedBeforeColon,
15221543
colon: colon,
15231544
value: value,
1524-
trailingComma: comma,
1545+
trailingComma: keepGoing,
15251546
arena: self.arena
15261547
)
15271548
if element.isEmpty {
@@ -1530,29 +1551,7 @@ extension Parser {
15301551
elements.append(RawSyntax(element))
15311552
}
15321553
}
1533-
1534-
// If we saw a comma, that's a strong indicator we have more elements
1535-
// to process. If that's not the case, we have to do some legwork to
1536-
// determine if we should bail out.
1537-
guard comma == nil || self.at(.rightSquare, .endOfFile) else {
1538-
continue
1539-
}
1540-
1541-
// If we found EOF or the closing square bracket, bailout.
1542-
if self.at(.rightSquare, .endOfFile) {
1543-
break
1544-
}
1545-
1546-
// If the next token is at the beginning of a new line and can never start
1547-
// an element, break.
1548-
if self.atStartOfLine
1549-
&& (self.at(.rightBrace, .poundEndif)
1550-
|| self.atStartOfDeclaration()
1551-
|| self.atStartOfStatement(preferExpr: false))
1552-
{
1553-
break
1554-
}
1555-
}
1554+
} while keepGoing != nil && self.hasProgressed(&collectionProgress)
15561555
}
15571556

15581557
let (unexpectedBeforeRSquare, rsquare) = self.expect(.rightSquare)

‎Tests/SwiftParserTest/ExpressionTests.swift‎

Lines changed: 93 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -2762,11 +2762,15 @@ final class StatementExpressionTests: ParserTestCase {
27622762
experimentalFeatures: .typedThrows
27632763
)
27642764
assertParse(
2765-
"[() try(MyError) async -> Void]()",
2765+
"[() 1️⃣try(MyError) async -> Void]()",
2766+
diagnostics: [DiagnosticSpec(message: "expected ',' in array element", fixIts: ["insert ','"])],
2767+
fixedSource: "[(), try(MyError) async -> Void]()",
27662768
experimentalFeatures: .typedThrows
27672769
)
27682770
assertParse(
2769-
"[() try async(MyError) -> Void]()",
2771+
"[() 1️⃣try async(MyError) -> Void]()",
2772+
diagnostics: [DiagnosticSpec(message: "expected ',' in array element", fixIts: ["insert ','"])],
2773+
fixedSource: "[(), try async(MyError) -> Void]()",
27702774
experimentalFeatures: .typedThrows
27712775
)
27722776
assertParse(
@@ -2787,30 +2791,49 @@ final class StatementExpressionTests: ParserTestCase {
27872791
experimentalFeatures: .typedThrows
27882792
)
27892793
assertParse(
2790-
"[() try(MyError) await 1️⃣-> Void]()",
2794+
"[() 1️⃣try(MyError) 2️⃣await 3️⃣-> Void]()",
27912795
diagnostics: [
27922796
DiagnosticSpec(
2797+
locationMarker: "1️⃣",
2798+
message: "expected ',' in array element",
2799+
fixIts: ["insert ','"]
2800+
),
2801+
DiagnosticSpec(
2802+
locationMarker: "2️⃣",
2803+
message: "expected ',' in array element",
2804+
fixIts: ["insert ','"]
2805+
),
2806+
DiagnosticSpec(
2807+
locationMarker: "3️⃣",
27932808
message: "expected expression in 'await' expression",
27942809
fixIts: ["insert expression"]
2795-
)
2810+
),
27962811
],
2797-
fixedSource: "[() try(MyError) await <#expression#> -> Void]()",
2812+
fixedSource: "[(), try(MyError), await <#expression#> -> Void]()",
27982813
experimentalFeatures: .typedThrows
27992814
)
28002815
assertParse(
2801-
"[() try await(MyError) -> Void]()",
2816+
"[() 1️⃣try await(MyError) -> Void]()",
2817+
diagnostics: [DiagnosticSpec(message: "expected ',' in array element", fixIts: ["insert ','"])],
2818+
fixedSource: "[(), try await(MyError) -> Void]()",
28022819
experimentalFeatures: .typedThrows
28032820
)
28042821
assertParse(
2805-
"[() async(MyError) -> Void]()",
2822+
"[() 1️⃣async(MyError) -> Void]()",
2823+
diagnostics: [DiagnosticSpec(message: "expected ',' in array element", fixIts: ["insert ','"])],
2824+
fixedSource: "[(), async(MyError) -> Void]()",
28062825
experimentalFeatures: .typedThrows
28072826
)
28082827
assertParse(
2809-
"[() await(MyError) -> Void]()",
2828+
"[() 1️⃣await(MyError) -> Void]()",
2829+
diagnostics: [DiagnosticSpec(message: "expected ',' in array element", fixIts: ["insert ','"])],
2830+
fixedSource: "[(), await(MyError) -> Void]()",
28102831
experimentalFeatures: .typedThrows
28112832
)
28122833
assertParse(
2813-
"[() try(MyError) -> Void]()",
2834+
"[() 1️⃣try(MyError) -> Void]()",
2835+
diagnostics: [DiagnosticSpec(message: "expected ',' in array element", fixIts: ["insert ','"])],
2836+
fixedSource: "[(), try(MyError) -> Void]()",
28142837
experimentalFeatures: .typedThrows
28152838
)
28162839
assertParse(
@@ -2826,4 +2849,65 @@ final class StatementExpressionTests: ParserTestCase {
28262849
experimentalFeatures: .typedThrows
28272850
)
28282851
}
2852+
2853+
func testArrayExprWithNoCommas() {
2854+
assertParse("[() ()]")
2855+
2856+
assertParse(
2857+
"[1 1️⃣2]",
2858+
diagnostics: [
2859+
DiagnosticSpec(
2860+
message: "expected ',' in array element",
2861+
fixIts: ["insert ','"]
2862+
)
2863+
],
2864+
fixedSource: "[1, 2]"
2865+
)
2866+
2867+
assertParse(
2868+
#"["hello" 1️⃣"world"]"#,
2869+
diagnostics: [
2870+
DiagnosticSpec(
2871+
message: "expected ',' in array element",
2872+
fixIts: ["insert ','"]
2873+
)
2874+
],
2875+
fixedSource: #"["hello", "world"]"#
2876+
)
2877+
}
2878+
2879+
func testDictionaryExprWithNoCommas() {
2880+
assertParse(
2881+
"[1: () 1️⃣2: ()]",
2882+
diagnostics: [
2883+
DiagnosticSpec(
2884+
message: "expected ',' in dictionary element",
2885+
fixIts: ["insert ','"]
2886+
)
2887+
],
2888+
fixedSource: #"[1: (), 2: ()]"#
2889+
)
2890+
2891+
assertParse(
2892+
#"["foo": 1 1️⃣"bar": 2]"#,
2893+
diagnostics: [
2894+
DiagnosticSpec(
2895+
message: "expected ',' in dictionary element",
2896+
fixIts: ["insert ','"]
2897+
)
2898+
],
2899+
fixedSource: #"["foo": 1, "bar": 2]"#
2900+
)
2901+
2902+
assertParse(
2903+
#"[1: "hello" 1️⃣2: "world"]"#,
2904+
diagnostics: [
2905+
DiagnosticSpec(
2906+
message: "expected ',' in dictionary element",
2907+
fixIts: ["insert ','"]
2908+
)
2909+
],
2910+
fixedSource: #"[1: "hello", 2: "world"]"#
2911+
)
2912+
}
28292913
}

‎Tests/SwiftParserTest/translated/ObjectLiteralsTests.swift‎

Lines changed: 27 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -18,41 +18,44 @@ final class ObjectLiteralsTests: ParserTestCase {
1818
func testObjectLiterals1a() {
1919
assertParse(
2020
"""
21-
let _ = [#Color(colorLiteralRed: red, green: green, blue: blue, alpha: alpha)#1️⃣]
21+
let _ = [#Color(colorLiteralRed: red, green: green, blue: blue, alpha: alpha)1️⃣#2️⃣]
2222
""",
2323
diagnostics: [
24-
DiagnosticSpec(locationMarker: "1️⃣", message: "expected identifier in macro expansion", fixIts: ["insert identifier"])
24+
DiagnosticSpec(locationMarker: "1️⃣", message: "expected ',' in array element", fixIts: ["insert ','"]),
25+
DiagnosticSpec(locationMarker: "2️⃣", message: "expected identifier in macro expansion", fixIts: ["insert identifier"]),
2526
],
2627
fixedSource: """
27-
let _ = [#Color(colorLiteralRed: red, green: green, blue: blue, alpha: alpha)#<#identifier#>]
28+
let _ = [#Color(colorLiteralRed: red, green: green, blue: blue, alpha: alpha), #<#identifier#>]
2829
"""
2930
)
3031
}
3132

3233
func testObjectLiterals1b() {
3334
assertParse(
3435
"""
35-
let _ = [#Image(imageLiteral: localResourceNameAsString)#1️⃣]
36+
let _ = [#Image(imageLiteral: localResourceNameAsString)1️⃣#2️⃣]
3637
""",
3738
diagnostics: [
38-
DiagnosticSpec(locationMarker: "1️⃣", message: "expected identifier in macro expansion", fixIts: ["insert identifier"])
39+
DiagnosticSpec(locationMarker: "1️⃣", message: "expected ',' in array element", fixIts: ["insert ','"]),
40+
DiagnosticSpec(locationMarker: "2️⃣", message: "expected identifier in macro expansion", fixIts: ["insert identifier"]),
3941
],
4042
fixedSource: """
41-
let _ = [#Image(imageLiteral: localResourceNameAsString)#<#identifier#>]
43+
let _ = [#Image(imageLiteral: localResourceNameAsString), #<#identifier#>]
4244
"""
4345
)
4446
}
4547

4648
func testObjectLiterals1c() {
4749
assertParse(
4850
"""
49-
let _ = [#FileReference(fileReferenceLiteral: localResourceNameAsString)#1️⃣]
51+
let _ = [#FileReference(fileReferenceLiteral: localResourceNameAsString)1️⃣#2️⃣]
5052
""",
5153
diagnostics: [
52-
DiagnosticSpec(locationMarker: "1️⃣", message: "expected identifier in macro expansion", fixIts: ["insert identifier"])
54+
DiagnosticSpec(locationMarker: "1️⃣", message: "expected ',' in array element", fixIts: ["insert ','"]),
55+
DiagnosticSpec(locationMarker: "2️⃣", message: "expected identifier in macro expansion", fixIts: ["insert identifier"]),
5356
],
5457
fixedSource: """
55-
let _ = [#FileReference(fileReferenceLiteral: localResourceNameAsString)#<#identifier#>]
58+
let _ = [#FileReference(fileReferenceLiteral: localResourceNameAsString), #<#identifier#>]
5659
"""
5760
)
5861
}
@@ -112,22 +115,22 @@ final class ObjectLiteralsTests: ParserTestCase {
112115
""",
113116
diagnostics: [
114117
DiagnosticSpec(locationMarker: "1️⃣", message: "expected identifier in macro expansion", fixIts: ["insert identifier"]),
118+
DiagnosticSpec(locationMarker: "1️⃣", message: "expected ',' in array element", fixIts: ["insert ','"]),
115119
DiagnosticSpec(locationMarker: "2️⃣", message: "expected identifier in macro expansion", fixIts: ["insert identifier"]),
116120
],
117121
fixedSource: """
118-
let _ = [#<#identifier#> #<#identifier#>]
122+
let _ = [#<#identifier#>, #<#identifier#>]
119123
"""
120124
)
121125
}
122126

123127
func testObjectLiterals5() {
124128
assertParse(
125129
"""
126-
let _ = ℹ️[#Color(_: 1, green: 1, 2)2️⃣
130+
let _ = ℹ️[#Color(_: 1, green: 1, 2)1️⃣
127131
""",
128132
diagnostics: [
129133
DiagnosticSpec(
130-
locationMarker: "2️⃣",
131134
message: "expected ']' to end array",
132135
notes: [NoteSpec(message: "to match this opening '['")],
133136
fixIts: ["insert ']'"]
@@ -142,37 +145,43 @@ final class ObjectLiteralsTests: ParserTestCase {
142145
func testObjectLiterals6() {
143146
assertParse(
144147
"""
145-
let _ = ℹ️[1️⃣#Color(red: 1, green: 1, blue: 1)#2️⃣3️⃣
148+
let _ = ℹ️[#Color(red: 1, green: 1, blue: 1)1️⃣#2️⃣
146149
""",
147150
diagnostics: [
151+
DiagnosticSpec(
152+
locationMarker: "1️⃣",
153+
message: "expected ',' in array element",
154+
fixIts: ["insert ','"]
155+
),
148156
DiagnosticSpec(
149157
locationMarker: "2️⃣",
150158
message: "expected identifier in macro expansion",
151159
fixIts: ["insert identifier"]
152160
),
153161
DiagnosticSpec(
154-
locationMarker: "3️⃣",
162+
locationMarker: "2️⃣",
155163
message: "expected ']' to end array",
156164
notes: [NoteSpec(message: "to match this opening '['")],
157165
fixIts: ["insert ']'"]
158166
),
159167
],
160168
fixedSource: """
161-
let _ = [#Color(red: 1, green: 1, blue: 1)#<#identifier#>]
169+
let _ = [#Color(red: 1, green: 1, blue: 1), #<#identifier#>]
162170
"""
163171
)
164172
}
165173

166174
func testObjectLiterals7() {
167175
assertParse(
168176
"""
169-
let _ = [#Color(withRed: 1, green: 1, whatever: 2)#1️⃣]
177+
let _ = [#Color(withRed: 1, green: 1, whatever: 2)1️⃣#2️⃣]
170178
""",
171179
diagnostics: [
172-
DiagnosticSpec(locationMarker: "1️⃣", message: "expected identifier in macro expansion", fixIts: ["insert identifier"])
180+
DiagnosticSpec(locationMarker: "1️⃣", message: "expected ',' in array element", fixIts: ["insert ','"]),
181+
DiagnosticSpec(locationMarker: "2️⃣", message: "expected identifier in macro expansion", fixIts: ["insert identifier"]),
173182
],
174183
fixedSource: """
175-
let _ = [#Color(withRed: 1, green: 1, whatever: 2)#<#identifier#>]
184+
let _ = [#Color(withRed: 1, green: 1, whatever: 2), #<#identifier#>]
176185
"""
177186
)
178187
}

‎Tests/SwiftParserTest/translated/RecoveryTests.swift‎

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2433,6 +2433,11 @@ final class RecoveryTests: ParserTestCase {
24332433
locationMarker: "4️⃣",
24342434
message: "unexpected code ') -> Int {}' in closure"
24352435
),
2436+
DiagnosticSpec(
2437+
locationMarker: "5️⃣",
2438+
message: "expected ',' in array element",
2439+
fixIts: ["insert ','"]
2440+
),
24362441
DiagnosticSpec(
24372442
locationMarker: "5️⃣",
24382443
message: "expected ']' to end array",
@@ -2449,7 +2454,7 @@ final class RecoveryTests: ParserTestCase {
24492454
struct Foo19605164 {
24502455
func a(s: S)
24512456
}[{{g) -> Int {}
2452-
}}]}
2457+
}},]}
24532458
#endif
24542459
"""
24552460
)

0 commit comments

Comments
 (0)