Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upImproved checking of destructuring with literal initializers #4598
Conversation
What if the extra binding elements are not initialized? Shouldn't that be an error? function bar([x, y = 0] = []) { } // Should error
function bar2([x, y = 0] = [0]) { }
bar2([]); // should error |
|
|
||
| // (arg: [number, number]) => void | ||
| function g2([x = 0, y = 0]) { } | ||
| g2([1, 1]); |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
JsonFreeman
Sep 2, 2015
Contributor
Right, ideally they wouldn't be errors, but it would require optional tuple elements or some other mechanism to allow them.
| g2([1, 1]); | ||
|
|
||
| // (arg?: [any, any]) => void | ||
| function g3([x, y] = []) { } |
This comment has been minimized.
This comment has been minimized.
JsonFreeman
Sep 2, 2015
Contributor
Why is [] assignable to [any, any]?
Oh, because the [] expression is contextually typed, which means its type will be [any, any]? I don't think that makes sense unless those binding elements are initialized.
This comment has been minimized.
This comment has been minimized.
ahejlsberg
Sep 2, 2015
Author
Member
It shouldn't matter if the elements are initialized. A missing initializer just means the variable is of type any and has the default value undefined.
This comment has been minimized.
This comment has been minimized.
| // (arg?: [number, number]) => void | ||
| function g4([x, y] = [0, 0]) { } | ||
| g4(); | ||
| g4([1, 1]); |
This comment has been minimized.
This comment has been minimized.
JsonFreeman
Sep 2, 2015
Contributor
Some more cases:
function g5([ x, y = 0 ] = [0]) { }
g5();
g5([0]);
g5([0, 0]);
function g6([x = 0, y = 0] = []) { }
g6();
g6([]);
g6([0]);
g6([,0]);
g6([0, 0]);
This comment has been minimized.
This comment has been minimized.
JsonFreeman
Sep 2, 2015
Contributor
g5([0, ""]); // Should error now
g6([""]); // Should error now
g6(["", ""); // Should error now
This comment has been minimized.
This comment has been minimized.
JsonFreeman
Sep 2, 2015
Contributor
There's also the idea of making optional the elements whose binding elements have default initializers, even if they are present in the top level initializer:
function g7([x = 0, y = 0] = [0, 0]) { }
g7([]); // Should work because first and second element should be optional
g7([0]); // Should work because second element should be optional
g7([0, ""]); // Should not work because type is wrong for second element| @@ -6607,12 +6606,18 @@ namespace ts { | |||
| } | |||
| } | |||
| if (isBindingPattern(declaration.name)) { | |||
| return getTypeFromBindingPattern(<BindingPattern>declaration.name); | |||
| return createImpliedType(getTypeFromBindingPattern(<BindingPattern>declaration.name)); | |||
This comment has been minimized.
This comment has been minimized.
JsonFreeman
Sep 2, 2015
Contributor
So this step applies both for top level initializers as well as initializers nested in binding patterns?
This comment has been minimized.
This comment has been minimized.
ahejlsberg
Sep 2, 2015
Author
Member
Yes, it applies generally. The level of nesting shouldn't matter.
This comment has been minimized.
This comment has been minimized.
| @@ -7157,6 +7183,17 @@ namespace ts { | |||
| propertiesArray.push(member); | |||
| } | |||
|
|
|||
| // If object literal is contextually typed by the implied type of a binding pattern, augment the result | |||
| // type with those properties for which the binding pattern specifies a default value. | |||
| if (contextualType && contextualType.flags & TypeFlags.ImpliedType) { | |||
This comment has been minimized.
This comment has been minimized.
JsonFreeman
Sep 2, 2015
Contributor
It would be good to add tests that demonstrate that this can cause an error. Something like this, which I don't believe would have errored before:
function foo({ a = 0 } = {}) { }
foo({ a: "" });Basically something where the property was dropped in the initializer, but then was present again in the argument with the wrong type.
This comment has been minimized.
This comment has been minimized.
ahejlsberg
Sep 2, 2015
Author
Member
It did error before (complaining that the object literal is missing property a), but it shouldn't have.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
| // If array literal is actually a destructuring pattern, mark it as an implied type. We do this such | ||
| // that we get the same behavior for "var [x, y] = []" and "[x, y] = []". | ||
| if (inDestructuringPattern && elementTypes.length) { | ||
| return createImpliedType(createTupleType(elementTypes)); |
This comment has been minimized.
This comment has been minimized.
JsonFreeman
Sep 2, 2015
Contributor
Don't you have to do this for object literals too (the ones that happen to be assignment patterns)? Also, I don't think there are any tests for assignment patterns, just binding patterns.
This comment has been minimized.
This comment has been minimized.
JsonFreeman
Sep 2, 2015
Contributor
Wait, actually maybe you did not mean to do that. Here you are creating an implied type for an assignment pattern. But I think only binding patterns can imply types.
This comment has been minimized.
This comment has been minimized.
ahejlsberg
Sep 2, 2015
Author
Member
If we want to allow var { x, y } = {} and the corresponding assignment { x, y } = {} then we'd have to do it for object literals as well.
This comment has been minimized.
This comment has been minimized.
JsonFreeman
Sep 2, 2015
Contributor
I kind of think that for parity with arrays, it's nice to do it for object literals too. Though I realize that the main rationale for arrays is to make the right hand side a tuple.
| if (contextualType && contextualTypeIsTupleLikeType(contextualType) || inDestructuringPattern) { | ||
| return createTupleType(elementTypes); | ||
| let contextualTupleLikeType = contextualType && contextualTypeIsTupleLikeType(contextualType) ? contextualType : undefined; | ||
| if (contextualTupleLikeType) { |
This comment has been minimized.
This comment has been minimized.
JsonFreeman
Sep 2, 2015
Contributor
I think it's simpler to just do
if (contextualType && contextualTypeIsTupleLikeType(contextualType)) {
...
}And just refer to contextualType inside. Essentially just inline the condition
This comment has been minimized.
This comment has been minimized.
| if (contextualTupleLikeType.flags & TypeFlags.Tuple && contextualTupleLikeType.flags & TypeFlags.ImpliedType) { | ||
| let contextualElementTypes = (<TupleType>contextualTupleLikeType).elementTypes; | ||
| for (let i = elementTypes.length; i < contextualElementTypes.length; i++) { | ||
| elementTypes.push(contextualElementTypes[i]); |
This comment has been minimized.
This comment has been minimized.
JsonFreeman
Sep 2, 2015
Contributor
Again, maybe I'm missing something. But it seems like only initialized properties should be copied, otherwise you allow default initializers that are too short to satisfy the destructuring.
|
Can you add the case from #2469 as a test? |
|
I think it may be challenging to get the array portion correct because there is no concept of optional tuple elements. It seems like that would be required to make array patterns work in a way that parallels how object patterns work in this change. |
|
@JsonFreeman Thanks for the comments. You're right, some of this is complicated by the fact that tuple types don't support optional elements, but I don't think it is too big of an issue. Certainly I wouldn't advocate adding optional tuple elements as a full blown feature. On multiple occasions I've seen the pattern of supplying an empty array literal as the default value for an array binding pattern. For example: function f1([x, y, text] = []) { } // f1(arg?: [any, any, any])
function f2([x = 0, y = 0, text] = []) { } // f2(arg?: [number, number, any])
function f3([x, y, text = ""] = []) { } // f3(arg?: [any, any, string])
function f4([x = 0, y = 0, text = ""] = []) { } // f4(arg?: [number, number, string])The emply array literal serves to indicate that the argument may be omitted and, when it is, the destructuring parameters get their default value or We currently only allow the same pattern with an empty object literal if the corresponding object binding pattern supplies a default values for each variable. However, I'm thinking of a slight tweak where if a binding variable has neither a default value in the object binding pattern nor a property specified in the object literal initializer, we add an optional property of type function f1({ a, b, c } = {}) { } // f1(arg?: { a?: any, b?: any, c?: any })
function f2({ a, b = "", c } = { a: 0 }) { } // f2(arg?: { a: number, b?: string, c?: any })I think this would complete the picture nicely. |
|
I'm thinking it would also make sense to report an error if an object literal initializer for a destructuring pattern includes a property that isn't listed in the destructuring pattern. For example: var { x, y } = { x: 0, y: 0, z: 0 }; // Should be an errorWe permit this today, but I can't think of any good reason why we should. |
|
Thanks @ahejlsberg. I agree with your notion that excess properties in the initializer, that are not mentioned in the object binding pattern, should be errors. It's an indication that the user probably made a spelling error. Interestingly, I think this could be achieved by requiring that the type of the initializer be assignable to the implied type of the binding pattern (the initializer is a fresh object literal, so it would fail assignability if it had extra properties). By the same token, you could make it an error if an array literal that initializes an array binding pattern is longer than the binding pattern. The extra elements would never be used. Regarding your analysis at #4598 (comment), what you are saying makes sense, but I have a few concerns:
|
Correct, we're more permissive in a destructuring of an array literal vs. a regular assignment of an array literal. Given that we want to support the
That seems reasonable. I think it boils down to only allowing a property with no default value in the contextual type to be omitted if it has type |
When would you do that? When you compute the contextual (implied) type? I thought it would be easier to do it when you process the destructuring and finally assign types to the binding elements. If you wait till then, you can explicitly only allow one level of destructuring for elements that are missing. The other reason to wait till the destructuring itself is that we've always said contextual typing should not cause errors. My reading of your suggestion is that it may cause an error during contextual typing itself. |
|
Additionally, if you allow a one level destructuring for missing elements, then you don't really have to copy any elements over to the initializer for the array binding pattern case. The type of the initializer can be exactly as it was before, and the treatment of the destructuring operation would be able to handle the missing elements. |
|
In terms of being more lenient for destructuring an array versus just assigning an array, I think it sounds odd to me, but you could make a case for it. I think the key is that I have to think of the initializer as determining the type of the parameter, while an argument would not. In this way, the initializer has more authority because when it's processed, the type of the parameter position hasn't been finalized yet. |
This comment has been minimized.
This comment has been minimized.
|
What about ObjectLiteral as an assignment pattern? |
This comment has been minimized.
This comment has been minimized.
|
That's handled in a later commit. |
This comment has been minimized.
This comment has been minimized.
|
Why is it useful to copy over the types faithfully? Isn't the purpose just to pad the length, so type |
This comment has been minimized.
This comment has been minimized.
|
Looking at the baselines, I see why the undefined is useful here. It errors on further destructuring. What is the purpose of copying the element type from the contextual type? Couldn't the element's initializer inside the binding pattern just supply the type of the element directly, instead of being copied into the type of the parent initializer? |
This comment has been minimized.
This comment has been minimized.
|
Correct on function foo([x, y = 1] = [0]) { }Here, the final type |
This comment has been minimized.
This comment has been minimized.
|
Got it. |
This comment has been minimized.
This comment has been minimized.
|
This is always the binary expression case of hasDefaultValue, right? |
This comment has been minimized.
This comment has been minimized.
|
Yes. |
This comment has been minimized.
This comment has been minimized.
|
Instead of marking the contextual prop optional here, wouldn't it be simpler to just do it in the initializer? Instead of copying the optional flag, you could check if the contextual type's pattern's corresponding property has an initializer. |
This comment has been minimized.
This comment has been minimized.
|
Basically here you could check the relevant property of |
This comment has been minimized.
This comment has been minimized.
|
Right, I like the way I have it now. |
This comment has been minimized.
This comment has been minimized.
|
Probably good to add a test where you use the suppressExcessPropertyErrors flag. |
|
@JsonFreeman Again, thanks for the careful review.
Yes, but since we allow the array literal to have fewer elements than the binding pattern, it seems consistent to also allow it to have more.
For object literals the intuition is that elements can't be omitted unless they have a default value (i.e. unless they're optional). This is consistent with our assignment compatibility rules and I think it harmonizes well with our excess property check (i.e. you can't have excess properties on either side unless there's a default involved). If anything we should more strictly check that array binding patterns and array literals have matching lengths, but I actually like the looser rules for arrays. After all, the minute you have rest or spread elements involved, we can't accurately check lengths. And, whereas we may think of something as a tuple, the user might actually think of it as an array. |
Fewer and more mean different things though. Fewer elements than the binding pattern means that some elements will become undefined or error (depending on whether the element is destructured further). More means that some values will be unused, not assigned anywhere. To me, consistency between those concepts isn't super meaningful.
That is fair, although I tend to think of destructuring as basically a declarative syntax for property accesses. So it seems more similar to a property access with subsequent assignments, rather than one big assignment. But I realize it's sort of halfway in between.
I realize there are certain cases that may be more common for arrays, but what principle dictates that the rules for arrays should be looser? I understand looseness with rest elements because we don't know what indices we are dealing with, but I think array destructuring is better treated tuple-wise unless there is some reason not to. I think the line between tuples and arrays is rather fuzzy, and it makes sense to think about what principles govern whether something is treated as an array versus a tuple. A few more thoughts I've had: I think there are a lot of concepts we are discussing here that are kind of independent, and may be good topics for the design meeting as separate topics. I can think of at least three:
Again, I think it is useful to discuss them separately at the design meeting. In particular, what is the intended behavior, and what are the principles driving those decisions? By principles, I mean for example that object destructuring an array destructuring should be consistent except where there is a fundamental difference that needs to be accounted for. That may not be one of the principles you used, but it is one that I was assuming, and may not be as important as I am making it. Once those principles are nailed down, it will be easier to review the language design mechanisms and the implementation strategies involved. I think it is easier than thinking about them all at the same time. One more point. Now that we have flags to track whether certain types came from literals, is it really necessary/better to use contextual typing as a device to make certain values assignable? Would it make more sense to just build those rules into assignability? For example, the excess property check on fresh object literals (non-destructuring) is done in assignability with no aid from contextual typing. It's not clear to me what determines whether contextual typing is the chosen vehicle. |
|
To summarize what we have now, it seems that for object destructuring, the rules are just as before except for two changes:
For array destructuring:
|
It's the fact that the same syntax is used for both arrays and tuples, and sometimes what we deem a tuple was actually intended by the programmer to be an array. For example: var [x, y] = []; // We treat this as a tuple
var [x, y, ...z] = []; // We treat this as an arrayIf we did strict length checking, the first line above would become an error but the second line would still be fine. Now, you could argue the second line should also be an error, but then we'd have to start reasoning about hybrid tuple/array like types. And it's just not clear we're solving anyone's problem--after all, the above is perfectly valid JavaScript. We may discuss your issues in a design meeting. Meanwhile, here's my take on them:
They should be errors for object destructuring, consistent with our excess property error checks. They should perhaps be errors for arrays, depending on whether we think it is fine to allow fewer elements, but not more.
Elements that are absent in the initializer can be destructured only if they specify a default value. This is true only for array and object literal initializers, not for non-literals (if a property is missing in the static type of a non-literal, the property may actually be present at run-time, but we have no idea what its type will be). No
Yes. I agree with your reasoning in #2469 and that's how it is implemented now.
We could build them into assignability, but we'd still have to construct the combined contextual/literal type as I explain here. By "adapting" the literal type according to the contextual type, we basically solve both problems in one place. |
|
@ahejlsberg Thanks for the responses. I think you identified the key question about arrays versus tuples - how do we really know what the user intended? Our original stance was that the presence of a tuple type literal, or an array destructuring (without a rest) meant a tuple type, and I feel like now there is some uncertainty about whether an array destructuring (without a rest) really indicates that the type should be treated as a tuple. So it may be a good opportunity to discuss whether that was a good assumption, and whether / where it still holds. I think all of your points make sense, and it was helpful to see the thought process. I think it is a reasonable place to land, and am curious what others think. |
|
@mhegazy @danquirk @RyanCavanaugh @DanielRosenwasser @vladima @JsonFreeman I have updated the initial description of this feature with the currently implemented behavior. There is one final question I'd like some feedback on. Let me know what you think. |
|
@ahejlsberg Regarding your question, there is one more option, also possibly not worth the effort:
Going back to the options you listed, I just want to clarify two things:
I like option 1, that only allows omitting the element when the binding pattern is initialized. The odd thing about option 2 is that there are array literals that are valid as initializers, but not valid as arguments. And option 3 is not bad, though depending on the answer to my question 2 above, it may have the nontransitivity property that we've seen with optionals: var a: [number, string] = [0, ""];
var b: [number] = a;
var c: [number, number] = b;I think options 1 and 3 are best. One attractive thing about 3 in terms of implementation is that all the information is present in the contextual type, and you don't have to pack along a pattern as an auxiliary store of information. |
Correct.
Also correct. It generally isn't safe to allow a shorter tuple type to be assigned to a longer tuple type (because, as your example illustrates, we don't know for sure that the missing elements are really missing at run-time). However, when the shorter tuple is an array literal we do know for sure there are no extra elements and the assignment is actually safe. I agree with you that option 1 and 3 are the most attractive. I'm actually leaning in favor of 3 as it is more consistent across the board. |
|
I researched implementing option 3. It turns out to have wider ranging effects on overloading because tuple literals with fewer elements now become assignable to tuple types with more elements. It's not impossible to go that route, but I think it is beyond the scope of this PR and something we could do later if we think it is necessary. My latest commits implement option 1 and furthermore improves error reporting. Specifically, for code like the following: var [x, y] = [];
var { x, y } = {};we now report the error message "Initializer provides no value for this binding element and the binding element has no default value" on x and y in both examples. I think this takes care of all the feedback, so check out the latest commits and give me a thumbs up if you're good with this. |
| function f1() { | ||
| var { x, y } = {}; | ||
| ~ | ||
| !!! error TS2525: Initializer provides no value for this binding element and the binding element has no default value |
This comment has been minimized.
This comment has been minimized.
|
|
Improved checking of destructuring with literal initializers


ahejlsberg commentedSep 1, 2015
Fixes #2469.
This PR makes the checking of destructuring patterns with an object literal or array literal initializers more flexible.
When an object literal is contextually typed by the implied type of an object binding pattern:
When an array literal is contextually typed by the implied type of an array binding pattern:
Some examples:
In the
f3example, note that the initializer for the parameter binding pattern is allowed to omit elements (and, in particular, is allowed to be an empty array literal). This is a common pattern that we want to support. However, we still require tuples to specify all elements in other cases, as is demonstrated by the calls tof3.UPDATE 1: A question remains about how far we go in allowing tuple literals to omit elements. Possible rules are:
function foo([x = 0, y = 0] = []). Since that pattern is common, this is the minimum bar.function foo([x, y] = []). We allow this currently, but we could say this is an error. That, however, would perhaps be inconsistent withfunction foo([x, y, ...z] = []), which we allow because spread and rest elements cause us to infer arrays instead of tuples.var x: [number, number]allow the assignmentx = []. This would effectively amount to saying that tuple elements are always optional, i.e. we don't guarantee they're present, but we do guarantee they have the right type when they are present.UPDATE 2: The PR implements option 1 above.