-
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
Explicitly Omit unspreadable properties from rest type in the generic case #47078
Conversation
@typescript-bot pack this |
Heya @jakebailey, I've started to run the parallelized community code test suite on this PR at f2bb9b7. You can monitor the build here. |
Heya @jakebailey, I've started to run the tarball bundle task on this PR at f2bb9b7. 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 |
The user suite test run you requested has finished and failed. I've opened a PR with the baseline diff from master. |
This is good; we're "allowed" to have types which don't represent the full list of properties present at runtime (this is the same logic by which |
Good, less work for me! 😄 |
Updated the PR to also test |
This is less about |
Yes, this is related to property spreading of a value with unspreadable properties; |
@jakebailey But this still breaks on runtime if you uses interface that are implemented with a class, right? interface Thing {
someProperty: number;
someMethod(): void;
}
function foo<T extends Thing>(x: T) {
let { someProperty, ...rest } = x;
// Not an error, but will break on runtime with: foo(new MyThing());
rest.someMethod();
}
class MyThing implements Thing {
someProperty = 42;
someMethod() {
// ...
}
}
// fine
foo({ someProperty: 1, someMethod: () => { } });
// breaks on runtime
foo(new MyThing()); |
Sure, but that code doesn't error before this PR too. I'd file a new issue for that (though there's probably an issue somewhere about how these sorts of own property semantics aren't fully modeled). |
Hello everyone! Thank you for your changes @jakebailey, I really appreciate it! I have one question out of curiosity: if I have a constructor where I bind methods, they should be allowed for the rest type, right? At the moment I see error that Playground – https://tsplay.dev/mMMebm Exampleclass Thing {
someProperty = 42;
constructor() {
this.someMethod = this.someMethod.bind(this);
}
someMethod() {
// ...
}
}
function foo<T extends Thing>(x: T) {
let { someProperty, ...rest } = x;
// Used to work, is now an error!
// Property 'someMethod' does not exist on type 'Omit<T, "someProperty" | "someMethod">'.
rest.someMethod();
} Should I create a separate issue for it? @RyanCavanaugh |
I would file a new issue (as this is the wrong place to discuss it), but I'm not sure that we do anything special for a method bound like that. Check out the This is no different than how we've always behaved in the non-generic case: Playground Link This PR made things consistent between the two. |
Fixes #46967
In the non-generic case, we'd produce a concrete type that consisted only of the spreadable properties, excluding already destructured and unspreadable properties. This is good.
However, in the generic case (including
this
, as it's an implicit type parameter), we instead produced anOmit
type, but only omitted the already destructured keys. This left a type where all of the unspreadable properties (e.g. properties with getters, methods) where left accessible.Make the generic case match the behavior of the non-generic case by also adding the unspreadable items to the omitted set of keys.
This might get pretty hairy for types with lots of getters/setters/methods, though, but the alternative (making a concrete type instead if it looks like
Omit
can't handle it) appears to break a few tests in even more undesirable ways. Hopefully destructuringthis
with a rest is not all that common (the user tests imply it is not).Additionally, I don't know if the behavior on non-public properties is correct; it seems weird that destructuring
this
within the class would only keep the public properties, even though at runtime they are there. They're dropped in theT extends SomeClass
case because that's a use where those symbols are not intended to be visible, but internally, maybe it's not right to drop them.Before: Playground Link
After: Playground Link