Remove missing type from tuple type arguments under exactOptionalPropertyTypes
#54718
Conversation
|
@typescript-bot test this |
|
Heya @jakebailey, I've started to run the parallelized Definitely Typed test suite on this PR at 1f43c8d. 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 1f43c8d. You can monitor the build here. |
|
Heya @jakebailey, I've started to run the abridged perf test suite on this PR at 1f43c8d. You can monitor the build here. Update: The results are in! |
|
Heya @jakebailey, I've started to run the diff-based user code test suite on this PR at 1f43c8d. 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 1f43c8d. You can monitor the build here. Update: The results are in! |
|
Heya @jakebailey, I've started to run the extended test suite on this PR at 1f43c8d. 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 they are:Comparison Report - main..54718
System
Hosts
Scenarios
Developer Information: |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@jakebailey Here are the results of running the user test suite comparing There were infrastructure failures potentially unrelated to your change:
Otherwise... Something interesting changed - please have a look. Details
|
|
@jakebailey Here are the results of running the top-repos suite comparing Everything looks good! |
|
Hey @jakebailey, the results of running the DT tests are ready. |
src/compiler/checker.ts
Outdated
| @@ -15217,7 +15217,7 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker { | |||
| const typeArguments = !node ? emptyArray : | |||
| node.kind === SyntaxKind.TypeReference ? concatenate(type.target.outerTypeParameters, getEffectiveTypeArguments(node, type.target.localTypeParameters!)) : | |||
| node.kind === SyntaxKind.ArrayType ? [getTypeFromTypeNode(node.elementType)] : | |||
| map(node.elements, getTypeFromTypeNode); | |||
| map(node.elements, element => removeMissingType(getTypeFromTypeNode(element), element.kind === SyntaxKind.OptionalType)); | |||
There was a problem hiding this comment.
It seems strange to me that this is being done inside of getTypeArguments; previously this function effectively just pulled info from the node and that's it, no processing.
Is there no other place this makes sense?
There was a problem hiding this comment.
I would go even further and say that the optionality should always be removed from those type arguments (of optional elements).
When it comes to object properties the optionality is added to the declared type based on the optionality marker on the property, see here. I see a strong parallel between this and the element types. The type argument is like a property's declared type - the optionality is a separate piece of information.
In a somewhat similar manner, in the case of a rest element - the type argument is not an array type! It's an element type of that array and the final array type~ gets created when the type argument meets the element flag.
I think removing this from the type arguments is best because the iterated type of an array comes from the Symbol.iterator method. Its signature is defined as (): IterableIterator<T> and T gets instantiated to the union that contains all type arguments. So at the end, we get something like (): IterableIterator<string | number | undefined>.
We might notice here that the rest type fits into this perfectly because its type is not an array type - so if we just push that to the union we end up with its element type and not an array, its concatenable~ with required elements, and the Symbol.iterator method that gets instantiated this way is correct.
It doesn't work well with optional elements in combination with exactOptionalPropertyTypes though if the undefined is included in the type argument right from the start. After all, at runtime - we won't actually iterate over that undefined since undefined isn't assignable to an optional element under EOPT.
So, I run an experiment to remove this at all times and "add back" that optionality at other places (see the commit here). It turned out to be... semi-successful. All the tests pass except the one that I'm trying to fix here.
The problem with this is that the easiest way to add back the optionality is through the created mapper (here). But then the T -> UnionOfTypeArguments uses that as well when instantiating the Symbol.iterator method. I don't see a clean way to branch this somehow or to remove the missing type afterward. After all, it's not only about what we get internally. The Symbol.iterator's method should get instantiated to, let's say, (): IterableIterator<string | number> and not to (): IterableIterator<string | number | undefined>. That's publicly~ visible information that can be obtained through types in user programs.
So, for better or worse - I think that it's best to leave things as-is but avoid adding the missingType to optional type arguments under EOPT. Perhaps a cleaner way of doing this would be something along those lines:
diff --git a/src/compiler/checker.ts b/src/compiler/checker.ts
index 512e191cce..874318fb3b 100644
--- a/src/compiler/checker.ts
+++ b/src/compiler/checker.ts
@@ -15563,7 +15563,7 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
const typeArguments = !node ? emptyArray :
node.kind === SyntaxKind.TypeReference ? concatenate(type.target.outerTypeParameters, getEffectiveTypeArguments(node, type.target.localTypeParameters!)) :
node.kind === SyntaxKind.ArrayType ? [getTypeFromTypeNode(node.elementType)] :
- map(node.elements, element => removeMissingType(getTypeFromTypeNode(element), element.kind === SyntaxKind.OptionalType));
+ map(node.elements, element => getTypeFromTypeNode(element.kind === SyntaxKind.OptionalType && exactOptionalPropertyTypes ? (element as OptionalTypeNode).type : element));
if (popTypeResolution()) {
type.resolvedTypeArguments = type.mapper ? instantiateTypes(typeArguments, type.mapper) : typeArguments;
}I think I like this one better and I am going to push it out in a second.
There was a problem hiding this comment.
There is also a non-zero chance that this actually makes some of the existing removeMissingType calls redundant. I will investigate this.
…optional-tuple-members
|
I managed to clean some things further. Personally, I like the changes - some bits feel simpler to me now. Let me know what do you think. |
| const propertiesMapper = isTupleType(type) | ||
| ? createTypeMapper(typeParameters, sameMap(typeArguments, (t, i) => addOptionality(t, /*isProperty*/ true, !!(type.target.elementFlags[i] & ElementFlags.Optional)))) | ||
| : mapper; |
There was a problem hiding this comment.
To make the overall change smaller in scope we could "flip" the situation here. The existing mapper could be used as propertiesMapper (without the current changes in this PR it would have the missingType in the type arguments) and we could removeMissingType from the "core" mapper here. This way T in Tuple<T> wouldn't contain the missingType but properties would still be instantiated with it.
Alternatively, it's also very likely that we could add the missingType to the tuple.target elements at optional elements. This way we could just have a single mapper here and the missingType would be added to tuple properties at the instantiation time of the .target's fixed elements. Currently, target is represented as smth like [?, ?, ?] - where ? are type parameters. It could be represented as [?, ?, ? | missingType] though. I'm still battling myself how to think about it - would it be a cool trick or an obfuscated solution? 😅 The trick is that the instantiated tuple still wouldn't have missingType in its type arguments but the properties would be automatically read from the instantiated .target - so there would be a tricky mismatch between the tuple's type arguments and its .target. I'm not even sure if that's even possible - I didn't actually try to do it this way, I distinctly recall having such an idea 2 days ago when analyzing this.
That said, I still very much like that the missingType gets removed from the type arguments of the tuple. It feels right to me. The optionality bit is added by the element flag - and it's not part of the "core" type at that position. This way, It would feel a lot closer to me to how it works with object properties where the optionality gets added here. It's part of the computed property type - but not part of the declaredType there.
| @@ -16591,7 +16593,8 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker { | |||
| } | |||
|
|
|||
| function getTypeFromOptionalTypeNode(node: OptionalTypeNode): Type { | |||
| return addOptionality(getTypeFromTypeNode(node.type), /*isProperty*/ true); | |||
| const type = getTypeFromTypeNode(node.type); | |||
| return exactOptionalPropertyTypes ? type : addOptionality(type, /*isProperty*/ true); | |||
There was a problem hiding this comment.
I kinda feel that this could be taken even further. As I mentioned in the other comment - it feels to me that the type argument's type shouldn't include the optionality, even outside of EOPT. The optionality could always be added based on the combination of the type argument and the respective element flag.
I'd rather prefer to explore this as a follow-up change in another PR - if there would be an intereset for it.
| @@ -135,15 +138,24 @@ strictOptionalProperties1.ts(211,1): error TS2322: Type 'string | boolean | unde | |||
| t = [42, 'abc']; | |||
| t = [42, 'abc', true]; | |||
| t = [42, ,]; | |||
| ~ | |||
| !!! error TS2322: Type '[number, undefined]' is not assignable to type '[number, string?, boolean?]'. | |||
There was a problem hiding this comment.
This is an extra fix that just happened to be fixed by this change! :)
| t = [42, , , ,]; // Error | ||
| ~ | ||
| !!! error TS2322: Type '[number, never?, never?, never?]' is not assignable to type '[number, string?, boolean?]'. | ||
| !!! error TS2322: Target allows only 3 element(s) but source may have more. | ||
| !!! error TS2322: Type '[number, undefined, undefined, undefined]' is not assignable to type '[number, string?, boolean?]'. |
There was a problem hiding this comment.
Currently this is how it works:
const arr = [10, ,] as const; // const arr: readonly [10, never?]I think that this is an improvement though. For the end user, it feels much more readable with undefined here for the omitted expression than never - especially within the error message. I could likely bring back never here though - it's assignable to everything after all.
There was a problem hiding this comment.
Note that never only appears here with EOPT and not without it. Should those really behave differently in this regard? I'd love to learn about some compelling example of it being useful.
There was a problem hiding this comment.
The optionality that was added to all elements since the omitted expression in the array literal breaks cases like this:
// @strict: true
// @exactOptionalPropertyTypes: true
function test3(arg: [number, undefined, string]) {}
test3([10, , "foo"]); // error but it should be OK
| @@ -185,8 +197,6 @@ strictOptionalProperties1.ts(211,1): error TS2322: Type 'string | boolean | unde | |||
| const t2: [number, string?, boolean?] = [1, undefined]; | |||
| ~~ | |||
| !!! error TS2322: Type '[number, undefined]' is not assignable to type '[number, string?, boolean?]'. | |||
| !!! error TS2322: Type at position 1 in source is not compatible with type at position 1 in target. | |||
There was a problem hiding this comment.
an elaboration got lost here, it's something I will investigate
There was a problem hiding this comment.
Elaborations only occur the first time the error is generated - once that result is cached, the elaboration isn't copied to followup errors on the same types. In this case, now this comparison has the same types as the comparison with the error on line 10 of the file, so the elaboration is there now.
There was a problem hiding this comment.
Implementation-wise, this looks like a pretty solid improvement to me (reasoning about missing types in fewer places seems good to me), but I want to defer to @RyanCavanaugh for the final call on this one, since he probably has the most complete mental model for how exactOptionalPropertyTypes is supposed to work. And, specifically,
I'd rather prefer to explore this as a follow-up change in another PR - if there would be an intereset for it.
does sound interesting. It does, at least, seem like it'd help tie the optionality more strongly to the "property symbol" rather than the type, which is crucial for how exactOptionalPropertyTypes is supposed to be reasoned about (afaik).
fixes #54302