Use discriminated unions for node categories #18285
Conversation
weswigham
requested review from
DanielRosenwasser,
ahejlsberg,
mhegazy and
a user
|
Fixes #13634 |
| export type BinaryOperatorToken = Token<BinaryOperator>; | ||
|
|
||
| export interface BinaryExpression extends Expression, Declaration { | ||
| export type BinaryOperatorToken = |
There was a problem hiding this comment.
Don't see why we need unions for the tokens; it would be easier to write Token<BinaryOperator>.
There was a problem hiding this comment.
Token is not assignable to the union of all tokens of the binary operator kinds because we do not multiply out/canonicalize unions prior to comparison checking. As such, it must be written this way to work around that.
There was a problem hiding this comment.
I actually tried this out myself and it compiled successfully. We're not depending anywhere on discriminating a Token by its kind; the only thing that matters is the kind and they're otherwise identical.
|
This branch takes 156 seconds to run |
Migration WIP More betterness Unions, unions everywhere Full type action Finish updating services layer Just change the type
weswigham
force-pushed
the
node-discriminated-union
branch
from
dfaa1b5 to
8110b8c
Compare
Found while updating #18285 to latest master. Not sure what this fixes, but it was definitely incorrect - `node` must be a `Block` at this point, so this cast must have been intended for `node.parent`, which was checked against `TryStatement` right before it.
Found while updating #18285 to latest master. Not sure what this fixes, but it was definitely incorrect - `node` must be a `Block` at this point, so this cast must have been intended for `node.parent`, which was checked against `TryStatement` right before it.
|
We have captured this as a test already. I will keep the branch for experimentation, but closing the PR since we have no plans to do this in the short/medium term. |
|
I'll be monitoring the perf of the capture and when it becomes subjectively acceptable, I will propose it again. 🌞 |
As mentioned in this comment, we can remove a lot of casts by using a discriminated union for
Node. This does so, and should be considered a breaking change as such. This only removes casts from locations which needed to be changed due to invalid casts/new required casts - many unneeded casts still remain and can be removed mechanically.While this should not affect our runtime performance (as it just changes our types, mostly) this does cause us to take significantly longer to compile ourselves. I hope that if it is not acceptably fast that this can drive us to make the features slowing this down faster, as we really want the extra safety offered by discrimination and exhaustiveness checks.