-
Notifications
You must be signed in to change notification settings - Fork 12.8k
Pad object literal types with implicit anys based on binding pattern elements #59924
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
@typescript-bot test it |
@jakebailey Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
@jakebailey, I have made local WIP improvements for nested patterns. However, there are some pre-existing inconsistencies between array and binding patterns. It would be great if we could establish the desired behavior as that heavily impacts how I should implement this. Let's take a look at this (TS 5.5 playground): function testObj({ a, b, c = 1 } = { a: 2 }) {} // implicit any on `b`, ok
testObj({ a: 3 }); // error, missing `b`
function testArr([a, b, c = 1] = [2]) {} // implicit any on `b`, ok
testArr([3]); // no error As we can see above, the optionality is different for those members with inferred implicit anys It might be easier to make those properties optional if we don't want to revert parts of #59183 . the parts mentioned there as "strictly not necessary for the fix" |
@@ -6010,7 +6010,6 @@ export interface SymbolLinks { | |||
exportsChecked?: boolean; // True if exports of external module have been checked | |||
typeParametersChecked?: boolean; // True if type parameters of merged class and interface declarations have been checked. | |||
isDeclarationWithCollidingName?: boolean; // True if symbol is block scoped redeclaration | |||
bindingElement?: BindingElement; // Binding element associated with property symbol |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like you're introducing a change to the public API surface area. If this includes breaking changes, please document them on our wiki's API Breaking Changes page. Also, please make sure @DanielRosenwasser and @RyanCavanaugh are aware of the changes, just as a heads up. |
@@ -8750,7 +8750,6 @@ declare namespace ts { | |||
function isJSDocCommentContainingNode(node: Node): boolean; | |||
function isSetAccessor(node: Node): node is SetAccessorDeclaration; | |||
function isGetAccessor(node: Node): node is GetAccessorDeclaration; | |||
/** True if has initializer node attached to it. */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this one was copied over from an internal function declared above it, I don't think this comment is true as it made much more sense for the other function
I pushed out changes to make those members with inferred implicit anys optional. Today they must be optional because the initializer's type is no longer "padded" (since #59183 ). Given this example: // @strict: true
function test({ a } = {}) {} We get
This doesn't feel right. So I've decided to make those implicit anys optional. This allows the non-padded initializer's type to stay assignable to the padded parameter type. Based on my previous comment I concluded that array and binding patterns were inconsistent and that it would be nice ot unify their behaviors. However, in the process of working on this, I discovered #4598 and the description of this PR indicates that they were not even meant to behave identically. This was in 2015 though, perhaps some adjustments could be made today. Either way, to fix the reported problem we can:
|
PR was changed, so rerun @typescript-bot test it |
@jakebailey Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
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 Something interesting changed - please have a look. Details
|
@jakebailey Here are the results of running the top 400 repos with tsc comparing Something interesting changed - please have a look. Details
|
I believe the latest commit in my other PR addresses the issues above. So that other one would have to land before this one here |
function pushCachedContextualType(node: Expression) { | ||
pushContextualType(node, getContextualType(node, /*contextFlags*/ undefined), /*isCache*/ true); | ||
} | ||
|
||
function pushContextualType(node: Expression, type: Type | undefined, isCache: boolean) { | ||
function pushContextualType(node: Node, type: Type | undefined, isCache: boolean) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this be a narrower type than just Node?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With this implementation, it could be Expression | BindingPattern | BindingElement
if you'd prefer that.
2 extra notes that come to my mind now:
- maybe I shouldn't be reusing the same
contextualTypes
stack for this and I should just introduce a new one? It seemed fine here to avoid creating a new but then reusinggetContextualType
wasn't possible so it's slightly weird now that I'm only using part of the existing pattern . Perhaps with a new context flag it would be possible but introducinggetContextualPaddingType
seemed easier - if this gets approved, it might be a good idea to explore expanding
getContextualPaddingType
with support for array patterns and usingpushContextualType
/popContextualType
inpadTupleType
. Since this PR tries to address a regression in a backportable manner I refrained from doing this to avoid any new unforeseen behavioral changes
It sure seems weird for that tuple case to pass, other than that maybe the type is inferred as @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 |
I'm not sure which one is function testArr([a, b, c]?: [number, any?, (number | undefined)?]): void |
Oh, I guess it could be undefined, yeah. |
Closing as per #59920 (comment) |
fixes #59920 (a regression)