Skip to content

Improve recovery of dollar identifiers #998

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

Improve recovery of dollar identifiers #998

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
19 changes: 13 additions & 6 deletions Sources/SwiftParser/Declarations.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1402,7 +1402,7 @@ extension Parser {
var elements = [RawFunctionParameterSyntax]()
// If we are missing the left parenthesis and the next token doesn't appear
// to be an argument label, don't parse any parameters.
let shouldSkipParameterParsing = lparen.isMissing && (!currentToken.canBeArgumentLabel || currentToken.isKeyword)
let shouldSkipParameterParsing = lparen.isMissing && (!currentToken.canBeArgumentLabel(allowDollarIdentifier: true) || currentToken.isKeyword)
if !shouldSkipParameterParsing {
var keepGoing = true
var loopProgress = LoopProgressCondition()
Expand All @@ -1426,22 +1426,26 @@ extension Parser {
misplacedSpecifiers.append(specifier)
}

let unexpectedBeforeFirstName: RawUnexpectedNodesSyntax?
let firstName: RawTokenSyntax?
let unexpectedBeforeSecondName: RawUnexpectedNodesSyntax?
let secondName: RawTokenSyntax?
let unexpectedBeforeColon: RawUnexpectedNodesSyntax?
let colon: RawTokenSyntax?
let shouldParseType: Bool

if self.withLookahead({ $0.startsParameterName(isClosure: subject.isClosure, allowMisplacedSpecifierRecovery: false) }) {
if self.currentToken.canBeArgumentLabel {
firstName = self.parseArgumentLabel()
if self.currentToken.canBeArgumentLabel(allowDollarIdentifier: true) {
(unexpectedBeforeFirstName, firstName) = self.parseArgumentLabel()
} else {
unexpectedBeforeFirstName = nil
firstName = nil
}

if self.currentToken.canBeArgumentLabel {
secondName = self.parseArgumentLabel()
if self.currentToken.canBeArgumentLabel(allowDollarIdentifier: true) {
(unexpectedBeforeSecondName, secondName) = self.parseArgumentLabel()
} else {
unexpectedBeforeSecondName = nil
secondName = nil
}
if subject.isClosure {
Expand All @@ -1453,7 +1457,9 @@ extension Parser {
shouldParseType = true
}
} else {
unexpectedBeforeFirstName = nil
firstName = nil
unexpectedBeforeSecondName = nil
secondName = nil
unexpectedBeforeColon = nil
colon = nil
Expand Down Expand Up @@ -1486,8 +1492,9 @@ extension Parser {
elements.append(RawFunctionParameterSyntax(
attributes: attrs,
modifiers: modifiers,
RawUnexpectedNodesSyntax(misplacedSpecifiers, arena: self.arena),
RawUnexpectedNodesSyntax(misplacedSpecifiers.map(RawSyntax.init) + (unexpectedBeforeFirstName?.elements ?? []), arena: self.arena),
firstName: firstName,
unexpectedBeforeSecondName,
secondName: secondName,
unexpectedBeforeColon,
colon: colon,
Expand Down
4 changes: 2 additions & 2 deletions Sources/SwiftParser/Diagnostics/ParseDiagnosticsGenerator.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -212,8 +212,8 @@ public class ParseDiagnosticsGenerator: SyntaxAnyVisitor {
let invalidIdentifier = node.previousToken(viewMode: .all),
let previousParent = invalidIdentifier.parent?.as(UnexpectedNodesSyntax.self) {
let fixIts: [FixIt]
if invalidIdentifier.tokenKind.isKeyword {
fixIts = [FixIt(message: .wrapKeywordInBackticks, changes: [
if invalidIdentifier.tokenKind.isKeyword || invalidIdentifier.tokenKind.isDollarIdentifier {
fixIts = [FixIt(message: .wrapInBackticks, changes: [
.replace(
oldNode: Syntax(invalidIdentifier),
newNode: Syntax(TokenSyntax.identifier("`\(invalidIdentifier.text)`", leadingTrivia: invalidIdentifier.leadingTrivia, trailingTrivia: invalidIdentifier.trailingTrivia))
Expand Down
2 changes: 1 addition & 1 deletion Sources/SwiftParser/Diagnostics/ParserDiagnosticMessages.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -319,7 +319,7 @@ extension FixItMessage where Self == StaticParserFixIt {
public static var removeOperatorBody: Self {
.init("remove operator body")
}
public static var wrapKeywordInBackticks: Self {
public static var wrapInBackticks: Self {
.init("if this name is unavoidable, use backticks to escape it")
}
}
Expand Down
9 changes: 9 additions & 0 deletions Sources/SwiftParser/Diagnostics/SyntaxExtensions.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,4 +133,13 @@ extension TokenKind {
return false
}
}

var isDollarIdentifier: Bool {
switch self {
case .dollarIdentifier:
return true
default:
return false
}
}
}
23 changes: 19 additions & 4 deletions Sources/SwiftParser/Expressions.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2185,8 +2185,21 @@ extension Parser {
var keepGoing: RawTokenSyntax? = nil
var loopProgress = LoopProgressCondition()
repeat {
let labelAndColon = self.consume(if: { $0.canBeArgumentLabel }, followedBy: { $0.tokenKind == .colon })
let (label, colon) = (labelAndColon?.0, labelAndColon?.1)
let unexpectedBeforeLabel: RawUnexpectedNodesSyntax?
let label: RawTokenSyntax?
let colon: RawTokenSyntax?
if let labelAndColon = self.consume(if: { $0.canBeArgumentLabel() }, followedBy: { $0.tokenKind == .colon }) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like it would be cleaner to just do

if currentToken.canBeArgumentLabel(allowDollarIdentifier: true) && peek().tokenKind == .colon {
  (unexpectedBeforeLabel, label) = parseArgumentLabel()
  colon = consumeAnyToken()
}

Or maybe not 🤷.

unexpectedBeforeLabel = nil
(label, colon) = labelAndColon
} else if let dollarLabelAndColon = self.consume(if: .dollarIdentifier, followedBy: .colon) {
unexpectedBeforeLabel = RawUnexpectedNodesSyntax([dollarLabelAndColon.0], arena: self.arena)
label = missingToken(.identifier)
colon = dollarLabelAndColon.1
} else {
unexpectedBeforeLabel = nil
label = nil
colon = nil
}

// See if we have an operator decl ref '(<op>)'. The operator token in
// this case lexes as a binary operator because it neither leads nor
Expand All @@ -2202,6 +2215,7 @@ extension Parser {
}
keepGoing = self.consume(if: .comma)
result.append(RawTupleExprElementSyntax(
unexpectedBeforeLabel,
label: label,
colon: colon,
expression: expr,
Expand Down Expand Up @@ -2231,10 +2245,11 @@ extension Parser {
var elements = [RawMultipleTrailingClosureElementSyntax]()
var loopProgress = LoopProgressCondition()
while self.lookahead().isStartOfLabelledTrailingClosure() && loopProgress.evaluate(currentToken) {
let label = self.parseArgumentLabel()
let (unexpectedBeforeLabel, label) = self.parseArgumentLabel()
let (unexpectedBeforeColon, colon) = self.expect(.colon)
let closure = self.parseClosureExpression()
elements.append(RawMultipleTrailingClosureElementSyntax(
unexpectedBeforeLabel,
label: label,
unexpectedBeforeColon,
colon: colon,
Expand All @@ -2253,7 +2268,7 @@ extension Parser.Lookahead {
// Fast path: the next two tokens must be a label and a colon.
// But 'default:' is ambiguous with switch cases and we disallow it
// (unless escaped) even outside of switches.
if !self.currentToken.canBeArgumentLabel
if !self.currentToken.canBeArgumentLabel()
|| self.at(.defaultKeyword)
|| self.peek().tokenKind != .colon {
return false
Expand Down
23 changes: 16 additions & 7 deletions Sources/SwiftParser/Names.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,9 +23,16 @@ extension Parser {
}
}

mutating func parseArgumentLabel() -> RawTokenSyntax {
assert(self.currentToken.canBeArgumentLabel)
return self.consumeAnyToken()
mutating func parseArgumentLabel() -> (RawUnexpectedNodesSyntax?, RawTokenSyntax) {
assert(self.currentToken.canBeArgumentLabel(allowDollarIdentifier: true))
if let dollarIdent = self.consume(if: .dollarIdentifier) {
return (
RawUnexpectedNodesSyntax(elements: [RawSyntax(dollarIdent)], arena: self.arena),
self.missingToken(.identifier, text: nil)
)
} else {
return (nil, self.consumeAnyToken())
}
}
}

Expand Down Expand Up @@ -86,7 +93,7 @@ extension Parser {
// A close parenthesis, if empty lists are allowed.
let nextIsRParen = flags.contains(.zeroArgCompoundNames) && next.tokenKind == .rightParen
// An argument label.
let nextIsArgLabel = next.canBeArgumentLabel || next.tokenKind == .colon
let nextIsArgLabel = next.canBeArgumentLabel() || next.tokenKind == .colon

guard nextIsRParen || nextIsArgLabel else {
return nil
Expand All @@ -105,7 +112,7 @@ extension Parser {
var loopProgress = LoopProgressCondition()
while !self.at(any: [.eof, .rightParen]) && loopProgress.evaluate(currentToken) {
// Check to see if there is an argument label.
assert(self.currentToken.canBeArgumentLabel && self.peek().tokenKind == .colon)
assert(self.currentToken.canBeArgumentLabel() && self.peek().tokenKind == .colon)
let name = self.consumeAnyToken()
let (unexpectedBeforeColon, colon) = self.expect(.colon)
elements.append(RawDeclNameArgumentSyntax(
Expand Down Expand Up @@ -217,7 +224,7 @@ extension Parser.Lookahead {
var loopProgress = LoopProgressCondition()
while !lookahead.at(any: [.eof, .rightParen]) && loopProgress.evaluate(lookahead.currentToken) {
// Check to see if there is an argument label.
guard lookahead.currentToken.canBeArgumentLabel && lookahead.peek().tokenKind == .colon else {
guard lookahead.currentToken.canBeArgumentLabel() && lookahead.peek().tokenKind == .colon else {
return false
}

Expand All @@ -236,14 +243,16 @@ extension Parser.Lookahead {
}

extension Lexer.Lexeme {
var canBeArgumentLabel: Bool {
func canBeArgumentLabel(allowDollarIdentifier: Bool = false) -> Bool {
if TypeSpecifier(lexeme: self) != nil {
return false
}
switch self.tokenKind {
case .identifier, .wildcardKeyword:
// Identifiers, escaped identifiers, and '_' can be argument labels.
return true
case .dollarIdentifier:
return allowDollarIdentifier
default:
// All other keywords can be argument labels.
return self.isKeyword
Expand Down
2 changes: 1 addition & 1 deletion Sources/SwiftParser/Parser.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -393,7 +393,7 @@ extension Parser {
self.missingToken(.identifier)
)
}
if let number = self.consume(ifAny: [.integerLiteral, .floatingLiteral]) {
if let number = self.consume(ifAny: [.integerLiteral, .floatingLiteral, .dollarIdentifier]) {
return (
RawUnexpectedNodesSyntax(elements: [RawSyntax(number)], arena: self.arena),
self.missingToken(.identifier, text: nil)
Expand Down
17 changes: 14 additions & 3 deletions Sources/SwiftParser/Patterns.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ extension Parser {
case leftParen
case wildcardKeyword
case identifier
case dollarIdentifier // For recovery
case letKeyword
case varKeyword

Expand All @@ -58,6 +59,7 @@ extension Parser {
case .leftParen: self = .leftParen
case .wildcardKeyword: self = .wildcardKeyword
case .identifier: self = .identifier
case .dollarIdentifier: self = .dollarIdentifier
case .letKeyword: self = .letKeyword
case .varKeyword: self = .varKeyword
default: return nil
Expand All @@ -69,6 +71,7 @@ extension Parser {
case .leftParen: return .leftParen
case .wildcardKeyword: return .wildcardKeyword
case .identifier: return .identifier
case .dollarIdentifier: return .dollarIdentifier
case .letKeyword: return .letKeyword
case .varKeyword: return .varKeyword
}
Expand Down Expand Up @@ -100,6 +103,14 @@ extension Parser {
identifier: identifier,
arena: self.arena
))
case (.dollarIdentifier, let handle)?:
let dollarIdent = self.eat(handle)
let unexpectedBeforeIdentifier = RawUnexpectedNodesSyntax(elements: [RawSyntax(dollarIdent)], arena: self.arena)
return RawPatternSyntax(RawIdentifierPatternSyntax(
unexpectedBeforeIdentifier,
identifier: missingToken(.identifier),
arena: self.arena
))
case (.letKeyword, let handle)?,
(.varKeyword, let handle)?:
let letOrVar = self.eat(handle)
Expand Down Expand Up @@ -324,7 +335,7 @@ extension Parser.Lookahead {
}

// To have a parameter name here, we need a name.
guard self.currentToken.canBeArgumentLabel else {
guard self.currentToken.canBeArgumentLabel(allowDollarIdentifier: true) else {
return false
}

Expand All @@ -335,7 +346,7 @@ extension Parser.Lookahead {
}

// If the next token can be an argument label, we might have a name.
if nextTok.canBeArgumentLabel {
if nextTok.canBeArgumentLabel(allowDollarIdentifier: true) {
// If the first name wasn't "isolated", we're done.
if !self.atContextualKeyword("isolated") &&
!self.atContextualKeyword("some") &&
Expand All @@ -351,7 +362,7 @@ extension Parser.Lookahead {
return true // isolated :
}
self.consumeAnyToken()
return self.currentToken.canBeArgumentLabel && self.peek().tokenKind == .colon
return self.currentToken.canBeArgumentLabel(allowDollarIdentifier: true) && self.peek().tokenKind == .colon
}
}

Expand Down
17 changes: 12 additions & 5 deletions Sources/SwiftParser/Types.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -398,7 +398,9 @@ extension Parser {
var keepGoing = true
var loopProgress = LoopProgressCondition()
while !self.at(any: [.eof, .rightParen]) && keepGoing && loopProgress.evaluate(currentToken) {
let unexpectedBeforeFirst: RawUnexpectedNodesSyntax?
let first: RawTokenSyntax?
let unexpectedBeforeSecond: RawUnexpectedNodesSyntax?
let second: RawTokenSyntax?
let unexpectedBeforeColon: RawUnexpectedNodesSyntax?
let colon: RawTokenSyntax?
Expand All @@ -407,21 +409,25 @@ extension Parser {
while let specifier = self.consume(ifAnyIn: TypeSpecifier.self) {
misplacedSpecifiers.append(specifier)
}
first = self.parseArgumentLabel()
(unexpectedBeforeFirst, first) = self.parseArgumentLabel()
if let parsedColon = self.consume(if: .colon) {
unexpectedBeforeSecond = nil
second = nil
unexpectedBeforeColon = nil
colon = parsedColon
} else if self.currentToken.canBeArgumentLabel && self.peek().tokenKind == .colon {
second = self.parseArgumentLabel()
} else if self.currentToken.canBeArgumentLabel(allowDollarIdentifier: true) && self.peek().tokenKind == .colon {
(unexpectedBeforeSecond, second) = self.parseArgumentLabel()
(unexpectedBeforeColon, colon) = self.expect(.colon)
} else {
unexpectedBeforeSecond = nil
second = nil
unexpectedBeforeColon = nil
colon = RawTokenSyntax(missing: .colon, arena: self.arena)
}
} else {
unexpectedBeforeFirst = nil
first = nil
unexpectedBeforeSecond = nil
second = nil
unexpectedBeforeColon = nil
colon = nil
Expand Down Expand Up @@ -459,8 +465,9 @@ extension Parser {
keepGoing = trailingComma != nil
elements.append(RawTupleTypeElementSyntax(
inOut: nil,
RawUnexpectedNodesSyntax(misplacedSpecifiers, arena: self.arena),
RawUnexpectedNodesSyntax(misplacedSpecifiers.map(RawSyntax.init) + (unexpectedBeforeFirst?.elements ?? []), arena: self.arena),
Copy link
Contributor

Choose a reason for hiding this comment

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

There's a bunch of places we're doing things like this now. Couldn't we just return [RawSyntax] any calls that return unexpected + make all the unexpected parameters [RawSyntax] instead? That would both avoid creating unexpected nodes that aren't removed + avoid looking into them to grab elements.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don’t think I like the idea of changing the parameters of all raw nodes from RawUnexpectedNodesSyntax to RawSyntax (incidentally that would also prevent us from using all the convenience initializer on RawUnexpectedNodesSyntax we currently have (e.g. the one taking [RawSyntax?]). I put together #1005. I’m not entirely sure whether I like the change yet because it does add as bit of complexity on the declaration side, but the call side is rather nice. WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

I've commented in that PR. Personally I'd find having all [RawSyntax] more convenient than the [RawSyntax?] convenience initializer. Though I haven't actually checked how many places that's being used, so maybe that wouldn't be the case. Anyway, that PR is better than the status quo so I'd be fine with merging that in.

name: first,
unexpectedBeforeSecond,
secondName: second,
unexpectedBeforeColon,
colon: colon,
Expand Down Expand Up @@ -626,7 +633,7 @@ extension Parser.Lookahead {
// by a type annotation.
if self.startsParameterName(isClosure: false, allowMisplacedSpecifierRecovery: false) {
self.consumeAnyToken()
if self.currentToken.canBeArgumentLabel {
if self.currentToken.canBeArgumentLabel() {
self.consumeAnyToken()
guard self.at(.colon) else {
return false
Expand Down
Loading