Infer type parameters from indexes on those parameters #53017
Conversation
typescript-bot
added
the
For Uncommitted Bug
|
The TypeScript team hasn't accepted the linked issue #51612. If you can get it accepted, this PR will have a better chance of being reviewed. |
src/compiler/checker.ts
Outdated
| const inferences = [createInferenceInfo(typeParameter)]; | ||
| inferTypes(inferences, sourceType, templateType); | ||
| const wholeInferredType = getTypeFromInference(inferences[0]); | ||
| if (!wholeInferredType && inferences.length > 1) { |
There was a problem hiding this comment.
If any direct inference was made for T[K] then we will ignore inferences made for its properties. I think it's the right thing to do here to preserve backward compatibility. This means that, In a sense, the property-based inferences simply~ have a lower inference priority.
src/compiler/checker.ts
Outdated
| const wholeInferredType = getTypeFromInference(inferences[0]); | ||
| if (!wholeInferredType && inferences.length > 1) { | ||
| const members = createSymbolTable(); | ||
| for (let i = 1; i < inferences.length; i++) { |
There was a problem hiding this comment.
This assumes that we only collect inferences for direct properties of the reverse mapped type. I think that this is enough for a lot of cases but we could also try to infer deeper ones. This would complicate the algorithm a little bit though. Right now I can just "merge" all of those extra inferences into a single object. If we'd infer into nested properties then:
- this merging would have to be smart enough to handle properties at arbitrary levels when those inferences are kept in a flat array and they don't have any particular order that would help the merging algorithm. Maybe the layout of those extra inferences could be improved to aid this merging algorithm but then that would complicate the
inferences.push(inference)call site - we should decide what to do when we find inferences for both
T[K]['foo']andT[K]['foo']['a']. That is pretty much the same situation asT[K]andT[K]['foo'], so I think that we simply should always "prefer" the less nested one
There was a problem hiding this comment.
There is also an open question if this whole feature is a good idea. One could argue that this is inconsistent with other parts of TS since TS never collects such "partial"/prop-based inferences elsewhere... OTOH, it actually kinda does this with T[K] in the reverse mapped type.
So I treat this feature as an extension of the existing mechanism for reverse mapped types. I think that this would enable some great new capabilities for library authors because this feature would allow inferring multiple unique things per tuple element/object property. Right now it's only possible to infer a single unique thing per tuple element/object property and that is a little bit limiting when it comes to expressing complex types.
There was a problem hiding this comment.
This assumes that we only collect inferences for direct properties of the reverse mapped type.
while the implementation has completely changed (basically I pulled #20126 here and just tweaked it a little bit) - this is still a "limitation" but I still believe that it's fine. So this doesn't work with the current state of this PR:
declare function test3<
T extends { p1: { p1p1: unknown; p1p2: unknown }; p2: unknown }
>(o: { a: T["p1"]["p1p1"]; b: T["p2"]; c: T["p1"]["p1p2"] }): T;
test3({
a: "foo",
b: 42,
c: true,
});There is also an open question if this whole feature is a good idea. One could argue that this is inconsistent with other parts of TS since TS never collects such "partial"/prop-based inferences elsewhere...
This is no longer the case with this PR since this infers now:
declare function test2<T extends { p1: unknown; p2: unknown }>(o: {
a: T["p1"];
b: T["p2"];
}): T;
// `T` inferred as `{ p1: string; } & { p2: number; }`
test2({
a: "foo",
b: 42,
});
typescript-bot
removed
the
For Uncommitted Bug
There was a problem hiding this comment.
I had #20126 which was pretty functional a long time ago, which we basically only didn't take because nobody could be bothered to care about it without a backing issue (myself included), but that seems to be an even more general version of this (or at least relied on a similar partial inferencing mechanism), since these kinds of partial inferences that you combine later are applicable beyond reverse mapped types.
src/compiler/checker.ts
Outdated
| (target as IndexedAccessType).indexType.flags & TypeFlags.StringLiteral && | ||
| ((target as IndexedAccessType).objectType.flags & TypeFlags.IndexedAccess) | ||
| ) { | ||
| inferences.push(inference = createInferenceInfo(target)); |
There was a problem hiding this comment.
inference lists are stored on inference contexts and used to created cached type mappers, so mutating the list like this is a no-go, since it could muck with the mapper state when the context is cloned and mappers get remade. You may want to take a page from #52944 and adjust inferTypes to take a full InferenceContext and store the sidecar data there, otherwise storing it on the existing inference for the root object could work reasonably well as in #20126.
There was a problem hiding this comment.
I tried to learn from #52944 but that PR is quite a lot to take in for me 😅 So I decided to reuse ur work from #20126 as it was much simpler to pull off.
After some tweaks I managed to make it work for the reverse mapped types case that this PR originally was meant to fix. There are currently some issues with the current state of this PR:
- some of the tests that you originally added were changed, you can see the commit that updates those baselines here (some are just display changes, some are not though)
- self-check fails
I intend to investigate both problems but perhaps you'd be able to peek here in the meantime and validate the used approach. It would be great to know if I'm heading into the right direction here.
…ties-of-reverse-mapped-types
…to infer-concrete-properties-of-reverse-mapped-types-with-wes
…le reverse mapped type
src/compiler/checker.ts
Outdated
| @@ -25141,6 +25160,39 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker { | |||
| let inferredType: Type | undefined; | |||
| const signature = context.signature; | |||
| if (signature) { | |||
| if (inference.indexes) { | |||
There was a problem hiding this comment.
At the time of this comment this aggregated inference is pushed as an extra candidate (that's the behavior preserved from #20126 ).
That creates problems with cases like this:
interface Expression extends Node {
_expressionBrand: any;
}
declare function setParent<T extends Node>(child: T, parent: T["parent"] | undefined): T;
interface Node {
readonly kind: number;
readonly parent: Node;
}
declare const expr: Expression
declare const node: Node
const res = setParent(expr, node)
// ^? actual: Node, expected: Expression
export {}It incorrectly widens the inferred type because it's able to eliminate the inferred subtype (Expression) through subtype reduction~.
I was afraid of cases like this in my initial implementation and in there I decided to just use the gathered indexes~ when there were no candidates for the "naked" type variable. Should I do that here and avoid making the intersected indexes the "same priority" candidate?
There was a problem hiding this comment.
To clarify, this is what happens right now:
- we end up with 2 candidates (because the
aggregateInferenceis appended as an extra candidate):Expressionand{ parent: Node; } - we compute a common super type (
getCommonSupertype(baseCandidates)) which is{ parent: Node; } - that is not assignable to the constraint, it fails the
compareTypescheck and thus we assign the instantiated constraint as the inferred type (inference.inferredType = inferredType = instantiatedConstraint) - that in turn is just OK further down the road since the constraint (
Node) satisfies everything here
I tried to just always require the aggregateInference to be comparable to the constraint but that broke the "discriminating logic" added for this aggregateInference stuff. So I reverted that change and only added this requirement for non-union constraints.
At this point, the logic related to pushing aggregateInference into the inference.candidates is a little bit hairy 🤷♂️ I guess the main question here is if every bit of the included enhancement is good at the end of the day, how it fits the rest of the compiler etc. That's something on what you'd have to chime in with @ahejlsberg .
I still have some of those change to investigate but at least the self-check is working now so perhaps we could create a build using this PR for toying purposes.
Andarist
changed the title
…ties-of-reverse-mapped-types
|
Status of this PR:
|
|
@weswigham friendly 🏓 I'm really curious about your thoughts on this one 😉 |
…ties-of-reverse-mapped-types # Conflicts: # src/lib/es5.d.ts
…ties-of-reverse-mapped-types
Andarist
force-pushed
the
infer-concrete-properties-of-reverse-mapped-types
branch
from
ded12e2 to
375c127
Compare
|
@weswigham friendly 🏓 |
…ties-of-reverse-mapped-types
…ties-of-reverse-mapped-types
|
@typescript-bot test it |
|
Hey @jakebailey, the results of running the DT tests are ready. There were interesting changes: Branch only errors:Package: node/v16 Package: node/v18 Package: rdfjs__data-model |
|
@jakebailey Here are the results of running the user tests with tsc comparing Something interesting changed - please have a look. Details
|
|
@jakebailey, the perf run you requested failed. You can check the log here. |
|
@typescript-bot perf test this faster |
|
@jakebailey Here are the results of running the top 400 repos with tsc comparing Something interesting changed - please have a look. Details
|
|
@jakebailey Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@typescript-bot pack this |
|
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 |
There was a problem hiding this comment.
I still think that this feature is especially nice for inferring indexes on reverse mapped types. This PR also enables inferring regular type parameters using those indexes. This is a nice improvement and I don't see many drawbacks of allowing this. However, I feel that a dedicated syntax for declaring/using "generic bags" would be much better. There are things that a dedicated syntax could do that this just can't do. I mentioned all of that in my feature proposal here: #54254 .
I think this change is a pretty
good improvement, but I'm biased. Probably need to look into the top repo changes, though.
the original PR by @weswigham included the "aggregate inference" as an extra candidate. That suffers from some subtle issues as it might impact how supertypes/subtypes are chosen from a set of candidates. I added tests to cover those cases (they were found by the self-check). That's why I only attempt to use the aggregate inference if there are no other candidates in sight. I think it's the best thing to do here.
Yeah, it being a lower priority inference target is probably fine.
the original PR by @weswigham had a logic for discriminating unions based on the observed usage of the index (a related test case can be seen here). This kind of code is fundamentally unsound since there is no true relation between the argument and the return value. It's impossible to write the implementation of such a function without casting and so on. It could work if a generic would be constrained using an imaginary oneof operator~ but - as it is - I don't think this is OK. For the time being, I retained this logic but I'd like to drop this branch of the code and the related test cases.
Inference isn't about picking the one choice to end them all, it's about picking a choice for instantiation that works for the arguments provided and that roughly matches user expectations. Otherwise we'd just always instantiate as never/unknown/any, since they'll trivially make arguments typecheck. When I write
function createNode<T extends Node>(kind: T["kind"]): T {
return nodeConstructors.get(kind)?.()! as T;
}
// assume `Node` is a `kind`-ed union type and `nodeConstructors` is a `Map<SyntaxKind, () => Node>`I'd assume, broadly speaking, that users are usually passing in a single kind, and, when they are, they'd like a single kind of node out the back end. I was always going to have to cast in the implementation though, and this choosier inference doesn't really have anything to do with that. 🤷
Anyways, broadly speaking, again, I like this, but we probably need to look at those extended test suites we just got, and, for the code itself, since I'm the originator of a big chunk of it now, I'm probably not best suited to review it. @jakebailey are you interested in this?
|
I can't say I'm an expert here but I can certainly try to review it if/when things are settled; I'm just always one to run extended tests and make playgrounds 😄 |
|
cross-linking #20126 here (it's mentioned above but hard to find) |
closes #51612