Add a new visitor: SyntaxTransformVisitor.
#643
Conversation
| % end | ||
| % end | ||
|
|
||
| public func visit(_ data: SyntaxData) -> [ResultType] { |
There was a problem hiding this comment.
We don't want clients visiting SyntaxData directly.
There was a problem hiding this comment.
I agree, SyntaxTransformVisitor should be a class and this should be private like we do in all the other initializers.
There was a problem hiding this comment.
Why do we want this to be a class? I think this should just take a Syntax.
There was a problem hiding this comment.
I’m a little worried about this implementation because IIUC its default implementation discards any recursive transformations by default. My suggestion would be to have a visitAny method (similar to SyntaxAnyVisitor) that all the visit methods default to and have that visitAny method fatalError by default. This way you’ll trap at runtime rather than just dropping results.
Also, I’m not a huge fan of tying this to Array as the transformation result. What do you think about having two generic types: NodeTransformResult and TransformResult (feel free to come up with a better name) and then combining them using a func reduce(initialResult: TransformResult?, nextPartialResult: NodeTransformResult). That way you could also return strings testStringify instead of arrays of strings.
| % end | ||
| % end | ||
|
|
||
| public func visit(_ data: SyntaxData) -> [ResultType] { |
There was a problem hiding this comment.
I agree, SyntaxTransformVisitor should be a class and this should be private like we do in all the other initializers.
| } | ||
| } | ||
|
|
||
| public func visitChildren<SyntaxType: SyntaxProtocol>(_ node: SyntaxType) -> [ResultType] { |
There was a problem hiding this comment.
Similarly to visit(_ data: SyntaxData), this should be private.
There was a problem hiding this comment.
Why? It is useful to API consumers (see the FuncCounter test).
| @@ -141,3 +141,75 @@ open class SyntaxVisitor { | |||
| } | |||
| } | |||
| } | |||
|
|
|||
| public protocol SyntaxTransformVisitor { | |||
There was a problem hiding this comment.
Let’s put this in a different file. SyntaxRewriter also has its dedicated file.
I'm not sure what you mean by this. The default implementation will visit all the children and flatMap them together. Nothing should be lost...
As long as there can be N children, I think this implementation will be hard. For a generic implementation, the reducer will either have to drop something on the floor or be non-trivial. I like that with the array approach we can handle any input and pass it up to the client for processing/validation. If performance is a concern, maybe we can introduce a SingleTransformVisitor that returns a |
|
@swift-ci please test |
|
@swift-ci test |
1 similar comment
|
@swift-ci test |
zoecarver
force-pushed
the
transform-visitor
branch
from
36e5608 to
7c14dad
Compare
|
The |
|
@swift-ci please test |
| public func visit(_ data: ExprSyntax) -> ResultType { | ||
| switch data.raw.kind { | ||
| % for node in NON_BASE_SYNTAX_NODES: | ||
| % if node.base_kind == "Expr": | ||
| case .${node.swift_syntax_kind}: | ||
| let node = data.as(${node.name}.self)! | ||
| return visit(node) | ||
| % end | ||
| % end | ||
| default: | ||
| fatalError("Not expression?") | ||
| } | ||
| } |
There was a problem hiding this comment.
Why do we need this method? I don’t think we should ever end up calling visit with a base node type. Same for the methods below.
|
@swift-ci please test |
This visitor returns a transformed Syntax node rather than exposing a pre-post visitor.
… RestultTypes. Rather than the default implemenation returning all children flatted together into an Array, the default implementation now just hits a fatalError.
zoecarver
force-pushed
the
transform-visitor
branch
from
caa99e8 to
cdb390d
Compare
|
@swift-ci please test |
This visitor returns a transformed Syntax node rather than exposing a pre-post visitor. This will be helpful for ASTGen.