[ASTDumper] Fix some more JSON AST dump crashes. #85681
Conversation
`ModuleType`s show up in some rare places, like the left-hand-side of a `DotSyntaxBaseIgnoredExpr` for a module-qualified function call. ASTMangler does not support these because they're not real types.
This avoids infinite recursion when a function captures itself; e.g.,
```swift
func foo() {
func bar() {
bar()
}
bar()
}
```
It also just avoids printing unnecessarily verbose information,
since those decls would have already been dumped elsewhere in the
tree.
allevato
requested review from
artemcm,
hborla,
slavapestov,
tshortli and
xedin
as code owners
|
@swift-ci Please smoke test |
| return ""; | ||
|
|
||
| if (isa<ModuleDecl>(D)) { | ||
| // ASTMangler does not support "module types". This can appear, for |
There was a problem hiding this comment.
You could conceivably plumb this through, because the ASTMangler does know how to mangle a ModuleDecl when it's a context of another decl. You could mangle a bare ModuleDecl as just that without anything else. Up to you
There was a problem hiding this comment.
I think that would violate the invariants about the kinds of USRs we generate in the AST output. We have declaration USRs, which are the "s:..." prefixed form used by indexstore and SourceKit, and type USRs, the standard "$s...D" symbol mangling that ends in the TypeMangling operator.
In this case, it's taking the ModuleDecl and wants to print the corresponding type USR, which I don't think we have a way to generate. Or am I misunderstanding?
Side question that came up while I was exploring this: what is the reason for leaving the DotSyntaxBaseIgnoredExpr in the already type-checked AST after resolving the function call? I noticed that only a few kinds of expressions still produce those, like Swift.print(). For type constructors (e.g., Swift.Int(), it just becomes a ConstructorRefCallExpr and the decl ref knows the exact decl, including the owning module, that Int comes from. But I would expect that to be true for the print() call, too.
| // We print the decl ref, not the full decl, to avoid infinite | ||
| // recursion when a function captures itself (and also because | ||
| // those decls are already printed elsewhere, so we don't need to | ||
| // print what could be a very large amount of information twice). |
There was a problem hiding this comment.
Yeah, that definitely makes sense even for VarDecl captures. Also, the same decl can be captured more than once, etc.
There was a problem hiding this comment.
One downside (which was already present before this PR, too) is that because local decls don't get USRs, it's not as easy to match the capture decl ref back to the exact original decl. But the decl ref still contains the name, so it can still be tracked down with a little bit more bookkeeping, and knowing that it's local constrains the search space.
allevato
left a comment
allevato
left a comment
There was a problem hiding this comment.
Thanks for the quick review!
| // We print the decl ref, not the full decl, to avoid infinite | ||
| // recursion when a function captures itself (and also because | ||
| // those decls are already printed elsewhere, so we don't need to | ||
| // print what could be a very large amount of information twice). |
There was a problem hiding this comment.
One downside (which was already present before this PR, too) is that because local decls don't get USRs, it's not as easy to match the capture decl ref back to the exact original decl. But the decl ref still contains the name, so it can still be tracked down with a little bit more bookkeeping, and knowing that it's local constrains the search space.
| return ""; | ||
|
|
||
| if (isa<ModuleDecl>(D)) { | ||
| // ASTMangler does not support "module types". This can appear, for |
There was a problem hiding this comment.
I think that would violate the invariants about the kinds of USRs we generate in the AST output. We have declaration USRs, which are the "s:..." prefixed form used by indexstore and SourceKit, and type USRs, the standard "$s...D" symbol mangling that ends in the TypeMangling operator.
In this case, it's taking the ModuleDecl and wants to print the corresponding type USR, which I don't think we have a way to generate. Or am I misunderstanding?
Side question that came up while I was exploring this: what is the reason for leaving the DotSyntaxBaseIgnoredExpr in the already type-checked AST after resolving the function call? I noticed that only a few kinds of expressions still produce those, like Swift.print(). For type constructors (e.g., Swift.Int(), it just becomes a ConstructorRefCallExpr and the decl ref knows the exact decl, including the owning module, that Int comes from. But I would expect that to be true for the print() call, too.
|
@swift-ci please smoke test macOS platform |
|
@swift-ci please smoke test |
|
@swift-ci please smoke test Windows platform |
2 participants
Don't attempt to compute type USR for
ModuleTypes.ModuleTypes show up in some rare places, like the left-hand-side of aDotSyntaxBaseIgnoredExprfor a module-qualified function call. ASTMangler does not support these because they're not real types.Print decl-refs, not full decls, for function captures.
This avoids infinite recursion when a function captures itself; e.g.,
It also just avoids printing unnecessarily verbose information, since those decls would have already been dumped elsewhere in the tree.