Default reverse mapped type inference to its constraint #56300
Conversation
|
@typescript-bot test top200 |
|
Heya @jakebailey, I've started to run the parallelized Definitely Typed test suite on this PR at 720a860. You can monitor the build here. Update: The results are in! |
|
Heya @jakebailey, I've started to run the tarball bundle task on this PR at 720a860. You can monitor the build here. |
|
Heya @jakebailey, I've started to run the diff-based user code test suite on this PR at 720a860. You can monitor the build here. Update: The results are in! |
|
Heya @jakebailey, I've started to run the regular perf test suite on this PR at 720a860. You can monitor the build here. Update: The results are in! |
|
Heya @jakebailey, I've started to run the diff-based top-repos suite on this PR at 720a860. You can monitor the build here. |
|
Hey @jakebailey, I've packed this into an installable tgz. You can install it for testing by referencing it in your and then running There is also a playground for this build and an npm module you can use via |
|
@jakebailey Here are the results of running the user test suite comparing There were infrastructure failures potentially unrelated to your change:
Otherwise... Everything looks good! |
|
@jakebailey Here they are:
CompilerComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
tsserverComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
StartupComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Hey @jakebailey, the results of running the DT tests are ready. |
src/compiler/checker.ts
Outdated
| const inference = createInferenceInfo(typeParameter); | ||
| inferTypes([inference], sourceType, templateType); | ||
| return getTypeFromInference(inference) || unknownType; | ||
| return getTypeFromInference(inference) || getBaseConstraintOfType(typeParameter) || unknownType; |
There was a problem hiding this comment.
getBaseConstraintOfType recursively walks all constraints - so if T extends U and U extends V and V extends number, you get number out. Which means you skip inferences to those intermediate results. I think you'd want to use getConstraintOfType, right? Because, in this case, if you have T[K] extends U and U extends number, U is a better (and more specific) answer than number.
There was a problem hiding this comment.
I think you'd want to use getConstraintOfType, right?
Sort of. It's true it won't immediately walk all constraints but at the same time, it's not inference-aware anyway. If I call it then I end up with T[keyof T] and when relating the inferred type to the instantiated constraint (from within getInferredType) we eventually simplify this to U and to its base constraint. So to some extent, this leads to the same thing and the same problems. To incorporate inferences of other type parameters I'd have to utilize context.nonFixingMapper.
It's not immediately obvious how to get access to this from within this function (inferReverseMappedType) but that certainly can be done. A bigger problem though is that inferReverseMappedType gets called at different times for object and array/tuple types - when dealing with the latter it's called eagerly~. So it gets called right from within inferFromTypes. This introduces differences in data availability between the object and the array/tuple case.
For instance, let's take a look at this:
declare function foo<T extends U[], U extends string | number | boolean>(
b: {
[K in keyof T]: (arg: T[K]) => void;
},
a: U,
): T;
declare const data: number | string;
const result = foo([(arg) => {}, () => {}], data);Since inferReverseMappedType gets called eagerly here to create the reverse mapped tuple type it's not possible to observe the U's inferred type. That is different from the equivalent object variant, like:
declare function foo<
T extends Record<PropertyKey, U>,
U extends string | number | boolean,
>(
b: {
[K in keyof T]: (arg: T[K]) => void;
},
a: U,
): T;
declare const data: number | string;
const result = foo(
{
a: (arg) => {},
b: () => {},
},
data,
);This isn't exactly a new problem. I've encountered this already in the past at least once or twice.
I'd really like to solve this. It feels like a separate issue though - but then: what would be the acceptable state of this PR to get this one in?
- does anything have to be done to move this one forward?
- would making
inferReverseMappedTypeaware of the inference context (to get access to its.nonFixingMapper) be enough? - should the object/array/tuple be unified for the whole thing to work in the same predictable way?
…efault-to-constraint
Andarist
force-pushed
the
reverse-mapped-type-default-to-constraint
branch
from
b3f2274 to
a0d8bcc
Compare
src/compiler/checker.ts
Outdated
| const instantiatedConstraint = instantiateType(constraint, context.nonFixingMapper); | ||
| // TODO: decide what to do about fallback type | ||
| if (inferredType && inferredType.flags & TypeFlags.Object && (inferredType as ObjectType).objectFlags & ObjectFlags.ReverseMapped) { | ||
| (inferredType as ReverseMappedType).inferenceMapper = context.nonFixingMapper; |
There was a problem hiding this comment.
Hm. A mutation like this is odd, especially if the context.compareTypes call causes reentrancy on the inferredType (different places in the comparison could witness different mappers with different results). Instead, having a non-fixing-mapper clone of the type seems prudent.
There was a problem hiding this comment.
Ah yes, this mutation is just the best I was able to figure out so far when it comes to exposing this mapper to inferReverseMappedType. I meant to comment on that here but you beat me to it and looked here first :P
I could maintain this in some stack or something but OTOH the mutation felt simpler because it just makes it directly accessible at the call site that needs this. It looks odd, especially here, but it's because reverse mapped types just don't participate in the same code path as everything else - all of them are kinda going through their own inference passes, even though they somewhat originate in this "parent" pass (they are not linked to it though).
I was also thinking that perhaps instead of assigning a mapper I could just assign the instantiatedConstraint. In away, the instantiation that I do now in inferReverseMappedType is quite similar to potential getIndexedAccessType(instantiatedConstraint, propertyNameType). Would that alleviate the reentrancy concern at all?
Instead, having a non-fixing-mapper clone of the type seems prudent.
I'm afraid that I don't understand what should be cloned here and where the clone should be used. Could you elaborate on this?
One extra thing that I was thinking about. getInferredType can return cached but non-fixed results. Does it mean that by doing this thing here we might accidentally~ "fix" the properties of a non-fixed result? Or is it just not a concern because the reverse mapped object with those fixed properties doesn't survive inferredType clears so it would be recreated and its properties would get "fixed" again (but now with the new nonFixingMapper/instantiatedConstraint). I didn't yet have time to investigate how this behaves in practice.
There was a problem hiding this comment.
I'm afraid that I don't understand what should be cloned here and where the clone should be used. Could you elaborate on this?
Have a copy of the reverse mapped type that stores the nonFixingMapper instead of the normal mapper (and probably have that reverse mapped type mutually reference the normal version of the type). (And include that variant-ness in the ID of the reverse mapped type, so they're recognized as separate types in the caches)
Or is it just not a concern because the reverse mapped object with those fixed properties doesn't survive inferredType clears so it would be recreated and its properties would get "fixed" again (but now with the new nonFixingMapper/instantiatedConstraint). I didn't yet have time to investigate how this behaves in practice.
Yes but no. Reverse mapped types are made in inference, so they can be pretty short-lived, but they're identified structurally - all reverse mapped types with the same source, target, and constraint types are the same reverse mapped types, even when they occur at differing places in inference. So even if it's initially short lived, it can be pulled back out later by another attempt to construct one over the same types. That's actually another issue with this as it currently stands - including the mapper in the type (...by including it in the properties via this honestly roundabout way - passing it into inferTypeForHomomorphicMappedType/createReverseMappedType should clarify things) should affect the type id, but getting a good structural id out of a mapper isn't simple (since function mappers exist aplenty). This means in this PR right now, reverse mapped types mangle one another - only the first inference context within which a reverse mapped type of a certain structure is used gets cached, which is not correct. I think that since reverse mapped types are made within an inference context, them keeping a reference to said context is probably reasonable, but we'll definitely need to check that potentially duplicating reverse mapped types like that isn't terrible for perf on real projects (since inference contexts (and mappers) are rather throwaway).
…efault-to-constraint
|
@typescript-bot test it |
|
Hey @jakebailey, I've packed this into an installable tgz. You can install it for testing by referencing it in your and then running There is also a playground for this build and an npm module you can use via |
|
Hey @jakebailey, the results of running the DT tests are ready. Everything looks the same! |
|
@jakebailey Here are the results of running the user tests with tsc comparing There were infrastructure failures potentially unrelated to your change:
Otherwise... Everything looks good! |
|
@jakebailey Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@jakebailey Here are the results of running the top 400 repos with tsc comparing Everything looks good! |
5 participants
closes #56241