Change visit(TokenSyntax) function of SyntaxRewriter to return TokenSyntax

[ahoppen]

There should be no reason why a TokenSyntax should get rewritten to a different node kind and this allow us to remove a force unwrap in the diagnostics infrastructure.

[ahoppen]

@swift-ci Please test

[ahoppen]

@swift-ci Please test

[ahoppen]

swiftlang/swift-stress-tester#202

@swift-ci Please test

[ahoppen]

swiftlang/swift-stress-tester#202

@swift-ci Please test

[bnbarham]

I think this is for the best, but I do wonder if we should extend it to others as well. Ie. should rewriting a function decl be able to rewrite to a statement? Or should we return a decl syntax there?

[ahoppen]

I was wondering the same. For TokenSyntax I was pretty sure that there’s no valid use case to rewrite it to any other node. For all the others, I would like to make a survey of the current SyntaxRewriter users (most notable swift-format and SwiftLint), whether they are making use of this option.

[allevato]

SyntaxRewriter already restricts for many return types; for example, a rewriter for FunctionDeclSyntax can only return other DeclSyntaxes. I think that's the right behavior, and this improvement for TokenSyntax is a good one.

One situation that comes to mind where it's valid to replace a node with one of a different kind is at a statement level; a CodeBlockItem can have a Decl, Expr, or Stmt as its child so one could feasibly want to reach for a rewriter that swaps one out for something else. However, that would also allow for invalid replacements (e.g., replacing a Decl with a Stmt in a member list), so such a replacement is probably better done at the CodeBlockItem level by replacing the child instead of allowing it at the child itself.

[bnbarham]

SyntaxRewriter already restricts for many return types; for example, a rewriter for FunctionDeclSyntax can only return other DeclSyntaxes. I think that's the right behavior, and this improvement for TokenSyntax is a good one.

Ah. I was looking at the private implementation method rather than the public one 😅. I'd still say this applies for all the functions that currently return Syntax though.

One situation that comes to mind where it's valid to replace a node with one of a different kind is at a statement level; a CodeBlockItem can have a Decl, Expr, or Stmt as its child so one could feasibly want to reach for a rewriter that swaps one out for something else. However, that would also allow for invalid replacements (e.g., replacing a Decl with a Stmt in a member list), so such a replacement is probably better done at the CodeBlockItem level by replacing the child instead of allowing it at the child itself.

I think in that case I'd be fine with requiring clients to visit the parent instead, ie. I'd rather to default to not being able to rewrite to completely bogus trees but if someone reallllyy wants to...