BasicFormat should not format unexpected nodes

`BasicFormat` depends on the parent layout nodes to determine formatting
in some cases, so it doesn't make sense to visit unexpected nodes.

Also fix up `SyntaxRewriter` to take a `viewMode`, since as it was
`BasicFormat` was actually visiting `missing` nodes, which also doesn't
make much sense.

Resolves rdar://110463876.

Author: bnbarham

CodeGeneration/Sources/Utils/CodeGenerationFormat.swift

@@ -108,7 +108,7 @@ public class CodeGenerationFormat: BasicFormat {
   private func formatChildrenSeparatedByNewline<SyntaxType: SyntaxProtocol>(children: SyntaxChildren, elementType: SyntaxType.Type) -> [SyntaxType] {
     increaseIndentationLevel()
     var formattedChildren = children.map {
-      self.visit($0).as(SyntaxType.self)!
+      self.rewrite($0.cast(SyntaxType.self)).cast(SyntaxType.self)
     }
     formattedChildren = formattedChildren.map {
       if $0.leadingTrivia.first?.isNewline == true {

CodeGeneration/Sources/generate-swiftsyntax/templates/swiftsyntax/SyntaxRewriterFile.swift

@@ -30,33 +30,28 @@ let syntaxRewriterFile = SourceFileSyntax(leadingTrivia: copyrightHeader) {
     open class SyntaxRewriter
     """
   ) {
-    DeclSyntax("public init() {}")
+    DeclSyntax("public let viewMode: SyntaxTreeViewMode")
 
-    for node in SYNTAX_NODES where !node.kind.isBase {
-      if (node.base == .syntax || node.base == .syntaxCollection) && node.kind != .missing {
-        DeclSyntax(
-          """
-          /// Visit a ``\(node.kind.syntaxType)``.
-          ///   - Parameter node: the node that is being visited
-          ///   - Returns: the rewritten node
-          open func visit(_ node: \(node.kind.syntaxType)) -> \(node.kind.syntaxType) {
-            return Syntax(visitChildren(node)).cast(\(node.kind.syntaxType).self)
-          }
-          """
-        )
-      } else {
-        DeclSyntax(
-          """
-          /// Visit a ``\(node.kind.syntaxType)``.
-          ///   - Parameter node: the node that is being visited
-          ///   - Returns: the rewritten node
-          open func visit(_ node: \(node.kind.syntaxType)) -> \(raw: node.baseType.syntaxBaseName) {
-            return \(raw: node.baseType.syntaxBaseName)(visitChildren(node))
-          }
-          """
-        )
+    DeclSyntax(
+      """
+      public init(viewMode: SyntaxTreeViewMode = .sourceAccurate) {
+        self.viewMode = viewMode
       }
-    }
+      """
+    )
+
+    DeclSyntax(
+      """
+      /// Rewrite `node` and anchor, making sure that the rewritten node also has
+      /// a parent if `node` had one.
+      public func rewrite(_ node: some SyntaxProtocol) -> Syntax {
+        let rewritten = self.visit(node)
+        return withExtendedLifetime(rewritten) {
+          return Syntax(node.data.replacingSelf(rewritten.raw, rawNodeArena: rewritten.raw.arena, allocationArena: SyntaxArena()))
+        }
+      }
+      """
+    )
 
     DeclSyntax(
       """
@@ -105,6 +100,7 @@ let syntaxRewriterFile = SourceFileSyntax(leadingTrivia: copyrightHeader) {
       /// Visit any Syntax node.
       ///   - Parameter node: the node that is being visited
       ///   - Returns: the rewritten node
+      @available(*, deprecated, renamed: "rewrite(_:)")
       public func visit(_ node: Syntax) -> Syntax {
         return visit(node.data)
       }
@@ -114,11 +110,37 @@ let syntaxRewriterFile = SourceFileSyntax(leadingTrivia: copyrightHeader) {
     DeclSyntax(
       """
       public func visit<T: SyntaxChildChoices>(_ node: T) -> T {
-        return visit(Syntax(node)).cast(T.self)
+        return visit(node.data).cast(T.self)
       }
       """
     )
 
+    for node in SYNTAX_NODES where !node.kind.isBase {
+      if (node.base == .syntax || node.base == .syntaxCollection) && node.kind != .missing {
+        DeclSyntax(
+          """
+          /// Visit a ``\(node.kind.syntaxType)``.
+          ///   - Parameter node: the node that is being visited
+          ///   - Returns: the rewritten node
+          open func visit(_ node: \(node.kind.syntaxType)) -> \(node.kind.syntaxType) {
+            return Syntax(visitChildren(node)).cast(\(node.kind.syntaxType).self)
+          }
+          """
+        )
+      } else {
+        DeclSyntax(
+          """
+          /// Visit a ``\(node.kind.syntaxType)``.
+          ///   - Parameter node: the node that is being visited
+          ///   - Returns: the rewritten node
+          open func visit(_ node: \(node.kind.syntaxType)) -> \(raw: node.baseType.syntaxBaseName) {
+            return \(raw: node.baseType.syntaxBaseName)(visitChildren(node))
+          }
+          """
+        )
+      }
+    }
+
     for baseKind in SyntaxNodeKind.allCases where baseKind.isBase && baseKind != .syntax && baseKind != .syntaxCollection {
       DeclSyntax(
         """
@@ -258,42 +280,44 @@ let syntaxRewriterFile = SourceFileSyntax(leadingTrivia: copyrightHeader) {
         // initialize the new layout. Once we know that we have to rewrite the
         // layout, we need to collect all futher children, regardless of whether
         // they are rewritten or not.
-        
+
         // newLayout is nil until the first child node is rewritten and rewritten
         // nodes are being collected.
         var newLayout: ContiguousArray<RawSyntax?>?
-        
+
         // Rewritten children just to keep their 'SyntaxArena' alive until they are
         // wrapped with 'Syntax'
         var rewrittens: ContiguousArray<Syntax> = []
-        
+
         let syntaxNode = node._syntaxNode
-        
+
         // Incrementing i manually is faster than using .enumerated()
         var childIndex = 0
         for (raw, info) in RawSyntaxChildren(syntaxNode) {
           defer { childIndex += 1 }
-          guard let child = raw else {
-            // Node does not exist. If we are collecting rewritten nodes, we need to
-            // collect this one as well, otherwise we can ignore it.
+
+          guard let child = raw, viewMode.shouldTraverse(node: child) else {
+            // Node does not exist or should not be visited. If we are collecting
+            // rewritten nodes, we need to collect this one as well, otherwise we
+            // can ignore it.
             if newLayout != nil {
-              newLayout!.append(nil)
+              newLayout!.append(raw)
             }
             continue
           }
-          
+
           // Build the Syntax node to rewrite
           let absoluteRaw = AbsoluteRawSyntax(raw: child, info: info)
           let data = SyntaxData(absoluteRaw, parent: syntaxNode)
-          
+
           let rewritten = visit(data)
           if rewritten.data.nodeId != info.nodeId {
             // The node was rewritten, let's handle it
             if newLayout == nil {
               // We have not yet collected any previous rewritten nodes. Initialize
               // the new layout with the previous nodes of the parent. This is
               // possible, since we know they were not rewritten.
-              
+
               // The below implementation is based on Collection.map but directly
               // reserves enough capacity for the entire layout.
               newLayout = ContiguousArray<RawSyntax?>()
@@ -302,7 +326,7 @@ let syntaxRewriterFile = SourceFileSyntax(leadingTrivia: copyrightHeader) {
                 newLayout!.append(node.raw.layoutView!.children[j])
               }
             }
-            
+
             // Now that we know we have a new layout in which we collect rewritten
             // nodes, add it.
             rewrittens.append(rewritten)
@@ -315,13 +339,13 @@ let syntaxRewriterFile = SourceFileSyntax(leadingTrivia: copyrightHeader) {
             }
           }
         }
-        
+
         if let newLayout {
           // A child node was rewritten. Build the updated node.
-          
+
           // Sanity check, ensure the new children are the same length.
           precondition(newLayout.count == node.raw.layoutView!.children.count)
-          
+
           let arena = SyntaxArena()
           let newRaw = node.raw.layoutView!.replacingLayout(with: Array(newLayout), arena: arena)
           // 'withExtendedLifetime' to keep 'SyntaxArena's of them alive until here.
@@ -335,18 +359,5 @@ let syntaxRewriterFile = SourceFileSyntax(leadingTrivia: copyrightHeader) {
       }
       """
     )
-
-    DeclSyntax(
-      """
-      /// Rewrite `node` and anchor, making sure that the rewritten node also has
-      /// a parent if `node` had one.
-      public func rewrite(_ node: Syntax) -> Syntax {
-        let rewritten = self.visit(node)
-        return withExtendedLifetime(rewritten) {
-          return Syntax(node.data.replacingSelf(rewritten.raw, rawNodeArena: rewritten.raw.arena, allocationArena: SyntaxArena()))
-        }
-      }
-      """
-    )
   }
 }

EditorExtension/SwiftRefactorExtension/SourceEditorCommand.swift

@@ -36,6 +36,7 @@ final class SourceEditorCommand: NSObject, XCSourceEditorCommand {
 
       init(provider: any RefactoringProvider.Type) {
         self.provider = provider
+        super.init(viewMode: .sourceAccurate)
       }
 
       override func visitAny(_ node: Syntax) -> Syntax? {

Sources/SwiftBasicFormat/BasicFormat.swift

@@ -12,6 +12,14 @@
 
 import SwiftSyntax
 
+/// A rewriter that performs a "basic" format of the passed tree.
+///
+/// The base implementation is primarily aimed at adding whitespace where
+/// required such that re-parsing the tree's description results in the same
+/// tree. But it also makes an attempt at adding in formatting, eg. splitting
+/// lines where obvious and some basic indentation at nesting levels.
+///
+/// Any subclasses *must* return the same node type as given.
 open class BasicFormat: SyntaxRewriter {
   /// How much indentation should be added at a new indentation level.
   public let indentationWidth: Trivia
@@ -35,8 +43,6 @@ 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.
@@ -49,7 +55,7 @@ open class BasicFormat: SyntaxRewriter {
   ) {
     self.indentationWidth = indentationWidth
     self.indentationStack = [(indentation: initialIndentation, isUserDefined: false)]
-    self.viewMode = viewMode
+    super.init(viewMode: viewMode)
   }
 
   // MARK: - Updating indentation level
@@ -70,6 +76,12 @@ open class BasicFormat: SyntaxRewriter {
     indentationStack.removeLast()
   }
 
+  open override func visit(_ node: UnexpectedNodesSyntax) -> UnexpectedNodesSyntax {
+    // Do not perform any formatting on unexpected nodes, the result won't make any
+    // sense as we rely on layout nodes to know what formatting to perform.
+    return node
+  }
+
   open override func visitPre(_ node: Syntax) {
     if requiresIndent(node) {
       if let firstToken = node.firstToken(viewMode: viewMode),

Sources/SwiftBasicFormat/SyntaxProtocol+Formatted.swift

@@ -3,6 +3,6 @@ import SwiftSyntax
 public extension SyntaxProtocol {
   /// Build a syntax node from this `Buildable` and format it with the given format.
   func formatted(using format: BasicFormat = BasicFormat()) -> Syntax {
-    return format.visit(Syntax(self))
+    return format.rewrite(self)
   }
 }

Sources/SwiftOperators/OperatorTable+Folding.swift

@@ -475,6 +475,7 @@ extension OperatorTable {
     ) {
       self.opPrecedence = opPrecedence
       self.errorHandler = errorHandler
+      super.init(viewMode: .fixedUp)
     }
 
     override func visitAny(_ node: Syntax) -> Syntax? {
@@ -538,7 +539,7 @@ extension OperatorTable {
         opPrecedence: self,
         errorHandler: errorHandler
       )
-      let result = folder.visit(Syntax(node))
+      let result = folder.rewrite(node)
 
       // If the sequence folder encountered an error that caused the error
       // handler to throw, invoke the error handler again with the original

Sources/SwiftParserDiagnostics/DiagnosticExtensions.swift

@@ -78,7 +78,7 @@ extension FixIt.MultiNodeChange {
     guard let node = node else {
       return FixIt.MultiNodeChange(primitiveChanges: [])
     }
-    var changes = [FixIt.Change.replace(oldNode: Syntax(node), newNode: MissingMaker().visit(Syntax(node)))]
+    var changes = [FixIt.Change.replace(oldNode: Syntax(node), newNode: MissingMaker().rewrite(node))]
     if transferTrivia {
       changes += FixIt.MultiNodeChange.transferTriviaAtSides(from: [node]).primitiveChanges
     }
@@ -123,7 +123,7 @@ extension FixIt.MultiNodeChange {
     leadingTrivia: Trivia? = nil,
     trailingTrivia: Trivia? = nil
   ) -> Self {
-    var presentNode = MissingNodesBasicFormatter(viewMode: .fixedUp).visit(Syntax(node))
+    var presentNode = MissingNodesBasicFormatter(viewMode: .fixedUp).rewrite(node)
     presentNode = PresentMaker().rewrite(presentNode)
 
     if let leadingTrivia {

Sources/SwiftParserDiagnostics/PresenceUtils.swift

@@ -45,6 +45,10 @@ extension SyntaxProtocol {
 
 /// Transforms a syntax tree by making all missing tokens present.
 class PresentMaker: SyntaxRewriter {
+  init() {
+    super.init(viewMode: .fixedUp)
+  }
+
   override func visit(_ token: TokenSyntax) -> TokenSyntax {
     if token.isMissing {
       let presentToken: TokenSyntax
@@ -62,7 +66,12 @@ class PresentMaker: SyntaxRewriter {
   }
 }
 
+/// Transforms a syntax tree by making all present tokens missing.
 class MissingMaker: SyntaxRewriter {
+  init() {
+    super.init(viewMode: .sourceAccurate)
+  }
+
   override func visit(_ node: TokenSyntax) -> TokenSyntax {
     guard node.isPresent else {
       return node

Sources/SwiftRefactor/OpaqueParameterToGeneric.swift

@@ -125,7 +125,7 @@ public struct OpaqueParameterToGeneric: SyntaxRefactoringProvider {
     in params: ParameterClauseSyntax,
     augmenting genericParams: GenericParameterClauseSyntax?
   ) -> (ParameterClauseSyntax, GenericParameterClauseSyntax)? {
-    let rewriter = SomeParameterRewriter()
+    let rewriter = SomeParameterRewriter(viewMode: .sourceAccurate)
     let rewrittenParams = rewriter.visit(params.parameterList)
 
     if rewriter.rewrittenSomeParameters.isEmpty {