Fixed error spans for SatisfiesExpression check nodes
#56918
Conversation
typescript-bot
added
the
For Milestone Bug
| @@ -33939,6 +33940,11 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker { | |||
| } | |||
| } | |||
|
|
|||
| function getEffectiveCheckArgumentNode(argument: Expression): Expression { | |||
There was a problem hiding this comment.
I was looking for other solutions that wouldn't explicitly target argument nodes. However, I concluded that this is quite a specific situation because it's the only situation (that I could find) when the SatisfiesExpression node is also the location on which other errors (argument checks) are meant to be reported. Usually the "final target" (like an assignment target) of SatisfiesExpression is different so there is no such overlap~ like here.
There was a problem hiding this comment.
Usually the "final target" (like an assignment target) of SatisfiesExpression is different so there is no such overlap~ like here.
Counterpoint:
((): null => ({}) satisfies unknown)();There was a problem hiding this comment.
Food for thought: If the arrow function above is rewritten with braces, then the error span is correctly placed on the return. For braceless arrow functions the "final target" could be the => token.
There was a problem hiding this comment.
Counterpoint:
Great example :P I'll fix this in a moment
For braceless arrow functions the "final target" could be the => token.
I prefer to highlight smaller spans over huge ones but this one feels just too short. OTOH, an elementwise elaboration can highlight even a single character if a property is that short. I'm not sure if it would be completely apparent to everybody that highlighted => is about the return value. It's not that it doesn't make sense when explained - it's just that in isolation it doesn't feel completely intuitive/unambiguous
Andarist
changed the title
SatisfiesExpressionSatisfiesExpression check nodes
| const tuple1: [boolean, boolean] = [true, 100 satisfies unknown]; | ||
| ~~~~~~~~~ | ||
| !!! error TS2322: Type 'number' is not assignable to type 'boolean'. |
There was a problem hiding this comment.
This behavior is unchanged by this PR, but looks wrong. Did you look at this example?
There was a problem hiding this comment.
heh, I was starring at this and somehow missed the fact that it's wrong in the very same way 😅
I just pushed out a fix for this. It is a little bit cumbersome to call getEffectiveCheckNode around the code to get the best behavior... I think it semantically makes a lot of sense. It is the expression of the SatisfiesExpression that we want to highlight. But since I have to call it at multiple places it's rather easy to miss some places where it should be called too.
So I wonder now if it wouldn't be better to flip the situation slightly. I could move this logic into getErrorSpanForNode and "target" the satisfies token in checkSatisfiesExpressionWorker (since that's the only place where we want to target the satisfies token). The problem is... that I can't easily pass a token down as errorNode. It's not even directly accessible in the checker. So maybe a text span could be passed down? I'm not sure if this is even feasible though since that argument is passed down through some functions and they might even try to check stuff on it. So overall it's likely not a 5min change.
Do you think the current solution with getEffectiveCheckNode is OK? Or should I try this other alternative?
fixes #56433