Skip to content

Don’t eat closing braces while trying to recover to a declaration start keyword #1023

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.

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 4 commits into from
Oct 27, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
7 changes: 6 additions & 1 deletion Sources/SwiftParser/Declarations.swift
Original file line number Diff line number Diff line change
Expand Up @@ -190,7 +190,12 @@ extension Parser {
let attrs = DeclAttributes(
attributes: self.parseAttributeList(),
modifiers: self.parseModifierList())
switch self.canRecoverTo(anyIn: DeclarationStart.self) {

// If we are inside a memberDecl list, we don't want to eat closing braces (which most likely close the outer context)
// while recoverying to the declaration start.
let recoveryPrecedence = inMemberDeclList ? TokenPrecedence.closingBrace : nil

switch self.canRecoverTo(anyIn: DeclarationStart.self, recoveryPrecedence: recoveryPrecedence) {
case (.importKeyword, let handle)?:
return RawDeclSyntax(self.parseImportDeclaration(attrs, handle))
case (.classKeyword, let handle)?:
Expand Down
5 changes: 3 additions & 2 deletions Sources/SwiftParser/Parser.swift
Original file line number Diff line number Diff line change
Expand Up @@ -263,13 +263,14 @@ extension Parser {
/// If so, return the token that we can recover to and a handle that can be
/// used to consume the unexpected tokens and the token we recovered to.
func canRecoverTo<Subset: RawTokenKindSubset>(
anyIn subset: Subset.Type
anyIn subset: Subset.Type,
recoveryPrecedence: TokenPrecedence? = nil
) -> (Subset, RecoveryConsumptionHandle)? {
if let (kind, handle) = self.at(anyIn: subset) {
return (kind, RecoveryConsumptionHandle(unexpectedTokens: 0, tokenConsumptionHandle: handle))
}
var lookahead = self.lookahead()
return lookahead.canRecoverTo(anyIn: subset)
return lookahead.canRecoverTo(anyIn: subset, recoveryPrecedence: recoveryPrecedence)
}

/// Eat a token that we know we are currently positioned at, based on `canRecoverTo(anyIn:)`.
Expand Down
4 changes: 2 additions & 2 deletions Sources/_SwiftSyntaxTestSupport/SyntaxComparison.swift
Original file line number Diff line number Diff line change
Expand Up @@ -45,8 +45,8 @@ extension TreeDifference: CustomDebugStringConvertible {
public var debugDescription: String {
let includeTrivia = reason == .trivia

let expectedConverter = SourceLocationConverter(file: "Baseline.swift", source: baseline.description)
let actualConverter = SourceLocationConverter(file: "Actual.swift", source: node.description)
let expectedConverter = SourceLocationConverter(file: "Baseline.swift", tree: baseline.root)
let actualConverter = SourceLocationConverter(file: "Actual.swift", tree: node.root)

let expectedDesc = baseline.debugDescription(includeTrivia: includeTrivia, converter: expectedConverter)
let actualDesc = node.debugDescription(includeTrivia: includeTrivia, converter: actualConverter)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -62,11 +62,11 @@ private extension TriviaPiece {
let (label, value) = Mirror(reflecting: self).children.first!
switch value {
case let value as String:
return FunctionCallExpr(calledExpression: MemberAccessExpr(name: label!)) {
return FunctionCallExpr(callee: ".\(label!)") {
TupleExprElement(expression: StringLiteralExpr(content: value))
}
case let value as Int:
return FunctionCallExpr(calledExpression: MemberAccessExpr(name: label!)) {
return FunctionCallExpr(callee: ".\(label!)") {
TupleExprElement(expression: IntegerLiteralExpr(value))
}
default:
Expand Down Expand Up @@ -106,7 +106,7 @@ extension SyntaxProtocol {
private var debugInitCallExpr: ExprSyntaxProtocol {
let mirror = Mirror(reflecting: self)
if self.isCollection {
return FunctionCallExpr(calledExpression: IdentifierExpr(String("\(type(of: self))"))) {
return FunctionCallExpr(callee: "\(type(of: self))") {
TupleExprElement(
expression: ArrayExpr() {
for child in mirror.children {
Expand All @@ -131,7 +131,7 @@ extension SyntaxProtocol {
tokenInitializerName = String(tokenKindStr[..<tokenKindStr.firstIndex(of: "(")!])
requiresExplicitText = true
}
return FunctionCallExpr(calledExpression: MemberAccessExpr(name: tokenInitializerName)) {
return FunctionCallExpr(callee: ".\(tokenInitializerName)") {
if requiresExplicitText {
TupleExprElement(
expression: StringLiteralExpr(content: token.text)
Expand Down Expand Up @@ -160,7 +160,7 @@ extension SyntaxProtocol {
}
}
} else {
return FunctionCallExpr(calledExpression: IdentifierExpr(String("\(type(of: self))"))) {
return FunctionCallExpr(callee: "\(type(of: self))") {
for child in mirror.children {
let label = child.label!
let value = child.value as! SyntaxProtocol?
Expand Down
80 changes: 80 additions & 0 deletions Tests/SwiftParserTest/DeclarationTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -1215,6 +1215,86 @@ final class DeclarationTests: XCTestCase {
// `actor` cannot recover from a missing identifier since it's contextual
// based on the presence of the identifier.
}

func testDontNestClassesIfTheyContainUnexpectedTokens() {
// There used to be a bug where `class B` was parsed as a nested class of A
// because recovery to the `class` keyword of B consumed the closing brace.
AssertParse(
"""
class A {
1️⃣^2️⃣
}
class B {
}
""",
substructure: Syntax(CodeBlockItemListSyntax([
CodeBlockItemSyntax(
item: .init(ClassDeclSyntax(
attributes: nil,
modifiers: nil,
classKeyword: .classKeyword(),
identifier: .identifier("A"),
genericParameterClause: nil,
inheritanceClause: nil,
genericWhereClause: nil,
members: MemberDeclBlockSyntax(
leftBrace: .leftBraceToken(),
members: MemberDeclListSyntax([
MemberDeclListItemSyntax(
decl: Decl(FunctionDeclSyntax(
attributes: nil,
modifiers: nil,
funcKeyword: .funcKeyword(presence: .missing),
identifier: .spacedBinaryOperator("^"),
genericParameterClause: nil,
signature: FunctionSignatureSyntax(
input: ParameterClauseSyntax(
leftParen: .leftParenToken(presence: .missing),
parameterList: FunctionParameterListSyntax([]),
rightParen: .rightParenToken(presence: .missing)
),
asyncOrReasyncKeyword: nil,
throwsOrRethrowsKeyword: nil,
output: nil
),
genericWhereClause: nil,
body: nil
)),
semicolon: nil
)
]),
rightBrace: .rightBraceToken()
)
)),
semicolon: nil,
errorTokens: nil
),
CodeBlockItemSyntax(
item: .init(ClassDeclSyntax(
attributes: nil,
modifiers: nil,
classKeyword: .classKeyword(),
identifier: .identifier("B"),
genericParameterClause: nil,
inheritanceClause: nil,
genericWhereClause: nil,
members: MemberDeclBlockSyntax(
leftBrace: .leftBraceToken(),
members: MemberDeclListSyntax([]),
rightBrace: .rightBraceToken()
)
)),
semicolon: nil,
errorTokens: nil
)
])
),
diagnostics: [
DiagnosticSpec(locationMarker: "1️⃣", message: "expected 'func' in function"),
DiagnosticSpec(locationMarker: "2️⃣", message: "expected parameter clause in function signature"),
]
)
}
}

extension Parser.DeclAttributes {
Expand Down
2 changes: 2 additions & 0 deletions Tests/SwiftParserTest/ParserTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -160,6 +160,8 @@ public class ParserTests: XCTestCase {
|| fileURL.absoluteString.contains("26773-swift-diagnosticengine-diagnose.swift")
|| fileURL.absoluteString.contains("parser-cutoff.swift")
|| fileURL.absoluteString.contains("26233-swift-iterabledeclcontext-getmembers.swift")
|| fileURL.absoluteString.contains("26190-swift-iterabledeclcontext-getmembers.swift")
|| fileURL.absoluteString.contains("26139-swift-abstractclosureexpr-setparams.swift")
Comment on lines +163 to +164
Copy link
Contributor

Choose a reason for hiding this comment

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

These aren't even that bad, pretty surprising to me that they cause a stack overflow. Isn't even 100 nested braces!

Copy link
Member Author

Choose a reason for hiding this comment

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

The issue here appears to be debug builds. In release builds, these are just fine.

}
)
}
Expand Down