Fixed excess and common property checks with NoInfer
#57673
Conversation
typescript-bot
added
the
For Uncommitted Bug
|
This PR doesn't have any linked issues. Please open an issue that references this PR. From there we can discuss and prioritise. |
Andarist
force-pushed
the
fix/excess-n-common-checks-noinfer
branch
from
aa83f78
to
bcbd419
Compare
src/compiler/checker.ts
Outdated
| @@ -23756,6 +23756,9 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker { | |||
| 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.
|
This PR was not up to date with main and broke baselines. |
|
Argh! |
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