Fix duplicate errors in js special assignments #24508
Conversation
src/compiler/checker.ts
Outdated
| @@ -4627,7 +4628,7 @@ namespace ts { | |||
| // function/class/{} assignments are fresh declarations, not property assignments, so only add prototype assignments | |||
| const specialDeclaration = getAssignedJavascriptInitializer(symbol.valueDeclaration); | |||
| if (specialDeclaration) { | |||
| return getWidenedLiteralType(checkExpressionCached(specialDeclaration)); | |||
| return getWidenedLiteralType(checkExpressionCached(specialDeclaration, CheckMode.NoDeferredCheck)); | |||
There was a problem hiding this comment.
So. CheckMode.NoDeferredCheck isn't falsy. Which means this call to checkExpressionCached is really a roundabout call to checkExpression.
There was a problem hiding this comment.
Yes, nice catch. Fixed.
Aside: This means that, originally, the second check of the method body must have gone through checkExpression already or it would have gotten the resolved type and avoided it. This leads to an invariant: once you call checkExpressionCached you should only ever call checkExpressionCached afterward, not checkExpression. Or checkExpression should check links.resolvedType before doing any work.
What's more, it's not in general safe to check any node twice because of side effects to deferredNodes. Maybe expensive asserts in checkDeferredNodes would actually be worth it...
Later: I added the resolvedType check in checkExpression and the only change it makes to tests is the order that a couple of unions print in. So it might be a worthwhile change — even better than the current one, in fact.
|
Reprinting my comment from above, because github attached it to a now-outdated line: This means that, originally, the second check of the method body must have gone through checkExpression already or it would have gotten the resolved type and avoided it. This leads to an invariant: once you call checkExpressionCached you should only ever call checkExpressionCached afterward, not checkExpression. Or checkExpression should check links.resolvedType before doing any work. What's more, it's not in general safe to check any node twice because of side effects to deferredNodes. Maybe expensive asserts in checkDeferredNodes would actually be worth it... Later: I added the resolvedType check in checkExpression and the only change it makes to tests is the order that a couple of unions print in. So it might be a worthwhile change — even better than the current one, in fact. |
|
@weswigham @Andy-MS let's talk about this after the standup today since I believe you both have spent more time thinking about deferred nodes than I have. |
|
We decided that nobody trusts our test coverage to tell us if using the resolvedType from checkExpressionCached in checkExpression is safe. I'll leave the PR the way it is. |
|
@weswigham @mhegazy User tests are still moving duplicate errors around because of this bug. Mind taking another look? |
src/compiler/checker.ts
Outdated
| @@ -598,6 +598,7 @@ namespace ts { | |||
| SkipContextSensitive = 1, // Skip context sensitive function expressions | |||
| Inferential = 2, // Inferential typing | |||
| Contextual = 3, // Normal type checking informed by a contextual type, therefore not cacheable | |||
| NoDeferredCheck = 4, // Don't add any nodes for deferred checking | |||
| } | |||
There was a problem hiding this comment.
The comment is a little misleading since it only disables deferred checking for class expressions - should probably mention that so someone doesn't come back later and because "it doesn't do what it says it does", "fix" it.
|
Ah, I had forgotten (and didn't write down) your Map suggestion. I will try it and see how it goes. I'm not sure it will work in this case, though, because I think there's one deferred check and one in-order check. Switching to Map is worthwhile just for the ability to add a cheap no-dupes assert, though. |
I added an assert when a duplicate was added, but it caused 18 failures in our test suite.
|
Looks like I was wrong -- switching to a map suffices to fix the bug. I'll just do that. |
Fixes #24252 by not adding a deferred check in getWidenedTypeFromJSSpecialAssignment. This check will be duplicated by the normal checker's walk.