[ASTGen] Several improvements to generalize node handling #69894
Conversation
`generate(choices:)` is not good because it expects "ASTNode". Not all `SyntaxChildChoices` can generate ASTNode. * `generate(codeBlockItem:)` to manually switch over the cases * switch on `IfExpr.ElseBody` insdie `makeIfStmt()`, now supports `else if` case
We already know the type of the node. There's no reason switch over again and wrap the result with ASTNode.
Generating ConditionElement using generate(_:) wouldn't work when we implemnt other condition kinds e.g. patterns because they are not ASTNode kinds. Remove generate(choices:) and generate(_:) as they weren't used anymore.
Now that ASTNode is used only for codeBlockItem generation. That align with C++ ASTNode where it represent code items. Since TypeRepr is not an ASTGen in C++, remove it from ASTGen too.
Defining `generate(_: XXXSyntax?) -> BridgedXXX` for each node kind is too much boilerplate. self.generate(optional: optionalXXXNode).asNullable is now: self.map(optionalXXXNode, generate(xxx:))
|
@swift-ci Please smoke test |
| thrownType: self.generate(optional: node.effectSpecifiers?.thrownError?.type).asNullable, | ||
| thrownType: self.map(node.effectSpecifiers?.thrownError?.type, generate(type:)), |
There was a problem hiding this comment.
I’m not a fan of this. I think the old version read a lot easier and IMO it read so much easier that it’s worth defining the optional overloads.
@AnthonyLatsis and I had a whole conversation about this in #67111 (comment)
There was a problem hiding this comment.
I don't feel too strongly about this, though I think I do mildly prefer the old version
There was a problem hiding this comment.
OK, let's restore the specialized generate functions.
| private func _build<Node: SyntaxProtocol>( | ||
| kind: Node.Type, | ||
| private func _build<Node: SyntaxProtocol, Result>( | ||
| generator: (ASTGenVisitor) -> (Node) -> Result, |
There was a problem hiding this comment.
I would name this generateFunction. generator sounds to much like an object to me. Also, I think a doc comment for this parameter wouldn’t hurt.
|
@swift-ci Please smoke test |
| @@ -83,7 +83,7 @@ extension ASTGenVisitor { | |||
|
|
|||
| let (secondName, secondNameLoc) = node.secondName.bridgedIdentifierAndSourceLoc(in: self) | |||
|
|
|||
| var type = self.generate(optional: node.optionalType) | |||
| var type = node.optionalType.map(generate(type:)) | |||
There was a problem hiding this comment.
Do you need to change this to also use generate(optional:)?
There was a problem hiding this comment.
Now it returns BridgedNullableTypeRepr instead of BridgedTypeRepr? which is not convenient to convert back to BridgedTypeRepr
generate(choices:)because it was expecting all the choices can generateASTNodewhich is not the case.generate(_:) -> ASTNodebecause, again, not all syntax kind can generateASTNodegenerate(codeBlockItem:)now returnsASTNodeandASTNodeis now.decl,.stmt, or.expronly. That resembles C++swift::ASTNode.Nullablevariation can now haveHasNullableconformance, that teaches how to convertOptional<BridgedT>toBridgedNullableTASTGenVisitor.map(_: T?, _: (T) -> BridgedU) -> BridgedNullableUfor optional node handling.ASTGenVisitor.map(_: TCollection?, _: (TCollection) -> BridgedArrayRef) -> BridgedArrayRef)for optional collection node handling