-
Notifications
You must be signed in to change notification settings - Fork 12.4k
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
Pad object literal types with implicit anys based on binding pattern elements #59924
base: main
Are you sure you want to change the base?
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 |
fixes #59920 (a regression)