Fixed excess and common property checks with NoInfer - #57673
Conversation
Sorry, something went wrong.
|
This PR doesn't have any linked issues. Please open an issue that references this PR. From there we can discuss and prioritise. |
Sorry, something went wrong.
Anders Hejlsberg (ahejlsberg)
left a comment
There was a problem hiding this comment.
Approved, but would prefer to erase all substitution types as I suggest.
Sorry, something went wrong.
| 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.
Sorry, something went wrong.
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/isKnownProperty didn't observe such substitution types at all
- isWeakType has observed it in deeplyNestedMappedTypes and conditionalTypeDoesntSpinForever. Those are complex enough that figuring out a test case based solely on them might take me a longer moment.
Sorry, something went wrong.
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.
Sorry, something went wrong.
|
This PR was not up to date with main and broke baselines. |
Sorry, something went wrong.
|
Argh! |
Sorry, something went wrong.
I figured out that it might be better to handle NoInfer at those levels because there is always a risk that isWeakType/isKnownProperty might get called outside of the checkTypeRelatedTo.
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