[v2] feat(ast): add CatchClause::kind - #2023
Conversation
|
|
Bencher Report| Branch | az/v2-catch-clause-kind |
| Testbed | ci |
🐰 View full continuous benchmarking report in Bencher
⚠️ WARNING: Truncated view!The full continuous benchmarking report exceeds the maximum length allowed on this platform.
c6e6930 to
491fe29
Compare
ggiraldez
left a comment
ggiraldez
left a comment
There was a problem hiding this comment.
I'm not convinced this is a good approach. The problem with the binding mentioned in the PR description is not fixed by this. If it makes sense, we should split BuiltIn::ErrorOrPanic into BuiltIn::Error and BuiltIn::Panic. Other than that, there are other ways to distinguish the clause kinds, even not resorting to name string comparisons. The parameter types captured at each clause have distinct types (which we should validate, but that's not done yet).
Also, since the valid clauses are very much restricted and since the number of possible variants is fixed, we should probably normalize this at the IR and provide the AST with a CatchClauseKind IR enum node which binds the parameters. Eg.
enum CatchClauseKind {
Error(ErrorClause),
Panic(PanicClause),
Lowlevel(LowlevelClause),
UnboundLowlevel,
}(Ideally we would elide the *Clause types and go with Parameters directly, but the IR model has some limitations)
Finally, CatchClauseKind moving to common seems off to me. It's a small detail in one diagnostic, nothing more. If we were to lift it up semantically, it makes more sense to have it as a IR construct.
|
@ggiraldez, thanks! Please check if the 2nd commit makes more sense. |
`Error` and `Panic` catch clauses both bind to the same built-in, so the kind a clause declares is not recoverable from the binding. The structure pass already decides it from the selector name to reject invalid and duplicate clauses; move that decision onto the IR node and read it from both the pass and the AST. `CatchClauseKind` was declared inside the `DuplicateCatchClause` diagnostic that carries it; give it a home of its own among the language-level enums in `common`, so that neither the IR nor the AST names a type owned by a diagnostic.
Supersedes the hand-written node extensions; the kind is classified once in the CST-to-IR builder, mirroring FunctionKind.
f34053a to
9da1e74
Compare
806fdb2 to
9da1e74
Compare
|
I submitted #2031 which supersedes this PR. The clause kind is now encoded in the IR/AST types and at the same time we ensure that only valid clauses appear in the IR. Sorry for the noise with the failed run. I had pushed a commit here but it overwrote all the existing changes, so I decided to open a new PR instead. |
|
I'll close it since #2031 replaced it |
3 participants
ErrorandPaniccatch clauses both bind to the same built-in, so the kind a clause declares is not recoverable from the binding. The structure pass already decides it from the selector name to reject invalid and duplicate clauses; move that decision onto the IR node and read it from both the pass and the AST.