Skip to content

Detect and recover from missing comma in tupel #898

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
Oct 21, 2022
Merged

Detect and recover from missing comma in tupel #898

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
14 changes: 7 additions & 7 deletions Sources/SwiftParser/Parser.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,7 +149,7 @@ public struct Parser: TokenConsumer {
}

@_spi(RawSyntax)
public mutating func missingToken(_ kind: RawTokenKind, text: SyntaxText?) -> RawTokenSyntax {
public mutating func missingToken(_ kind: RawTokenKind, text: SyntaxText? = nil) -> RawTokenSyntax {
return RawTokenSyntax(missing: kind, text: text, arena: self.arena)
}

Expand Down Expand Up @@ -327,7 +327,7 @@ extension Parser {
if let remapping = remapping {
return $0.missingToken(remapping, text: kind.defaultText)
} else {
return $0.missingToken(kind, text: nil)
return $0.missingToken(kind)
}
}
)
Expand Down Expand Up @@ -366,7 +366,7 @@ extension Parser {
return expectImpl(
consume: { $0.consume(ifAny: kinds, contextualKeywords: contextualKeywords) },
canRecoverTo: { $0.canRecoverTo(kinds, contextualKeywords: contextualKeywords) },
makeMissing: { $0.missingToken(defaultKind, text: nil) }
makeMissing: { $0.missingToken(defaultKind) }
)
}

Expand All @@ -390,7 +390,7 @@ extension Parser {
if let unknown = self.consume(if: .unknown) {
return (
RawUnexpectedNodesSyntax(elements: [RawSyntax(unknown)], arena: self.arena),
self.missingToken(.identifier, text: nil)
self.missingToken(.identifier)
)
}
if let number = self.consume(ifAny: [.integerLiteral, .floatingLiteral]) {
Expand All @@ -407,7 +407,7 @@ extension Parser {
}
return (
nil,
self.missingToken(.identifier, text: nil)
self.missingToken(.identifier)
)
}

Expand All @@ -419,12 +419,12 @@ extension Parser {
if let unknown = self.consume(if: .unknown) {
return (
RawUnexpectedNodesSyntax(elements: [RawSyntax(unknown)], arena: self.arena),
self.missingToken(.identifier, text: nil)
self.missingToken(.identifier)
)
}
return (
nil,
self.missingToken(.identifier, text: nil)
self.missingToken(.identifier)
)
}

Expand Down
28 changes: 27 additions & 1 deletion Sources/SwiftParser/Types.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -426,10 +426,36 @@ extension Parser {
unexpectedBeforeColon = nil
colon = nil
}

// In the case that the input is "(foo bar)" we have to decide whether we parse it as "(foo: bar)" or "(foo, bar)".
// As most people write identifiers lowercase and types capitalized, we decide on the first character of the first token
if let first = first,
second == nil,
colon?.isMissing == true,
first.tokenText.description.first?.isUppercase == true {
elements.append(RawTupleTypeElementSyntax(
inOut: nil,
name: nil,
secondName: nil,
unexpectedBeforeColon,
colon: nil,
type: RawTypeSyntax(RawSimpleTypeIdentifierSyntax(name: first, genericArgumentClause: nil, arena: self.arena)),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

In this case we have to "reparse" an already as identifier parsed token as a type. I'm not quite sure if there is a better way.

Copy link
Member

Choose a reason for hiding this comment

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

I don’t see any issues here in wrapping first in a RawSimpleTypeIdentifierSyntax but because we won’t be parsing generic types like Array<Int> correctly here. It would also be worth adding test cases for this, I think.

What I don’t understand here, is why we are wrapping first here. For (foo Bar) doesn’t this result in two tuple elements, one that only has a label and one that only has a type but no label? I think we should be forming a single tuple element, that has a name foo, missing colon and type Bar.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don’t see any issues here in wrapping first in a RawSimpleTypeIdentifierSyntax but because we won’t be parsing generic types like Array correctly here.

As we visit this code only if first was set above, and this is only the case if self.lookahead().startsParameterName(false) is true, first must be an argument label (and thus, always a RawSimpleTypeIdentifierSyntax), this code does not apply for things like Array<Int>

It would also be worth adding test cases for this, I think.

I added some test cases for generic types

What I don’t understand here, is why we are wrapping first here. For (foo Bar) doesn’t this result in two tuple elements, one that only has a label and one that only has a type but no label?

For (foo Bar) we start to look for an argument label (first), an optional second argument label (second), and a colon. If we found first (foo), but no second and no colon, we are in the case where we must decide if foo is an identifier and a colon is missing or a type and a comma is missing. In the first case we don't visited the commented code and proceed to parse Bar as a type. But if we think a comma is missing we must convert first (already parsed as an argument label) to a type. Bar now has to be parsed as a type, too, but as a separate RawTupleTypeElementSyntax, thus we cannot proceed with the current loop here, but we have to continue the loop and start the "tuple element parsing" on Bar again, resulting in two separate RawTupleTypeElementSyntax, with the first one missing a comma. To summarize: Is first uppercase, finish this RawTupleTypeElementSyntax directly, if not, proceed this RawTupleTypeElementSyntax.

I think we should be forming a single tuple element, that has a name foo, missing colon and type Bar.

This is exactly what's happening here if this code is not visited, respectively, foo is not uppercase.

Copy link
Member

Choose a reason for hiding this comment

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

As we visit this code only if first was set above, and this is only the case if self.lookahead().startsParameterName(false) is true, first must be an argument label (and thus, always a RawSimpleTypeIdentifierSyntax), this code does not apply for things like Array<Int>

Ah, I see. That makes sense. 👍

It would also be worth adding test cases for this, I think.

I added some test cases for generic types

Thank you 🙏

For (foo Bar) we start to look for an argument label (first), an optional second argument label (second), and a colon. If we found first (foo), but no second and no colon, we are in the case where we must decide if foo is an identifier and a colon is missing or a type and a comma is missing. In the first case we don't visited the commented code and proceed to parse Bar as a type. But if we think a comma is missing we must convert first (already parsed as an argument label) to a type. Bar now has to be parsed as a type, too, but as a separate RawTupleTypeElementSyntax, thus we cannot proceed with the current loop here, but we have to continue the loop and start the "tuple element parsing" on Bar again, resulting in two separate RawTupleTypeElementSyntax, with the first one missing a comma. To summarize: Is first uppercase, finish this RawTupleTypeElementSyntax directly, if not, proceed this RawTupleTypeElementSyntax.

Oh, I know what my misunderstanding was. I thought we were parsing tuples in patterns (as in let (a, b) = foo()) here, but this is for tuple types. In that case everything makes sense 👍

ellipsis: nil,
initializer: nil,
trailingComma: self.missingToken(.comma),
arena: self.arena
))
keepGoing = true
continue
}
// Parse the type annotation.
let type = self.parseType(misplacedSpecifiers: misplacedSpecifiers)
let ellipsis = self.currentToken.isEllipsis ? self.consumeAnyToken() : nil
let trailingComma = self.consume(if: .comma)
var trailingComma = self.consume(if: .comma)
if trailingComma == nil && self.withLookahead({ $0.canParseType() }) {
// If the next token does not close the tuple, it is very likely the user forgot the comma.
trailingComma = self.missingToken(.comma)
}
keepGoing = trailingComma != nil
elements.append(RawTupleTypeElementSyntax(
inOut: nil,
Expand Down
79 changes: 78 additions & 1 deletion Tests/SwiftParserTest/Types.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -357,10 +357,87 @@ final class TypeParameterPackTests: XCTestCase {
func testParameterPacks28() throws {
// We allow whitespace between the generic parameter and the '...', this is
// consistent with regular variadic parameters.
AssertParse(
"""
func f1<T ...>(_ x: T ...) -> (T ...) {}
""")
}

func testMissingCommaInType() throws {
AssertParse(
"""
func f1<T ...>(_ x: T ...) -> (T ...) {}
var foo: (Int)
""")

AssertParse(
"""
var foo: (Int, Int)
""")

AssertParse(
"""
var foo: (bar: Int 1️⃣bar2: Int)
""",
diagnostics: [
DiagnosticSpec(message: "expected ',' in tuple type")
])

AssertParse(
"""
var foo: (bar: Int 1️⃣Int)
""",
diagnostics: [
DiagnosticSpec(message: "expected ',' in tuple type")
])

AssertParse(
"""
var foo: (a 1️⃣Int)
""",
diagnostics: [
DiagnosticSpec(message: "expected ':' in tuple type")
])

AssertParse(
"""
var foo: (A 1️⃣Int)
""",
diagnostics: [
DiagnosticSpec(message: "expected ',' in tuple type")
])

AssertParse(
"""
var foo: (_ 1️⃣a 2️⃣Int)
""",
diagnostics: [
DiagnosticSpec(locationMarker: "1️⃣", message: "expected ':' in tuple type"),
DiagnosticSpec(locationMarker: "2️⃣", message: "expected ',' in tuple type")
])

AssertParse(
"""
var foo: (Array<Foo> 1️⃣Array<Bar>)
""",
diagnostics: [
DiagnosticSpec(message: "expected ',' in tuple type"),
])

AssertParse(
"""
var foo: (a 1️⃣Array<Bar>)
""",
diagnostics: [
DiagnosticSpec(message: "expected ':' in tuple type"),
])

AssertParse(
"""
var foo: (Array<Foo> 1️⃣a)
""",
diagnostics: [
DiagnosticSpec(message: "expected ',' in tuple type"),
])
}
}