Avoid pulling object function property augmentations when resolving intersections' properties #54753
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Andarist
commented
Jun 23, 2023
sandersn
requested changes
Jul 19, 2023
Andarist
changed the title
Done. |
Andarist
commented
Jul 22, 2023
…ntersections' properties
Andarist
force-pushed
the
avoid-global-augmentations-when-relating-props-of-intersections
branch
from
9f37b5e to
6727033
Compare
sandersn
requested changes
Jul 24, 2023
src/compiler/checker.ts
Outdated
| @@ -13483,7 +13483,7 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker { | |||
| for (const current of type.types) { | |||
| for (const prop of getPropertiesOfType(current)) { | |||
| if (!members.has(prop.escapedName)) { | |||
| const combinedProp = getPropertyOfUnionOrIntersectionType(type, prop.escapedName); | |||
| const combinedProp = getPropertyOfUnionOrIntersectionType(type, prop.escapedName, /*skipObjectFunctionPropertyAugment*/ !!(type.flags & TypeFlags.Intersection)); | |||
There was a problem hiding this comment.
why is this unconditional here but has a fallback in the getPropertyOfType code?
There was a problem hiding this comment.
This resolves "own" members of the intersection so we never want to pull from function property augment here.
The other place is responsible for getting a name out of all accessible properties, it prioritizes its own ones but has to fallback to the ones from the augment to account for this:
interface Test1 { toString: null | 'string'; }
type Test2 = Test1 & { optional?: unknown };
declare const source: Test1;
const target: Test2 = { ...source };
const toString = target.toString; // own one, augment is avoided here
const hasOwn = target.hasOwnProperty; // not an own member but it should still be accessibleI pushed out this (hasOwn bit) as an extra thing in the added test case.
sandersn
approved these changes
Jul 25, 2023
|
@typescript-bot test this |
|
Heya @sandersn, I've started to run the perf test suite on this PR at d641df8. You can monitor the build here. Update: The results are in! |
|
Heya @sandersn, I've started to run the parallelized Definitely Typed test suite on this PR at d641df8. You can monitor the build here. Update: The results are in! |
|
Heya @sandersn, I've started to run the diff-based top-repos suite on this PR at d641df8. You can monitor the build here. Update: The results are in! |
|
Heya @sandersn, I've started to run the diff-based user code test suite on this PR at d641df8. You can monitor the build here. Update: The results are in! |
|
@sandersn 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
|
|
@sandersn Here are the results of running the top-repos suite comparing Something interesting changed - please have a look. Details
|
|
Hm, doesn't feel unrelated when another user test suite reports the same error in a bunch of new repos... |
|
@sandersn Here they are:
CompilerComparison Report - main..54753
System
Hosts
Scenarios
TSServerComparison Report - main..54753
System
Hosts
Scenarios
StartupComparison Report - main..54753
System
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Hey @sandersn, the results of running the DT tests are ready. Branch only errors:Package: ember/v2 Package: ember__array Package: ember__object Package: ember__array/v3 Package: ember__object/v3 Package: ember Package: ember/v3 |
|
Hm, ye - definitely something is wrong here. Especially the DefinitelyTyped errors are interesting. I'll have to investigate this further later this week. |
|
material-ui check time is slower across node versions, by 1-2%. I'm not sure what the cutoff is for slowdowns. |
|
The RWC script failed to post a comment, retrying with an updated token, but it does appear to have caused a break there. @typescript-bot test this |
|
RWC is still busted but I'll run everything and I can at least see if it failed. Really gotta get that infra replaced. @typescript-bot test this |
|
Heya @jakebailey, I've started to run the tarball bundle task on this PR at 184e1c7. You can monitor the build here. |
|
Heya @jakebailey, I've started to run the extended test suite on this PR at 184e1c7. You can monitor the build here. |
|
Heya @jakebailey, I've started to run the abridged perf test suite on this PR at 184e1c7. 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 184e1c7. 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 184e1c7. You can monitor the build here. Update: The results are in! |
|
Heya @jakebailey, I've started to run the parallelized Definitely Typed test suite on this PR at 184e1c7. You can monitor the build here. Update: The results are in! |
|
Hey @jakebailey, I've packed this into an installable tgz. You can install it for testing by referencing it in your and then running |
|
@jakebailey Here they are:Comparison Report - main..54753
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. Branch only errors:Package: ember Package: ember/v3 Package: ember/v2 Package: ember__array Package: ember__object Package: ember__array/v3 Package: ember__object/v3 |
|
@typescript-bot pack this |
|
Heya @jakebailey, I've started to run the tarball bundle task on this PR at 184e1c7. 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 |
|
There is still perf impact here that I have to investigate (and code cleanup) but it seems that now we are sitting only on the Ember-related breaks. This break can be narrowed down to this: declare class EmberObject {}
type PersonType = Readonly<typeof EmberObject> &
(new (properties?: object) => {
firstName: string;
lastName: string;
} & EmberObject) &
(new (...args: any[]) => {
firstName: string;
lastName: string;
} & EmberObject);
type PersonPrototype = PersonType["prototype"];
// ^? 5.1: any; this PR: EmberObjectSince those assertions in Ember-related tests are doing things like: assertType<string>(Person.prototype.firstName);I'd call them broken in the first place because they just rely on A mapped type (like type A = "prototype" extends keyof typeof EmberObject ? 1 : 0;
// ^? type A = 1
type B = "prototype" extends keyof Readonly<typeof EmberObject> ? 1 : 0;
// ^? type B = 1
type C = (typeof EmberObject)["prototype"];
// ^? type C = EmberObject
type D = Readonly<typeof EmberObject>["prototype"];
// ^? type D = EmberObject
type ConstructSignature = new (...args: any[]) => {
firstName: string;
lastName: string;
} & EmberObject;
type E = "prototype" extends keyof ConstructSignature ? 1 : 0;
// ^? type E = 0
type F = ConstructSignature["prototype"];
// ^? type F = any So far we established that "own" properties (not from the object/function augment) should take priority over the augment in intersections. Given this desired semantic, this break looks desired to me. WDYT @sandersn ? cc @chriskrycho @wagenet @gitKrystan @jamescdavis (Ember's typedef owners) |
|
I can verify that the existing tests are broken. Types for EmberObject are significantly limited and the class will be going away entirely in the future so we were unlikely to actually fix those anyway. |
|
To check my understanding:
If the only breaks are in old versions of Ember, I'd guess that this pattern is not that common and is OK to not support (either correctly, or by inadvertent |
Yes.
Yes, that's probably what would have to be done. It would probably be good to break this down into some cases that might be desirable/practical. Do you have anything in particular in mind?
Yes, my bad - it doesn't change the result though.
Should I assume then that this PR is something that you want to pull in and that I should investigate how to improve the perf? |
Yes. I don't think it's worth pursuing special case |
|
Just got back from vacation and saw this – yeah, given the existing reliance on The reason I want to avoid breaking that is: a lot of real-world (legacy, but real) code relies on it, including code that makes other major Ember packages work. Ember's just-shipped-last-month official types don't support that at all, and we're using the DT types as the fallback for folks who are working through that transition. But from what I can tell, this should still be fine? Assuming so, |
…ps-of-intersections
sandersn
added a commit
to sandersn/DefinitelyTyped
that referenced
this pull request
Nov 30, 2023
Ember is broken by microsoft/TypeScript#54753 The discussion on this PR ending at microsoft/TypeScript#54753 (comment) makes me think that these breaks are acceptable where they aren't strict improvements.
sandersn
added a commit
to DefinitelyTyped/DefinitelyTyped
that referenced
this pull request
Dec 1, 2023
* Update ember for TS 5.4 Ember is broken by microsoft/TypeScript#54753 The discussion on this PR ending at microsoft/TypeScript#54753 (comment) makes me think that these breaks are acceptable where they aren't strict improvements. * update ExpectType for ember__array/v3 * dprint fmt
Andarist
deleted the
avoid-global-augmentations-when-relating-props-of-intersections
branch
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
fixes #54345