From 21e5eaab9fc36218b9402af54b8b8a5a75c90048 Mon Sep 17 00:00:00 2001 From: Alex Hoppen Date: Sat, 28 Jan 2023 15:32:02 +0100 Subject: [PATCH] =?UTF-8?q?Don=E2=80=99t=20fatalError=20when=20expressing?= =?UTF-8?q?=20syntax=20nodes=20by=20string=20literals?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We should not crash the process if the source code that you use to create a syntax node using string interpolation is invalid. Instead we should: - Not check for errors in the normal string interpolation case and just return a syntax tree that has the `hasError` flag set when there are syntax errors - In either SwiftSyntaxBuilder offer `init(validating node: Self) throws` that check that `node` has no sytnax errors and throws otherwise. This allows us write e.g. `try wDeclSyntax(validating: "struct Foo {}")` to create a `DeclSyntax` that’s guaranteed to not have any syntax errors rdar://104423126 --- ...yStringInterpolationConformancesFile.swift | 22 +++--- .../MissingNodesError.swift | 2 - Sources/SwiftSyntaxBuilder/CMakeLists.txt | 1 + .../ConvenienceInitializers.swift | 2 +- .../Syntax+StringInterpolation.swift | 75 +------------------ .../SyntaxNodeWithBody.swift | 2 +- .../ValidatingSyntaxNodes.swift | 58 ++++++++++++++ ...bleByStringInterpolationConformances.swift | 22 +++--- ...n.swift => StringInterpolationTests.swift} | 25 ++++++- 9 files changed, 107 insertions(+), 102 deletions(-) create mode 100644 Sources/SwiftSyntaxBuilder/ValidatingSyntaxNodes.swift rename Tests/SwiftSyntaxBuilderTest/{StringInterpolation.swift => StringInterpolationTests.swift} (94%) diff --git a/CodeGeneration/Sources/generate-swiftsyntax/templates/swiftsyntaxbuilder/SyntaxExpressibleByStringInterpolationConformancesFile.swift b/CodeGeneration/Sources/generate-swiftsyntax/templates/swiftsyntaxbuilder/SyntaxExpressibleByStringInterpolationConformancesFile.swift index 087e22aafaf..6bb2eb22db3 100644 --- a/CodeGeneration/Sources/generate-swiftsyntax/templates/swiftsyntaxbuilder/SyntaxExpressibleByStringInterpolationConformancesFile.swift +++ b/CodeGeneration/Sources/generate-swiftsyntax/templates/swiftsyntaxbuilder/SyntaxExpressibleByStringInterpolationConformancesFile.swift @@ -25,10 +25,12 @@ let syntaxExpressibleByStringInterpolationConformancesFile = SourceFileSyntax { DeclSyntax("import SwiftParserDiagnostics") try! ExtensionDeclSyntax("extension SyntaxParseable") { + DeclSyntax("public typealias StringInterpolation = SyntaxStringInterpolation") + DeclSyntax( """ - public init(stringInterpolationOrThrow stringInterpolation: SyntaxStringInterpolation) throws { - self = try performParse(source: stringInterpolation.sourceText, parse: { parser in + public init(stringInterpolation: SyntaxStringInterpolation) { + self = performParse(source: stringInterpolation.sourceText, parse: { parser in return Self.parse(from: &parser) }) } @@ -41,19 +43,15 @@ let syntaxExpressibleByStringInterpolationConformancesFile = SourceFileSyntax { DeclSyntax( """ - // TODO: This should be fileprivate, but is currently used in - // `ConvenienceInitializers.swift`. See the corresponding TODO there. - func performParse(source: [UInt8], parse: (inout Parser) throws -> SyntaxType) throws -> SyntaxType { - return try source.withUnsafeBufferPointer { buffer in + // TODO: This should be inlined in SyntaxParseable.init(stringInterpolation:), + // but is currently used in `ConvenienceInitializers.swift`. + // See the corresponding TODO there. + func performParse(source: [UInt8], parse: (inout Parser) -> SyntaxType) -> SyntaxType { + return source.withUnsafeBufferPointer { buffer in var parser = Parser(buffer) // FIXME: When the parser supports incremental parsing, put the // interpolatedSyntaxNodes in so we don't have to parse them again. - let result = try parse(&parser) - if result.hasError { - let diagnostics = ParseDiagnosticsGenerator.diagnostics(for: result) - assert(!diagnostics.isEmpty) - throw SyntaxStringInterpolationError.diagnostics(diagnostics, tree: Syntax(result)) - } + let result = parse(&parser) return result } } diff --git a/Sources/SwiftParserDiagnostics/MissingNodesError.swift b/Sources/SwiftParserDiagnostics/MissingNodesError.swift index 52d244e39d8..3664f07ad40 100644 --- a/Sources/SwiftParserDiagnostics/MissingNodesError.swift +++ b/Sources/SwiftParserDiagnostics/MissingNodesError.swift @@ -334,8 +334,6 @@ extension ParseDiagnosticsGenerator { break } } - } else { - missingNodes = [] } let changes = missingNodes.enumerated().map { (index, missingNode) -> FixIt.Changes in diff --git a/Sources/SwiftSyntaxBuilder/CMakeLists.txt b/Sources/SwiftSyntaxBuilder/CMakeLists.txt index dc5e37f4c84..7f700cc86aa 100644 --- a/Sources/SwiftSyntaxBuilder/CMakeLists.txt +++ b/Sources/SwiftSyntaxBuilder/CMakeLists.txt @@ -12,6 +12,7 @@ add_swift_host_library(SwiftSyntaxBuilder ResultBuilderExtensions.swift Syntax+StringInterpolation.swift SyntaxNodeWithBody.swift + ValidatingSyntaxNodes.swift WithTrailingCommaSyntax+EnsuringTrailingComma.swift diff --git a/Sources/SwiftSyntaxBuilder/ConvenienceInitializers.swift b/Sources/SwiftSyntaxBuilder/ConvenienceInitializers.swift index 7790f8e95cd..e0d3f6291dd 100644 --- a/Sources/SwiftSyntaxBuilder/ConvenienceInitializers.swift +++ b/Sources/SwiftSyntaxBuilder/ConvenienceInitializers.swift @@ -176,7 +176,7 @@ extension FunctionParameterSyntax { _ source: String, for subject: Parser.ParameterSubject ) { - self = try! performParse( + self = performParse( source: Array(source.utf8), parse: { let raw = RawSyntax($0.parseFunctionParameter(for: subject)) diff --git a/Sources/SwiftSyntaxBuilder/Syntax+StringInterpolation.swift b/Sources/SwiftSyntaxBuilder/Syntax+StringInterpolation.swift index 5b77bdaed22..bec6579e09e 100644 --- a/Sources/SwiftSyntaxBuilder/Syntax+StringInterpolation.swift +++ b/Sources/SwiftSyntaxBuilder/Syntax+StringInterpolation.swift @@ -143,7 +143,7 @@ extension SyntaxStringInterpolation: StringInterpolationProtocol { public protocol SyntaxExpressibleByStringInterpolation: ExpressibleByStringInterpolation where Self.StringInterpolation == SyntaxStringInterpolation { - init(stringInterpolationOrThrow stringInterpolation: SyntaxStringInterpolation) throws + init(stringInterpolation: SyntaxStringInterpolation) } enum SyntaxStringInterpolationError: Error, CustomStringConvertible { @@ -221,34 +221,10 @@ public protocol ExpressibleByLiteralSyntax { } extension SyntaxExpressibleByStringInterpolation { - /// Initialize a syntax node by parsing the contents of the interpolation. - /// This function is marked `@_transparent` so that fatalErrors raised here - /// are reported at the string literal itself. - /// This makes debugging easier because Xcode will jump to the string literal - /// that had a parsing error instead of the initializer that raised the `fatalError` - @_transparent - public init(stringInterpolation: SyntaxStringInterpolation) { - do { - try self.init(stringInterpolationOrThrow: stringInterpolation) - } catch { - fatalError(String(describing: error)) - } - } - - @_transparent public init(stringLiteral value: String) { - do { - try self.init(stringLiteralOrThrow: value) - } catch { - fatalError(String(describing: error)) - } - } - - /// Initialize a syntax node from a string literal. - public init(stringLiteralOrThrow value: String) throws { var interpolation = SyntaxStringInterpolation() interpolation.appendLiteral(value) - try self.init(stringInterpolationOrThrow: interpolation) + self.init(stringInterpolation: interpolation) } } @@ -445,7 +421,7 @@ extension Optional: ExpressibleByLiteralSyntax where Wrapped: ExpressibleByLiter } extension TokenSyntax: SyntaxExpressibleByStringInterpolation { - public init(stringInterpolationOrThrow stringInterpolation: SyntaxStringInterpolation) throws { + public init(stringInterpolation: SyntaxStringInterpolation) { let string = stringInterpolation.sourceText.withUnsafeBufferPointer { buf in return String(syntaxText: SyntaxText(buffer: buf)) } @@ -476,21 +452,7 @@ struct UnexpectedTrivia: DiagnosticMessage { } extension Trivia: ExpressibleByStringInterpolation { - /// Initialize a syntax node by parsing the contents of the interpolation. - /// This function is marked `@_transparent` so that fatalErrors raised here - /// are reported at the string literal itself. - /// This makes debugging easier because Xcode will jump to the string literal - /// that had a parsing error instead of the initializer that raised the `fatalError` - @_transparent public init(stringInterpolation: String.StringInterpolation) { - do { - try self.init(stringInterpolationOrThrow: stringInterpolation) - } catch { - fatalError(String(describing: error)) - } - } - - public init(stringInterpolationOrThrow stringInterpolation: String.StringInterpolation) throws { var text = String(stringInterpolation: stringInterpolation) let pieces = text.withUTF8 { (buf) -> [TriviaPiece] in // The leading trivia position is a little bit less restrictive (it allows a shebang), so let's use it. @@ -499,40 +461,11 @@ extension Trivia: ExpressibleByStringInterpolation { } self.init(pieces: pieces) - - if pieces.contains(where: { $0.isUnexpected }) { - var diagnostics: [Diagnostic] = [] - let tree = SourceFileSyntax(statements: [], eofToken: .eof(leadingTrivia: self)) - var offset = 0 - for piece in pieces { - if case .unexpectedText(let contents) = piece { - diagnostics.append( - Diagnostic( - node: Syntax(tree), - position: tree.position.advanced(by: offset), - message: UnexpectedTrivia(triviaContents: contents) - ) - ) - } - offset += piece.sourceLength.utf8Length - } - throw SyntaxStringInterpolationError.diagnostics(diagnostics, tree: Syntax(tree)) - } } - @_transparent public init(stringLiteral value: String) { - do { - try self.init(stringLiteralOrThrow: value) - } catch { - fatalError(String(describing: error)) - } - } - - /// Initialize a syntax node from a string literal. - public init(stringLiteralOrThrow value: String) throws { var interpolation = String.StringInterpolation(literalCapacity: 1, interpolationCount: 0) interpolation.appendLiteral(value) - try self.init(stringInterpolationOrThrow: interpolation) + self.init(stringInterpolation: interpolation) } } diff --git a/Sources/SwiftSyntaxBuilder/SyntaxNodeWithBody.swift b/Sources/SwiftSyntaxBuilder/SyntaxNodeWithBody.swift index 94ffd084a76..cef4ee219c3 100644 --- a/Sources/SwiftSyntaxBuilder/SyntaxNodeWithBody.swift +++ b/Sources/SwiftSyntaxBuilder/SyntaxNodeWithBody.swift @@ -23,7 +23,7 @@ import SwiftSyntax public struct PartialSyntaxNodeString: SyntaxExpressibleByStringInterpolation { let sourceText: [UInt8] - public init(stringInterpolationOrThrow stringInterpolation: SyntaxStringInterpolation) throws { + public init(stringInterpolation: SyntaxStringInterpolation) { self.sourceText = stringInterpolation.sourceText } } diff --git a/Sources/SwiftSyntaxBuilder/ValidatingSyntaxNodes.swift b/Sources/SwiftSyntaxBuilder/ValidatingSyntaxNodes.swift new file mode 100644 index 00000000000..a5a17fd3434 --- /dev/null +++ b/Sources/SwiftSyntaxBuilder/ValidatingSyntaxNodes.swift @@ -0,0 +1,58 @@ +//===----------------------------------------------------------------------===// +// +// This source file is part of the Swift.org open source project +// +// Copyright (c) 2014 - 2023 Apple Inc. and the Swift project authors +// Licensed under Apache License v2.0 with Runtime Library Exception +// +// See https://swift.org/LICENSE.txt for license information +// See https://swift.org/CONTRIBUTORS.txt for the list of Swift project authors +// +//===----------------------------------------------------------------------===// + +import SwiftSyntax +import SwiftDiagnostics +import SwiftParserDiagnostics + +extension SyntaxProtocol { + /// If `node` has contains no syntax errors, return `node`, otherwise + /// throw an error with diagnostics describing the syntax errors. + /// + /// This allows clients to e.g. write `try DeclSyntax(validating: "struct Foo {}")` + /// which constructs a `DeclSyntax` that's guaranteed to be free of syntax + /// errors. + public init(validating node: Self) throws { + if node.hasError { + let diagnostics = ParseDiagnosticsGenerator.diagnostics(for: node) + assert(!diagnostics.isEmpty) + throw SyntaxStringInterpolationError.diagnostics(diagnostics, tree: Syntax(node)) + } + self = node + } +} + +extension Trivia { + /// If `trivia` has contains no unexpected trivia, return `trivia`, otherwise + /// throw an error with diagnostics describing the unexpected trivia. + public init(validating trivia: Trivia) throws { + self = trivia + if pieces.contains(where: { $0.isUnexpected }) { + var diagnostics: [Diagnostic] = [] + let tree = SourceFileSyntax(statements: [], eofToken: .eof(leadingTrivia: self)) + var offset = 0 + for piece in pieces { + if case .unexpectedText(let contents) = piece { + diagnostics.append( + Diagnostic( + node: Syntax(tree), + position: tree.position.advanced(by: offset), + message: UnexpectedTrivia(triviaContents: contents) + ) + ) + } + offset += piece.sourceLength.utf8Length + } + throw SyntaxStringInterpolationError.diagnostics(diagnostics, tree: Syntax(tree)) + } + } +} diff --git a/Sources/SwiftSyntaxBuilder/generated/SyntaxExpressibleByStringInterpolationConformances.swift b/Sources/SwiftSyntaxBuilder/generated/SyntaxExpressibleByStringInterpolationConformances.swift index 1ab22af4d2d..b9b429236a1 100644 --- a/Sources/SwiftSyntaxBuilder/generated/SyntaxExpressibleByStringInterpolationConformances.swift +++ b/Sources/SwiftSyntaxBuilder/generated/SyntaxExpressibleByStringInterpolationConformances.swift @@ -19,8 +19,10 @@ import SwiftParser import SwiftParserDiagnostics extension SyntaxParseable { - public init(stringInterpolationOrThrow stringInterpolation: SyntaxStringInterpolation) throws { - self = try performParse(source: stringInterpolation.sourceText, parse: { parser in + public typealias StringInterpolation = SyntaxStringInterpolation + + public init(stringInterpolation: SyntaxStringInterpolation) { + self = performParse(source: stringInterpolation.sourceText, parse: { parser in return Self.parse(from: &parser) }) } @@ -62,19 +64,15 @@ extension SwitchCaseSyntax: SyntaxExpressibleByStringInterpolation { extension TypeSyntax: SyntaxExpressibleByStringInterpolation { } -// TODO: This should be fileprivate, but is currently used in -// `ConvenienceInitializers.swift`. See the corresponding TODO there. -func performParse(source: [UInt8], parse: (inout Parser) throws -> SyntaxType) throws -> SyntaxType { - return try source.withUnsafeBufferPointer { buffer in +// TODO: This should be inlined in SyntaxParseable.init(stringInterpolation:), +// but is currently used in `ConvenienceInitializers.swift`. +// See the corresponding TODO there. +func performParse(source: [UInt8], parse: (inout Parser) -> SyntaxType) -> SyntaxType { + return source.withUnsafeBufferPointer { buffer in var parser = Parser(buffer) // FIXME: When the parser supports incremental parsing, put the // interpolatedSyntaxNodes in so we don't have to parse them again. - let result = try parse(&parser) - if result.hasError { - let diagnostics = ParseDiagnosticsGenerator.diagnostics(for: result) - assert(!diagnostics.isEmpty) - throw SyntaxStringInterpolationError.diagnostics(diagnostics, tree: Syntax(result)) - } + let result = parse(&parser) return result } } diff --git a/Tests/SwiftSyntaxBuilderTest/StringInterpolation.swift b/Tests/SwiftSyntaxBuilderTest/StringInterpolationTests.swift similarity index 94% rename from Tests/SwiftSyntaxBuilderTest/StringInterpolation.swift rename to Tests/SwiftSyntaxBuilderTest/StringInterpolationTests.swift index 8d8df2f4746..8f3791ca528 100644 --- a/Tests/SwiftSyntaxBuilderTest/StringInterpolation.swift +++ b/Tests/SwiftSyntaxBuilderTest/StringInterpolationTests.swift @@ -471,9 +471,10 @@ final class StringInterpolationTests: XCTestCase { } func testInvalidTrivia() { - var interpolation = String.StringInterpolation(literalCapacity: 1, interpolationCount: 0) - interpolation.appendLiteral("/*comment*/ invalid /*comm*/") - XCTAssertThrowsError(try Trivia(stringInterpolationOrThrow: interpolation)) { error in + let invalid = Trivia("/*comment*/ invalid /*comm*/") + XCTAssertEqual(invalid, [.blockComment("/*comment*/"), .spaces(1), .unexpectedText("invalid"), .spaces(1), .blockComment("/*comm*/")]) + + XCTAssertThrowsError(try Trivia(validating: "/*comment*/ invalid /*comm*/")) { error in AssertStringsEqualWithDiff( String(describing: error), """ @@ -485,4 +486,22 @@ final class StringInterpolationTests: XCTestCase { ) } } + + func testInvalidSyntax() { + let invalid = DeclSyntax("return 1") + XCTAssert(invalid.hasError) + + XCTAssertThrowsError(try DeclSyntax(validating: "return 1")) { error in + AssertStringsEqualWithDiff( + String(describing: error), + """ + + 1 │ return 1 + ∣ │ ╰─ expected declaration + ∣ ╰─ unexpected code 'return 1' before declaration + + """ + ) + } + } }