[ASTGen] Introduce wrapper types for AST nodes #69229
Conversation
hamishknight
force-pushed
the
boxing-match
branch
from
9a25563 to
2cf13d8
Compare
hamishknight
changed the title
hamishknight
requested review from
CodaFi,
hborla,
slavapestov,
xedin and
zoecarver
as code owners
|
@swift-ci please test |
|
@ahoppen For #68348 my preference is to import as static members since this would make it easier to switch to C++ interop (I suppose we could import as initializers with C++ interop too, but I don't think we should favor |
hamishknight
added
the
ASTGen
ahoppen
left a comment
ahoppen
left a comment
There was a problem hiding this comment.
Soooo much nicer. Thank you!
A couple of ideas to improve things further inline but as far as I’m concerned nothing that’s blocking us from merging the PR.
lib/AST/CASTBridging.cpp
Outdated
| static inline BridgedDeclContext bridgedDeclContext(DeclContext *declContext) { | ||
| return {declContext}; | ||
| } | ||
|
|
||
| static inline DeclContext *unbridged(BridgedDeclContext cDeclContext) { | ||
| return static_cast<DeclContext *>(cDeclContext.raw); | ||
| } |
There was a problem hiding this comment.
In Swift, these would make wonderful extensions on BridgedDeclContext and DeclContext. A shame we don’t have type extensions in C++. 😢
| return BridgedASTNode(ptr: e, kind: .expr) | ||
| return BridgedASTNode(ptr: e.raw!, kind: .expr) |
There was a problem hiding this comment.
Should we create a BridgedASTNode with a nullptr instead of crashing here? AFAICT nothing guarantees that raw is not nil here.
There was a problem hiding this comment.
BridgedASTNode has a non-nullable value, so we have to unwrap here. We could change BridgedASTNode to take a nullable value, but that would just be trading an explicit unwrap for an implicit null pointer access in C++ land. The alternative here would be to introduce separate nullable types for the base AST node wrapper types, e.g BridgedExpr and BridgedNullableExpr, and then parameters expecting a nullable expr could use the nullable version and you'd have to add an extra .init(...) when passing the argument. I can't say I'm a huge fan of that, though I do agree the force unwrap here isn't great.
There was a problem hiding this comment.
Or a completely different option: Do you think we actually need ASTNode or could we remove it altogether (definitely not this PR). To me ASTNode seems like an anti-pattern anyway because it generalizes too much.
For example ASTGen.generate(ExprSyntax) should switch over all the expression nodes (for which we need to implement an ExprSyntaxEnum, which I think I have thought about adding for 2 years now) and then it could just return a BridgedExprSyntax without the need to go through ASTNode.
There was a problem hiding this comment.
The issue is for things like BraceStmt, where the elements truly are ASTNodes :/ AFAIK that's actually the only place this logic is currently used.
There was a problem hiding this comment.
FWIW this is what it would look like with nullable wrappers ef74078, I guess it's not too bad? Still can't say I love it
There was a problem hiding this comment.
I think I prefer it with ef74078. At least it’s explicit about what it’s doing and the asNullable are annoying but not toooo bad. But it’s Friday evening and I might change my mind when I look at this again on Monday.
There was a problem hiding this comment.
Yeah, I don't really have particulary strong feelings either way, I've left it as-is for now, but am happy to do the nullable wrappers in a follow up
There was a problem hiding this comment.
Hmm the more I think about, the less I hate it, put up a PR: #69326
This is for decl nodes that are both abstract and are DeclContexts.
Stamp out wrapper types for all the AST nodes, and use them for ASTGen, with members being imported on those types.
This is consistent with the AST node bridging naming scheme and IMO the old names were needlessly verbose.
hamishknight
force-pushed
the
boxing-match
branch
from
2cf13d8 to
3639d86
Compare
|
@swift-ci please test |
2 participants
Stamp out wrapper types for all the AST nodes, and use them for ASTGen, with members being imported on those types.
Resolves #68346
Resolves #68348
rdar://117159010