Fixed excess and common property checks with NoInfer#57673
Uh oh!
There was an error while loading. Please reload this page.
Conversation
typescript-bot added the For Uncommitted Bug 
typescript-bot commented Mar 7, 2024
This PR doesn't have any linked issues. Please open an issue that references this PR. From there we can discuss and prioritise. |
Andaristforce-pushed the fix/excess-n-common-checks-noinfer branch from aa83f78 to bcbd419Compare
ahejlsberg left a comment
ahejlsberg left a comment There was a problem hiding this comment.
Approved, but would prefer to erase all substitution types as I suggest.
src/compiler/checker.ts Outdated
| return resolved.callSignatures.length === 0 && resolved.constructSignatures.length === 0 && resolved.indexInfos.length === 0 && | ||
| resolved.properties.length > 0 && every(resolved.properties, p => !!(p.flags & SymbolFlags.Optional)); | ||
| } | ||
| if (isNoInferType(type)){ |
There was a problem hiding this comment.
I think it would be better to erase all substitution types (to their base types) instead of just NoInfer types.
There was a problem hiding this comment.
I pushed out the requested change but without any new tests.
I've tried a bunch of things based on my intuition in an attempt to write some tests for this change and nothing - from my attempts - managed to hit those lines with a non-NoInfer substitution type. So I went to recheck if I could observe some cases like this using the existing test suite:
isExcessPropertyCheckTarget/isKnownPropertydidn't observe such substitution types at allisWeakTypehas observed it indeeplyNestedMappedTypesandconditionalTypeDoesntSpinForever. Those are complex enough that figuring out a test case based solely on them might take me a longer moment.
There was a problem hiding this comment.
Yeah, it's not going to be easy to come up with a case where it matters and I'm fine going without.
jakebailey commented Apr 2, 2024
This PR was not up to date with main and broke baselines. |
ahejlsberg commented Apr 2, 2024
Argh! |
4 participants
I figured out that it might be better to handle
NoInferat those levels because there is always a risk thatisWeakType/isKnownPropertymight get called outside of thecheckTypeRelatedTo.However, this could potentially also be fixed within
getNormalizedType- I briefly took a look at this possible solution and it felt like it could be slightly more complicated and that it could potentially result in potential gotchas. Normally, each constituent gets normalized while the type gets broken down in the relationship check - not when the union/intersection gets first observed by it. In fact, at one moment in time normalization for intersection members was introduced by #49119 and then partially reverted in #50535 (the logic got adjusted to only normalize intersections containing{}).If you think that it's a wrong judgement call - please let me know and I'll adjust the PR to put this logic into
getNormalizedType. There is also a risk that normalization earlier could fix some other things. For the most part those other things are likely working correctly already because those types get normalized - just later on.fixes#57697