From b00a40e7ede7ab81c62a07aa1011f8cf72d3c0f1 Mon Sep 17 00:00:00 2001 From: Alex Hoppen Date: Fri, 21 Apr 2023 13:44:50 -0700 Subject: [PATCH 1/2] Default to separating tokens by a space in BasicFormat Instead of having to opt into separating tokens by spaces, assume that all tokens need to be separated by spaces and explicitly opt-out of space separtion of tokens. --- .../BasicFormatExtensionsFile.swift | 56 ------- Sources/SwiftBasicFormat/BasicFormat.swift | 131 +++++++++++++--- .../generated/BasicFormat+Extensions.swift | 148 ------------------ .../DiagnosticExtensions.swift | 137 ++++++++-------- .../MissingNodesError.swift | 28 ++-- .../MissingTokenError.swift | 15 +- .../ParseDiagnosticsGenerator.swift | 4 +- .../PresenceUtils.swift | 6 +- .../BasicFormatTests.swift | 67 ++++++++ Tests/SwiftParserTest/AttributeTests.swift | 4 +- Tests/SwiftParserTest/DeclarationTests.swift | 3 +- .../VariadicGenericsTests.swift | 4 +- ...AvailabilityQueryUnavailabilityTests.swift | 8 +- .../DiagnoseDynamicReplacementTests.swift | 2 +- .../translated/InvalidTests.swift | 2 +- .../translated/RecoveryTests.swift | 2 +- .../translated/SubscriptingTests.swift | 4 +- .../translated/ToplevelLibraryTests.swift | 3 +- 18 files changed, 280 insertions(+), 344 deletions(-) diff --git a/CodeGeneration/Sources/generate-swiftsyntax/templates/swiftbasicformat/BasicFormatExtensionsFile.swift b/CodeGeneration/Sources/generate-swiftsyntax/templates/swiftbasicformat/BasicFormatExtensionsFile.swift index 33cba270f02..e56a80f5ddd 100644 --- a/CodeGeneration/Sources/generate-swiftsyntax/templates/swiftbasicformat/BasicFormatExtensionsFile.swift +++ b/CodeGeneration/Sources/generate-swiftsyntax/templates/swiftbasicformat/BasicFormatExtensionsFile.swift @@ -76,62 +76,6 @@ let basicFormatExtensionsFile = SourceFileSyntax(leadingTrivia: copyrightHeader) } """ ) - - try VariableDeclSyntax("var requiresLeadingSpace: Bool") { - StmtSyntax( - """ - if let keyPath = keyPathInParent, let requiresLeadingSpace = keyPath.requiresLeadingSpace { - return requiresLeadingSpace - } - """ - ) - - try SwitchExprSyntax("switch tokenKind") { - for token in SYNTAX_TOKENS { - if token.requiresLeadingSpace { - SwitchCaseSyntax("case .\(raw: token.swiftKind):") { - StmtSyntax("return true") - } - } - } - for keyword in KEYWORDS where keyword.requiresLeadingSpace { - SwitchCaseSyntax("case .keyword(.\(raw: keyword.escapedName)):") { - StmtSyntax("return true") - } - } - SwitchCaseSyntax("default:") { - StmtSyntax("return false") - } - } - } - - try VariableDeclSyntax("var requiresTrailingSpace: Bool") { - StmtSyntax( - """ - if let keyPath = keyPathInParent, let requiresTrailingSpace = keyPath.requiresTrailingSpace { - return requiresTrailingSpace - } - """ - ) - - try SwitchExprSyntax("switch tokenKind") { - for token in SYNTAX_TOKENS { - if token.requiresTrailingSpace { - SwitchCaseSyntax("case .\(raw: token.swiftKind):") { - StmtSyntax("return true") - } - } - } - for keyword in KEYWORDS where keyword.requiresTrailingSpace { - SwitchCaseSyntax("case .keyword(.\(raw: keyword.escapedName)):") { - StmtSyntax("return true") - } - } - SwitchCaseSyntax("default:") { - StmtSyntax("return false") - } - } - } } try! ExtensionDeclSyntax("fileprivate extension AnyKeyPath") { diff --git a/Sources/SwiftBasicFormat/BasicFormat.swift b/Sources/SwiftBasicFormat/BasicFormat.swift index 930079cb1ad..4f4c6847a95 100644 --- a/Sources/SwiftBasicFormat/BasicFormat.swift +++ b/Sources/SwiftBasicFormat/BasicFormat.swift @@ -35,21 +35,28 @@ open class BasicFormat: SyntaxRewriter { /// This is used as a reference-point to indent user-indented code. private var anchorPoints: [TokenSyntax: Trivia] = [:] + public let viewMode: SyntaxTreeViewMode + /// The previously visited token. This is faster than accessing /// `token.previousToken` inside `visit(_:TokenSyntax)`. `nil` if no token has /// been visited yet. private var previousToken: TokenSyntax? = nil - public init(indentationWidth: Trivia = .spaces(4), initialIndentation: Trivia = []) { + public init( + indentationWidth: Trivia = .spaces(4), + initialIndentation: Trivia = [], + viewMode: SyntaxTreeViewMode = .sourceAccurate + ) { self.indentationWidth = indentationWidth self.indentationStack = [(indentation: initialIndentation, isUserDefined: false)] + self.viewMode = viewMode } // MARK: - Updating indentation level public func increaseIndentationLevel(to userDefinedIndentation: Trivia? = nil) { if let userDefinedIndentation = userDefinedIndentation { - indentationStack.append((indentation: userDefinedIndentation, isUserDefined: false)) + indentationStack.append((indentation: userDefinedIndentation, isUserDefined: true)) } else { indentationStack.append((indentation: currentIndentationLevel + indentationWidth, isUserDefined: false)) } @@ -65,7 +72,7 @@ open class BasicFormat: SyntaxRewriter { open override func visitPre(_ node: Syntax) { if requiresIndent(node) { - if let firstToken = node.firstToken(viewMode: .sourceAccurate), + if let firstToken = node.firstToken(viewMode: viewMode), let tokenIndentation = firstToken.leadingTrivia.indentation(isOnNewline: false), !tokenIndentation.isEmpty, let lastNonUserDefinedIndentation = indentationStack.last(where: { !$0.isUserDefined })?.indentation @@ -103,6 +110,8 @@ open class BasicFormat: SyntaxRewriter { return true case .codeBlockItemList: return true + case .ifConfigClauseList: + return true case .memberDeclList: return true case .switchCaseList: @@ -118,7 +127,7 @@ open class BasicFormat: SyntaxRewriter { var ancestor: Syntax = Syntax(token) while let parent = ancestor.parent { ancestor = parent - if let firstToken = parent.firstToken(viewMode: .sourceAccurate), + if let firstToken = parent.firstToken(viewMode: viewMode), let anchorPointIndentation = anchorPoints[firstToken] { return anchorPointIndentation @@ -148,12 +157,18 @@ open class BasicFormat: SyntaxRewriter { var ancestor: Syntax = Syntax(token) while let parent = ancestor.parent { ancestor = parent - if ancestor.position != token.position { + if ancestor.position != token.position || ancestor.firstToken(viewMode: viewMode) != token { break } if let ancestorsParent = ancestor.parent, childrenSeparatedByNewline(ancestorsParent) { return true } + switch ancestor.keyPathInParent { + case \IfConfigClauseSyntax.elements: + return true + default: + break + } } return false @@ -161,28 +176,94 @@ open class BasicFormat: SyntaxRewriter { open func requiresWhitespace(between first: TokenSyntax?, and second: TokenSyntax?) -> Bool { switch (first?.tokenKind, second?.tokenKind) { - case (.leftParen, .leftBrace), // Ensures there is not a space in `.map({ $0.foo })` - (.exclamationMark, .leftParen), // Ensures there is not a space in `myOptionalClosure!()` - (.exclamationMark, .period), // Ensures there is not a space in `myOptionalBar!.foo()` - (.keyword(.as), .exclamationMark), // Ensures there is not a space in `as!` - (.keyword(.as), .postfixQuestionMark), // Ensures there is not a space in `as?` - (.keyword(.try), .exclamationMark), // Ensures there is not a space in `try!` - (.keyword(.try), .postfixQuestionMark), // Ensures there is not a space in `try?`: - (.postfixQuestionMark, .leftParen), // Ensures there is not a space in `init?()` or `myOptionalClosure?()`s - (.postfixQuestionMark, .rightAngle), // Ensures there is not a space in `ContiguousArray` - (.postfixQuestionMark, .rightParen): // Ensures there is not a space in `myOptionalClosure?()` + case (.atSign, _), + (.backslash, _), + (.backtick, _), + (.dollarIdentifier, .period), // a.b + (.eof, _), + (.exclamationMark, .leftParen), // myOptionalClosure!() + (.exclamationMark, .period), // myOptionalBar!.foo() + (.extendedRegexDelimiter, .leftParen), // opening extended regex delimiter should never be separate by a space + (.extendedRegexDelimiter, .regexSlash), // opening extended regex delimiter should never be separate by a space + (.identifier, .leftAngle), // MyType + (.identifier, .leftParen), // foo() + (.identifier, .leftSquareBracket), // myArray[1] + (.identifier, .period), // a.b + (.integerLiteral, .period), // macOS 11.2.1 + (.keyword(.`init`), .leftAngle), // init() + (.keyword(.`init`), .leftParen), // init() + (.keyword(.self), .period), // self.someProperty + (.keyword(.Self), .period), // self.someProperty + (.keyword(.set), .leftParen), // var mYar: Int { set(value) {} } + (.keyword(.subscript), .leftParen), // subscript(x: Int) + (.keyword(.super), .period), // super.someProperty + (.leftBrace, _), + (.leftParen, _), + (.leftSquareBracket, _), + (.multilineStringQuote, .rawStringDelimiter), // closing raw string delimiter should never be separate by a space + (.period, _), + (.postfixQuestionMark, .leftAngle), // init?() + (.postfixQuestionMark, .leftParen), // init?() or myOptionalClosure?() + (.postfixQuestionMark, .period), // someOptional?.someProperty + (.pound, _), + (.poundUnavailableKeyword, .leftParen), // #unavailable(...) + (.prefixAmpersand, _), + (.prefixOperator, _), + (.rawStringDelimiter, .leftParen), // opening raw string delimiter should never be separate by a space + (.rawStringDelimiter, .multilineStringQuote), // opening raw string delimiter should never be separate by a space + (.rawStringDelimiter, .singleQuote), // opening raw string delimiter should never be separate by a space + (.rawStringDelimiter, .stringQuote), // opening raw string delimiter should never be separate by a space + (.regexLiteralPattern, _), + (.regexSlash, .extendedRegexDelimiter), // closing extended regex delimiter should never be separate by a space + (.rightAngle, .leftParen), // func foo(x: T) + (.rightParen, .leftParen), // returnsClosure()() + (.rightParen, .period), // foo().bar + (.rightSquareBracket, .period), // myArray[1].someProperty + (.singleQuote, .rawStringDelimiter), // closing raw string delimiter should never be separate by a space + (.stringQuote, .rawStringDelimiter), // closing raw string delimiter should never be separate by a space + (.stringSegment, _), + (_, .colon), + (_, .comma), + (_, .ellipsis), + (_, .eof), + (_, .exclamationMark), + (_, .postfixOperator), + (_, .postfixQuestionMark), + (_, .rightBrace), + (_, .rightParen), + (_, .rightSquareBracket), + (_, .semicolon), + (_, nil), + (nil, _): + return false + case (.leftAngle, _) where second?.tokenKind != .rightAngle: // `<` and `>` need to be separated by a space because otherwise they become an operator + return false + case (_, .rightAngle) where first?.tokenKind != .leftAngle: // `<` and `>` need to be separated by a space because otherwise they become an operator return false default: break } - if first?.requiresTrailingSpace ?? false { - return true - } - if second?.requiresLeadingSpace ?? false { - return true + switch first?.keyPathInParent { + case \ExpressionSegmentSyntax.backslash, + \ExpressionSegmentSyntax.rightParen, + \DeclNameArgumentSyntax.colon, + \StringLiteralExprSyntax.openQuote, + \RegexLiteralExprSyntax.openSlash: + return false + default: + break } - return false + + return true + } + + /// Whether the formatter should consider this token as being mutable. + /// This allows the diagnostic generator to only assume that missing nodes + /// will be mutated. Thus, if two tokens need to be separated by a space, it + /// will not be assumed that the space is added to an immutable previous node. + open func isMutable(_ token: TokenSyntax) -> Bool { + return true } // MARK: - Formatting a token @@ -191,15 +272,15 @@ open class BasicFormat: SyntaxRewriter { defer { self.previousToken = token } - let previousToken = self.previousToken ?? token.previousToken(viewMode: .sourceAccurate) - let nextToken = token.nextToken(viewMode: .sourceAccurate) + let previousToken = self.previousToken ?? token.previousToken(viewMode: viewMode) + let nextToken = token.nextToken(viewMode: viewMode) lazy var previousTokenWillEndWithWhitespace: Bool = { guard let previousToken = previousToken else { return false } return previousToken.trailingTrivia.endsWithWhitespace - || requiresWhitespace(between: previousToken, and: token) + || (requiresWhitespace(between: previousToken, and: token) && isMutable(previousToken)) }() lazy var previousTokenWillEndWithNewline: Bool = { @@ -234,7 +315,7 @@ open class BasicFormat: SyntaxRewriter { return false } return nextToken.leadingTrivia.startsWithNewline - || requiresLeadingNewline(nextToken) + || (requiresLeadingNewline(nextToken) && isMutable(nextToken)) }() /// This token's trailing trivia + any spaces or tabs at the start of the diff --git a/Sources/SwiftBasicFormat/generated/BasicFormat+Extensions.swift b/Sources/SwiftBasicFormat/generated/BasicFormat+Extensions.swift index ac2a48fcfd0..2e07341afc0 100644 --- a/Sources/SwiftBasicFormat/generated/BasicFormat+Extensions.swift +++ b/Sources/SwiftBasicFormat/generated/BasicFormat+Extensions.swift @@ -30,154 +30,6 @@ public extension TokenSyntax { } return false } - - var requiresLeadingSpace: Bool { - if let keyPath = keyPathInParent, let requiresLeadingSpace = keyPath.requiresLeadingSpace { - return requiresLeadingSpace - } - switch tokenKind { - case .arrow: - return true - case .binaryOperator: - return true - case .equal: - return true - case .leftBrace: - return true - case .keyword(.`catch`): - return true - case .keyword(.`else`): - return true - case .keyword(.`in`): - return true - case .keyword(.`where`): - return true - default: - return false - } - } - - var requiresTrailingSpace: Bool { - if let keyPath = keyPathInParent, let requiresTrailingSpace = keyPath.requiresTrailingSpace { - return requiresTrailingSpace - } - switch tokenKind { - case .arrow: - return true - case .binaryOperator: - return true - case .colon: - return true - case .comma: - return true - case .equal: - return true - case .poundAvailableKeyword: - return true - case .poundElseKeyword: - return true - case .poundElseifKeyword: - return true - case .poundEndifKeyword: - return true - case .poundIfKeyword: - return true - case .poundSourceLocationKeyword: - return true - case .poundUnavailableKeyword: - return true - case .keyword(.`Any`): - return true - case .keyword(.`as`): - return true - case .keyword(.`associatedtype`): - return true - case .keyword(.async): - return true - case .keyword(.await): - return true - case .keyword(.`break`): - return true - case .keyword(.`case`): - return true - case .keyword(.`class`): - return true - case .keyword(.`continue`): - return true - case .keyword(.`defer`): - return true - case .keyword(.`else`): - return true - case .keyword(.`enum`): - return true - case .keyword(.`extension`): - return true - case .keyword(.`fallthrough`): - return true - case .keyword(.`fileprivate`): - return true - case .keyword(.`for`): - return true - case .keyword(.`func`): - return true - case .keyword(.`guard`): - return true - case .keyword(.`if`): - return true - case .keyword(.`import`): - return true - case .keyword(.`in`): - return true - case .keyword(.`inout`): - return true - case .keyword(.`internal`): - return true - case .keyword(.`is`): - return true - case .keyword(.`let`): - return true - case .keyword(.`operator`): - return true - case .keyword(.`precedencegroup`): - return true - case .keyword(.`private`): - return true - case .keyword(.`protocol`): - return true - case .keyword(.`public`): - return true - case .keyword(.`repeat`): - return true - case .keyword(.`rethrows`): - return true - case .keyword(.`return`): - return true - case .keyword(.`static`): - return true - case .keyword(.`struct`): - return true - case .keyword(.`subscript`): - return true - case .keyword(.`switch`): - return true - case .keyword(.`throw`): - return true - case .keyword(.`throws`): - return true - case .keyword(.`try`): - return true - case .keyword(.`typealias`): - return true - case .keyword(.`var`): - return true - case .keyword(.`where`): - return true - case .keyword(.`while`): - return true - default: - return false - } - } } fileprivate extension AnyKeyPath { diff --git a/Sources/SwiftParserDiagnostics/DiagnosticExtensions.swift b/Sources/SwiftParserDiagnostics/DiagnosticExtensions.swift index 2a2d7d35b82..9ce0838f598 100644 --- a/Sources/SwiftParserDiagnostics/DiagnosticExtensions.swift +++ b/Sources/SwiftParserDiagnostics/DiagnosticExtensions.swift @@ -59,6 +59,8 @@ extension FixIt { } } +// MARK: - Make missing + extension FixIt.MultiNodeChange { /// Replaced a present token with a missing node. /// If `transferTrivia` is `true`, the leading and trailing trivia of the @@ -98,6 +100,37 @@ extension FixIt.MultiNodeChange { return FixIt.MultiNodeChange(primitiveChanges: changes) } + /// Transfers the leading and trailing trivia of `nodes` to the previous token + /// While doing this, it tries to be smart, merging trivia where it makes sense + /// and refusing to add e.g. a space after punctuation, where it usually + /// doesn't make sense. + private static func transferTriviaAtSides(from nodes: [SyntaxType]) -> Self { + let removedTriviaAtSides = (nodes.first?.leadingTrivia ?? []).merging(nodes.last?.trailingTrivia ?? []) + if !removedTriviaAtSides.isEmpty, let previousToken = nodes.first?.previousToken(viewMode: .sourceAccurate) { + let mergedTrivia = previousToken.trailingTrivia.merging(removedTriviaAtSides) + if previousToken.tokenKind.isPunctuation, mergedTrivia.allSatisfy({ $0.isSpaceOrTab }) { + // Punctuation is generally not followed by spaces in Swift. + // If this action would only add spaces to the punctuation, drop it. + // This generally yields better results. + return FixIt.MultiNodeChange() + } + return FixIt.MultiNodeChange(.replaceTrailingTrivia(token: previousToken, newTrivia: mergedTrivia)) + } else { + return FixIt.MultiNodeChange() + } + } +} + +// MARK: - Make present + +class MissingNodesBasicFormatter: BasicFormat { + override func isMutable(_ token: TokenSyntax) -> Bool { + // Assume that all missing nodes will be made present by the Fix-It. + return token.presence == .missing + } +} + +extension FixIt.MultiNodeChange { /// Make a node present. If `leadingTrivia` or `trailingTrivia` is specified, /// override the default leading/trailing trivia inferred from `BasicFormat`. static func makePresent( @@ -105,93 +138,53 @@ extension FixIt.MultiNodeChange { leadingTrivia: Trivia? = nil, trailingTrivia: Trivia? = nil ) -> Self { - var presentNode = PresentMaker().visit(Syntax(node)) + var presentNode = MissingNodesBasicFormatter(viewMode: .fixedUp).visit(Syntax(node)) + presentNode = PresentMaker().rewrite(presentNode) + if let leadingTrivia = leadingTrivia { presentNode = presentNode.with(\.leadingTrivia, leadingTrivia) } if let trailingTrivia = trailingTrivia { presentNode = presentNode.with(\.trailingTrivia, trailingTrivia) } + + var changes: [FixIt.Change] = [] + if node.shouldBeInsertedAfterNextTokenTrivia, let nextToken = node.nextToken(viewMode: .sourceAccurate), leadingTrivia == nil { - return FixIt.MultiNodeChange( - .replace( - oldNode: Syntax(node), - newNode: Syntax(presentNode).with(\.leadingTrivia, nextToken.leadingTrivia) - ), - .replaceLeadingTrivia(token: nextToken, newTrivia: []) - ) - } else if node.leadingTrivia.isEmpty, - let previousToken = node.previousToken(viewMode: .fixedUp), - previousToken.presence == .present, - previousToken.trailingTrivia.isEmpty, - BasicFormat().requiresWhitespace(between: previousToken, and: node.firstToken(viewMode: .fixedUp)), - leadingTrivia == nil - { - /// If neither this nor the previous token are punctionation make sure they - /// are separated by a space. - return FixIt.MultiNodeChange( - .replace( - oldNode: Syntax(node), - newNode: Syntax(presentNode).with(\.leadingTrivia, .space) - ) - ) - } else { - return FixIt.MultiNodeChange( - .replace( - oldNode: Syntax(node), - newNode: Syntax(presentNode) - ) - ) - } - } + // Move the next token's leading trivia to this node's leading trivia. + changes.append(.replaceLeadingTrivia(token: nextToken, newTrivia: [])) + presentNode = presentNode.with(\.leadingTrivia, nextToken.leadingTrivia) - /// Makes the `token` present, moving it in front of the previous token's trivia. - static func makePresentBeforeTrivia(_ token: TokenSyntax) -> Self { - if let previousToken = token.previousToken(viewMode: .sourceAccurate) { - var presentToken = PresentMaker().visit(token) - if !previousToken.trailingTrivia.isEmpty { - presentToken = presentToken.with(\.trailingTrivia, previousToken.trailingTrivia) + // If this node and the next token need to be separated, insert a space. + if let lastToken = node.lastToken(viewMode: .all), + lastToken.trailingTrivia.isEmpty, + BasicFormat().requiresWhitespace(between: lastToken, and: nextToken) + { + presentNode = presentNode.with(\.trailingTrivia, .space) } - return FixIt.MultiNodeChange( - .replaceTrailingTrivia(token: previousToken, newTrivia: []), - .replace(oldNode: Syntax(token), newNode: Syntax(presentToken)) - ) - } else { - return .makePresent(token) } - } - /// Transfers the leading and trailing trivia of `nodes` to the previous token - /// While doing this, it tries to be smart, merging trivia where it makes sense - /// and refusing to add e.g. a space after punctuation, where it usually - /// doesn't make sense. - static func transferTriviaAtSides(from nodes: [SyntaxType]) -> Self { - let removedTriviaAtSides = (nodes.first?.leadingTrivia ?? []).merging(nodes.last?.trailingTrivia ?? []) - if !removedTriviaAtSides.isEmpty, let previousToken = nodes.first?.previousToken(viewMode: .sourceAccurate) { - let mergedTrivia = previousToken.trailingTrivia.merging(removedTriviaAtSides) - if previousToken.tokenKind.isPunctuation, mergedTrivia.allSatisfy({ $0.isSpaceOrTab }) { - // Punctuation is generally not followed by spaces in Swift. - // If this action would only add spaces to the punctuation, drop it. - // This generally yields better results. - return FixIt.MultiNodeChange() - } - return FixIt.MultiNodeChange(.replaceTrailingTrivia(token: previousToken, newTrivia: mergedTrivia)) - } else { - return FixIt.MultiNodeChange() + if let previousToken = node.previousToken(viewMode: .all), + previousToken.presence == .present, + let firstToken = node.firstToken(viewMode: .all), + previousToken.trailingTrivia.allSatisfy({ $0.isWhitespace }), + !BasicFormat().requiresWhitespace(between: previousToken, and: firstToken) + { + // If the previous token and this node don't need to be separated, remove + // the separation. + changes.append(.replaceTrailingTrivia(token: previousToken, newTrivia: [])) } - } -} -extension TriviaPiece { - var isSpaceOrTab: Bool { - switch self { - case .spaces, .tabs: - return true - default: - return false - } + changes.append( + .replace( + oldNode: Syntax(node), + newNode: Syntax(presentNode) + ) + ) + + return FixIt.MultiNodeChange(primitiveChanges: changes) } } diff --git a/Sources/SwiftParserDiagnostics/MissingNodesError.swift b/Sources/SwiftParserDiagnostics/MissingNodesError.swift index f4d8e55d2a5..7135b404d47 100644 --- a/Sources/SwiftParserDiagnostics/MissingNodesError.swift +++ b/Sources/SwiftParserDiagnostics/MissingNodesError.swift @@ -21,6 +21,12 @@ fileprivate func findCommonAncestor(_ nodes: [Syntax]) -> Syntax? { return findCommonAncestorOrSelf(nodes.compactMap({ $0.parent })) } +class NoNewlinesFormat: BasicFormat { + override func requiresLeadingNewline(_ token: TokenSyntax) -> Bool { + return false + } +} + fileprivate enum NodesDescriptionPart { case tokensWithDefaultText([TokenSyntax]) case tokenWithoutDefaultText(TokenSyntax) @@ -30,7 +36,7 @@ fileprivate enum NodesDescriptionPart { switch self { case .tokensWithDefaultText(var tokens): if format { - tokens = tokens.map({ BasicFormat().visit($0) }) + tokens = tokens.map({ NoNewlinesFormat(viewMode: .all).visit($0) }) } if !tokens.isEmpty { tokens[0] = tokens[0].with(\.leadingTrivia, []) @@ -70,11 +76,11 @@ fileprivate enum NodesDescriptionPart { } else { let (rawKind, text) = token.tokenKind.decomposeToRaw() if let text = text, !text.isEmpty { - let presentToken = TokenSyntax(token.tokenKind, presence: .present) + let presentToken = token.with(\.presence, .present) newPart = .tokensWithDefaultText([presentToken]) } else if let defaultText = rawKind.defaultText { let newKind = TokenKind.fromRaw(kind: rawKind, text: String(syntaxText: defaultText)) - let presentToken = TokenSyntax(newKind, presence: .present) + let presentToken = token.with(\.tokenKind, newKind).with(\.presence, .present) newPart = .tokensWithDefaultText([presentToken]) } else { newPart = .tokenWithoutDefaultText(token) @@ -367,21 +373,7 @@ extension ParseDiagnosticsGenerator { } } - let changes = missingNodes.enumerated().map { (index, missingNode) -> FixIt.MultiNodeChange in - if index == 0, - let token = missingNode.as(TokenSyntax.self), - let previousTokenKind = missingNode.previousToken(viewMode: .sourceAccurate)?.tokenKind - { - if token.tokenKind.isPunctuation && !previousTokenKind.isPunctuation { - // Don't want whitespace before punctuation - return .makePresentBeforeTrivia(token) - } else if (token.tokenKind.isIdentifier || token.tokenKind.isDollarIdentifier) && previousTokenKind.isPunctuation { - // Don't want whitespace after punctuation where the following token is an identifier - return .makePresentBeforeTrivia(token) - } - } - return .makePresent(missingNode) - } + let changes = missingNodes.map { FixIt.MultiNodeChange.makePresent($0) } let fixIt = FixIt( message: InsertTokenFixIt(missingNodes: missingNodes), changes: additionalChanges + changes diff --git a/Sources/SwiftParserDiagnostics/MissingTokenError.swift b/Sources/SwiftParserDiagnostics/MissingTokenError.swift index 24b84119710..a50f57fe83a 100644 --- a/Sources/SwiftParserDiagnostics/MissingTokenError.swift +++ b/Sources/SwiftParserDiagnostics/MissingTokenError.swift @@ -109,11 +109,16 @@ extension ParseDiagnosticsGenerator { // The extraneous whitespace caused a missing identifier, output a // diagnostic inserting it instead of a diagnostic to fix the trivia // around the period. - _ = handleMissingSyntax( + let fixIt = FixIt( + message: InsertTokenFixIt(missingNodes: [Syntax(identifier)]), + changes: changes + [.makePresent(identifier, trailingTrivia: invalidToken.trailingTrivia)] + ) + addDiagnostic( identifier, - overridePosition: invalidToken.endPositionBeforeTrailingTrivia, - additionalChanges: changes, - additionalHandledNodes: [invalidTokenContainer.id] + position: invalidToken.endPositionBeforeTrailingTrivia, + MissingNodesError(missingNodes: [Syntax(identifier)]), + fixIts: [fixIt], + handledNodes: [invalidTokenContainer.id, identifier.id] ) } else { let fixIt = FixIt(message: .removeExtraneousWhitespace, changes: changes) @@ -143,7 +148,7 @@ extension ParseDiagnosticsGenerator { message: isTooMany ? .removeExtraneousDelimiters : .insertExtraClosingPounds, changes: [ .makeMissing(invalidToken), - .makePresentBeforeTrivia(missingToken), + .makePresent(missingToken), ] ) addDiagnostic( diff --git a/Sources/SwiftParserDiagnostics/ParseDiagnosticsGenerator.swift b/Sources/SwiftParserDiagnostics/ParseDiagnosticsGenerator.swift index a73e8b1cee2..e1655eccea7 100644 --- a/Sources/SwiftParserDiagnostics/ParseDiagnosticsGenerator.swift +++ b/Sources/SwiftParserDiagnostics/ParseDiagnosticsGenerator.swift @@ -564,7 +564,7 @@ public class ParseDiagnosticsGenerator: SyntaxAnyVisitor { position: position, .consecutiveStatementsOnSameLine, fixIts: [ - FixIt(message: .insertSemicolon, changes: .makePresentBeforeTrivia(semicolon)) + FixIt(message: .insertSemicolon, changes: .makePresent(semicolon)) ], handledNodes: [semicolon.id] ) @@ -941,7 +941,7 @@ public class ParseDiagnosticsGenerator: SyntaxAnyVisitor { position: position, .consecutiveDeclarationsOnSameLine, fixIts: [ - FixIt(message: .insertSemicolon, changes: .makePresentBeforeTrivia(semicolon)) + FixIt(message: .insertSemicolon, changes: .makePresent(semicolon)) ], handledNodes: [semicolon.id] ) diff --git a/Sources/SwiftParserDiagnostics/PresenceUtils.swift b/Sources/SwiftParserDiagnostics/PresenceUtils.swift index 654ca8d0973..71c083cbb63 100644 --- a/Sources/SwiftParserDiagnostics/PresenceUtils.swift +++ b/Sources/SwiftParserDiagnostics/PresenceUtils.swift @@ -50,12 +50,12 @@ class PresentMaker: SyntaxRewriter { let presentToken: TokenSyntax let (rawKind, text) = token.tokenKind.decomposeToRaw() if let text = text, (!text.isEmpty || rawKind == .stringSegment) { // string segments can have empty text - presentToken = TokenSyntax(token.tokenKind, presence: .present) + presentToken = token.with(\.presence, .present) } else { let newKind = TokenKind.fromRaw(kind: rawKind, text: rawKind.defaultText.map(String.init) ?? "<#\(token.tokenKind.nameForDiagnostics)#>") - presentToken = TokenSyntax(newKind, leadingTrivia: token.leadingTrivia, trailingTrivia: token.trailingTrivia, presence: .present) + presentToken = token.with(\.tokenKind, newKind).with(\.presence, .present) } - return BasicFormat().visit(presentToken) + return presentToken } else { return token } diff --git a/Tests/SwiftBasicFormatTest/BasicFormatTests.swift b/Tests/SwiftBasicFormatTest/BasicFormatTests.swift index 6ce1b0a3a80..58a556c0917 100644 --- a/Tests/SwiftBasicFormatTest/BasicFormatTests.swift +++ b/Tests/SwiftBasicFormatTest/BasicFormatTests.swift @@ -190,4 +190,71 @@ final class BasicFormatTest: XCTestCase { """ ) } + + func testStringInterpolationAtStartOfMultiLineStringLiteral() { + assertFormatted( + source: #""" + class Foo { + func test() { + assertionFailure(""" + ABC + \(myVar) + """) + } + } + """#, + expected: #""" + class Foo { + func test() { + assertionFailure(""" + ABC + \(myVar) + """) + } + } + """# + ) + } + + func testMultilineStringLiteral() { + assertFormatted( + source: #""" + assertionFailure(""" + First line + Second line + """) + """#, + expected: #""" + assertionFailure(""" + First line + Second line + """) + """# + ) + } + + func testNestedUserDefinedIndentation() { + assertFormatted( + source: """ + class X { + func test() { + withTrailingClosure() { + if true { return } + } + } + } + """, + expected: """ + class X { + func test() { + withTrailingClosure() { + if true { + return + } + } + } + } + """ + ) + } } diff --git a/Tests/SwiftParserTest/AttributeTests.swift b/Tests/SwiftParserTest/AttributeTests.swift index ddea7673132..bdd0c6abf13 100644 --- a/Tests/SwiftParserTest/AttributeTests.swift +++ b/Tests/SwiftParserTest/AttributeTests.swift @@ -27,7 +27,7 @@ final class AttributeTests: XCTestCase { DiagnosticSpec(message: "expected ')' to end attribute", fixIts: ["insert ')'"]), ], fixedSource: """ - @_dynamicReplacement(for : <#identifier#>) + @_dynamicReplacement(for: <#identifier#>) func test_dynamic_replacement_for2() { } """ @@ -311,7 +311,7 @@ final class AttributeTests: XCTestCase { "@resultBuilder1️⃣", diagnostics: [DiagnosticSpec(message: "expected declaration after attribute", fixIts: ["insert declaration"])], fixedSource: """ - @resultBuilder<#declaration#> + @resultBuilder <#declaration#> """ ) } diff --git a/Tests/SwiftParserTest/DeclarationTests.swift b/Tests/SwiftParserTest/DeclarationTests.swift index 6e49712a253..e8062c54128 100644 --- a/Tests/SwiftParserTest/DeclarationTests.swift +++ b/Tests/SwiftParserTest/DeclarationTests.swift @@ -1005,7 +1005,8 @@ final class DeclarationTests: XCTestCase { DiagnosticSpec(locationMarker: "2️⃣", message: "expected member block in class", fixIts: ["insert member block"]), ], fixedSource: """ - }class C {} + }class C { + } """ ) assertParse( diff --git a/Tests/SwiftParserTest/VariadicGenericsTests.swift b/Tests/SwiftParserTest/VariadicGenericsTests.swift index 0ad82e72509..85124384aae 100644 --- a/Tests/SwiftParserTest/VariadicGenericsTests.swift +++ b/Tests/SwiftParserTest/VariadicGenericsTests.swift @@ -616,7 +616,7 @@ final class TypeParameterPackTests: XCTestCase { DiagnosticSpec(message: "expected ',' in tuple type", fixIts: ["insert ','"]) ], fixedSource: """ - var foo: (Array , Array) + var foo: (Array, Array) """ ) @@ -640,7 +640,7 @@ final class TypeParameterPackTests: XCTestCase { DiagnosticSpec(message: "expected ',' in tuple type", fixIts: ["insert ','"]) ], fixedSource: """ - var foo: (Array , a) + var foo: (Array, a) """ ) } diff --git a/Tests/SwiftParserTest/translated/AvailabilityQueryUnavailabilityTests.swift b/Tests/SwiftParserTest/translated/AvailabilityQueryUnavailabilityTests.swift index e698196fbe0..86587ee3fb3 100644 --- a/Tests/SwiftParserTest/translated/AvailabilityQueryUnavailabilityTests.swift +++ b/Tests/SwiftParserTest/translated/AvailabilityQueryUnavailabilityTests.swift @@ -93,7 +93,7 @@ final class AvailabilityQueryUnavailabilityTests: XCTestCase { DiagnosticSpec(message: "expected '(', '@availability' arguments, and ')' in availability condition", fixIts: ["insert '(', '@availability' arguments, and ')'"]) ], fixedSource: """ - if #unavailable( <#identifier#>){ + if #unavailable(<#identifier#>) { } """ ) @@ -109,7 +109,7 @@ final class AvailabilityQueryUnavailabilityTests: XCTestCase { DiagnosticSpec(message: "expected platform and ')' to end availability condition", fixIts: ["insert platform and ')'"]) ], fixedSource: """ - if #unavailable(<#identifier#> ){ + if #unavailable(<#identifier#>) { } """ ) @@ -323,7 +323,7 @@ final class AvailabilityQueryUnavailabilityTests: XCTestCase { DiagnosticSpec(message: "expected ')' to end availability condition", fixIts: ["insert ')'"]), ], fixedSource: """ - if #unavailable(OSX 10.51,<#identifier#> ){ + if #unavailable(OSX 10.51, <#identifier#>) { } """ ) @@ -339,7 +339,7 @@ final class AvailabilityQueryUnavailabilityTests: XCTestCase { DiagnosticSpec(message: "expected version restriction in availability argument", fixIts: ["insert version restriction"]) ], fixedSource: """ - if #unavailable(OSX 10.51,<#identifier#>) { + if #unavailable(OSX 10.51, <#identifier#>) { } """ ) diff --git a/Tests/SwiftParserTest/translated/DiagnoseDynamicReplacementTests.swift b/Tests/SwiftParserTest/translated/DiagnoseDynamicReplacementTests.swift index 141c72855b2..51d1d93d819 100644 --- a/Tests/SwiftParserTest/translated/DiagnoseDynamicReplacementTests.swift +++ b/Tests/SwiftParserTest/translated/DiagnoseDynamicReplacementTests.swift @@ -68,7 +68,7 @@ final class DiagnoseDynamicReplacementTests: XCTestCase { ) ], fixedSource: """ - @_dynamicReplacement(for: dynamically_replaceable() ) + @_dynamicReplacement(for: dynamically_replaceable()) func test_dynamic_replacement_for3() { } """ diff --git a/Tests/SwiftParserTest/translated/InvalidTests.swift b/Tests/SwiftParserTest/translated/InvalidTests.swift index 5d59ed45922..4dc17123de2 100644 --- a/Tests/SwiftParserTest/translated/InvalidTests.swift +++ b/Tests/SwiftParserTest/translated/InvalidTests.swift @@ -482,7 +482,7 @@ final class InvalidTests: XCTestCase { ], fixedSource: """ func were( - wolf: () ){} + wolf: ()) {} """ ) } diff --git a/Tests/SwiftParserTest/translated/RecoveryTests.swift b/Tests/SwiftParserTest/translated/RecoveryTests.swift index ca8872a0277..46475d01ae8 100644 --- a/Tests/SwiftParserTest/translated/RecoveryTests.swift +++ b/Tests/SwiftParserTest/translated/RecoveryTests.swift @@ -2317,7 +2317,7 @@ final class RecoveryTests: XCTestCase { ) ], fixedSource: """ - func f<<#identifier#> >() {} + func f<<#identifier#>>() {} """ ) diff --git a/Tests/SwiftParserTest/translated/SubscriptingTests.swift b/Tests/SwiftParserTest/translated/SubscriptingTests.swift index 2b7b1c67d7f..ea420323e94 100644 --- a/Tests/SwiftParserTest/translated/SubscriptingTests.swift +++ b/Tests/SwiftParserTest/translated/SubscriptingTests.swift @@ -210,7 +210,7 @@ final class SubscriptingTests: XCTestCase { struct A0 { subscript( i : (Int) - -> Int) -> <#type#>{ + -> Int) -> <#type#> { get { return stored } @@ -460,7 +460,7 @@ final class SubscriptingTests: XCTestCase { ], fixedSource: """ struct A11 { - subscript( x y : (Int) -> Int) -> <#type#>{ + subscript(x y : (Int) -> Int) -> <#type#> { return 0 } } diff --git a/Tests/SwiftParserTest/translated/ToplevelLibraryTests.swift b/Tests/SwiftParserTest/translated/ToplevelLibraryTests.swift index e24d70fa8c9..e6a32bf87e0 100644 --- a/Tests/SwiftParserTest/translated/ToplevelLibraryTests.swift +++ b/Tests/SwiftParserTest/translated/ToplevelLibraryTests.swift @@ -46,7 +46,8 @@ final class ToplevelLibraryTests: XCTestCase { DiagnosticSpec(message: "expected 'in', expression, and body in 'for' statement", fixIts: ["insert 'in', expression, and body"]) ], fixedSource: """ - for i in <#expression#> {} + for i in <#expression#> { + } """ ) } From cf70c5b46d81253da23ed85ae2128b8868a4d90e Mon Sep 17 00:00:00 2001 From: Alex Hoppen Date: Fri, 21 Apr 2023 13:45:25 -0700 Subject: [PATCH 2/2] Assert that for each `assertParse` test without errors, we produce the same tree by formatting it using BasicFormat --- .../Syntax+Assertions.swift | 14 ++++- Tests/SwiftParserTest/Assertions.swift | 55 +++++++++++++++++++ 2 files changed, 68 insertions(+), 1 deletion(-) diff --git a/Sources/_SwiftSyntaxTestSupport/Syntax+Assertions.swift b/Sources/_SwiftSyntaxTestSupport/Syntax+Assertions.swift index d5e9c59608e..c11f3310656 100644 --- a/Sources/_SwiftSyntaxTestSupport/Syntax+Assertions.swift +++ b/Sources/_SwiftSyntaxTestSupport/Syntax+Assertions.swift @@ -115,11 +115,23 @@ public struct SubtreeMatcher { afterMarker: String? = nil, _ expected: Syntax, includeTrivia: Bool = false, + additionalInfo: String? = nil, file: StaticString = #filePath, line: UInt = #line ) throws { if let diff = try findFirstDifference(afterMarker: afterMarker, baseline: expected, includeTrivia: includeTrivia) { - XCTFail(diff.debugDescription, file: file, line: line) + let message: String + if let additionalInfo = additionalInfo { + message = """ + \(additionalInfo) + + \(diff.debugDescription) + """ + } else { + message = diff.debugDescription + } + + XCTFail(message, file: file, line: line) } } } diff --git a/Tests/SwiftParserTest/Assertions.swift b/Tests/SwiftParserTest/Assertions.swift index 888f61c7386..6a6824847a6 100644 --- a/Tests/SwiftParserTest/Assertions.swift +++ b/Tests/SwiftParserTest/Assertions.swift @@ -625,6 +625,10 @@ func assertParse( ) } + if expectedDiagnostics.isEmpty { + assertBasicFormat(source: source, parse: parse, file: file, line: line) + } + #if SWIFTPARSER_ENABLE_ALTERNATE_TOKEN_INTROSPECTION if enableTestCaseMutation { let mutations: [(offset: Int, replacement: TokenSpec)] = parser.alternativeTokenChoices.flatMap { offset, replacements in @@ -659,3 +663,54 @@ func assertParse( } #endif } + +class TriviaRemover: SyntaxRewriter { + override func visit(_ token: TokenSyntax) -> TokenSyntax { + var ancestor = Syntax(token) + while let parent = ancestor.parent { + ancestor = parent + if ancestor.is(StringLiteralExprSyntax.self) || ancestor.is(RegexLiteralExprSyntax.self) { + // Don't mess with indentation inside string or regex literals. + // BasicFormat doesn't know where to re-apply newlines and how much to indent the string literal contents. + return token + } + } + if token.parent?.is(StringSegmentSyntax.self) ?? false { + return token + } + return token.with(\.leadingTrivia, []).with(\.trailingTrivia, []) + } +} + +func assertBasicFormat( + source: String, + parse: (inout Parser) -> S, + file: StaticString = #file, + line: UInt = #line +) { + var parser = Parser(source) + let sourceTree = Syntax(parse(&parser)) + let withoutTrivia = TriviaRemover().visit(sourceTree) + let formatted = withoutTrivia.formatted() + + var formattedParser = Parser(formatted.description) + let formattedReparsed = Syntax(parse(&formattedParser)) + + do { + let subtreeMatcher = SubtreeMatcher(Syntax(formattedReparsed), markers: [:]) + try subtreeMatcher.assertSameStructure( + Syntax(sourceTree), + includeTrivia: false, + additionalInfo: """ + Removing trivia, formatting using BasicFormat and re-parsing did not produce the same syntax tree. + + Formatted source: + \(formatted) + """, + file: file, + line: line + ) + } catch { + XCTFail("Matching for a subtree failed with error: \(error)", file: file, line: line) + } +}