Skip to content
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

someTypeRelatedToType now passes isIntersectionConstituent #33213

Merged
merged 3 commits into from
Sep 4, 2019

Conversation

sandersn
Copy link
Member

@sandersn sandersn commented Sep 3, 2019

The quick check for intersection sources doesn't pass isIntersectionConstituent, which incorrectly causes excess property checking to happen on intersection constituents.

In this example, the quick check happens when checking g(CC), and incorrectly finds that CP is not assignable to { children?: boolean } even though the latter is part of CP & { children?: boolean }. The result is then cached, which breaks the assignability check on the next line: <CC {...(null as CP) }/>.

This is a 3.6 regression, but the example is so complex that I'm not sure how hard it is to reproduce.

interface F<P> {
    (props: P & { children?: boolean }): void;
    propTypes: { [K in keyof P]: null extends P ? K : K };
}
declare function g(C: F<unknown>): string;
export function wu<CP extends { o: object }>(CC: F<CP>) {
    class WU {
        m() {
            g(CC)
            return <CC {...(null as unknown as CP)} />;
        }
    }
}

Fixes #33133

Edit: Added commentary from the issue:

The basic problem is that the flag isIntersectionConstituent, which is used to exempt intersection constituents from common-property checks ("weak types"), isn't threaded through one particular intersection call in assignability checking. I missed it when threading that flag through more calls in 3.6 (#32582), and my new assertion in overload error reporting caught the mistake. It's quite likely that this assert would have fired a lot more before #32582.

@sandersn sandersn requested a review from weswigham September 3, 2019 16:41
@@ -22018,7 +22018,7 @@ namespace ts {
}
}
else {
const allDiagnostics: DiagnosticRelatedInformation[][] = [];
const allDiagnostics: Array<ReadonlyArray<DiagnosticRelatedInformation>> = [];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const allDiagnostics: Array<ReadonlyArray<DiagnosticRelatedInformation>> = [];
const allDiagnostics: (readonly DiagnosticRelatedInformation[])[] = [];

to fix that lint failure.

Copy link
Member Author

@sandersn sandersn Sep 4, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Our parsing for readonly T[][] is so bad, and yet we have a lint rule requiring it.

@sandersn sandersn added this to the TypeScript 3.6.3 milestone Sep 4, 2019
@sandersn sandersn merged commit 8ca36f3 into master Sep 4, 2019
@sandersn
Copy link
Member Author

sandersn commented Sep 4, 2019

@typescript-bot cherry-pick this to release-3.6

typescript-bot pushed a commit to typescript-bot/TypeScript that referenced this pull request Sep 4, 2019
Component commits:
cc1dc3b someTypeRelatedToType now passes isIntersectionConstituent

cf3eadc Merge branch 'master' into fix-missed-intersection-constituent-threading

f10fe38 Fix [][] lint
@typescript-bot
Copy link
Collaborator

Hey @sandersn, I've opened #33245 for you.

@sandersn sandersn deleted the fix-missed-intersection-constituent-threading branch September 4, 2019 20:42
sandersn pushed a commit that referenced this pull request Sep 4, 2019
Component commits:
cc1dc3b someTypeRelatedToType now passes isIntersectionConstituent

cf3eadc Merge branch 'master' into fix-missed-intersection-constituent-threading

f10fe38 Fix [][] lint
sandersn added a commit that referenced this pull request Sep 5, 2019
weswigham added a commit to weswigham/TypeScript that referenced this pull request Sep 5, 2019
timsuchanek pushed a commit to timsuchanek/TypeScript that referenced this pull request Sep 11, 2019
…#33213)

* someTypeRelatedToType now passes isIntersectionConstituent

* Fix [][] lint
sandersn added a commit that referenced this pull request Oct 28, 2019
isIntersectionConstituent controls whether relation checking performs
excess property and common property checks. It is possible to fail a
relation check with excess property checks turned on, cache the result,
and then skip a relation check with excess property checks that would
have succeeded. #33133 provides an example of such a program.

Fixes #33133 the right way, so I reverted the fix at #33213
Fixes #34762 (by reverting #33213)
Fixes #33944 -- I added the test from #34646
sandersn added a commit that referenced this pull request Oct 29, 2019
* Add isIntersectionConstituent to relation key

isIntersectionConstituent controls whether relation checking performs
excess property and common property checks. It is possible to fail a
relation check with excess property checks turned on, cache the result,
and then skip a relation check with excess property checks that would
have succeeded. #33133 provides an example of such a program.

Fixes #33133 the right way, so I reverted the fix at #33213
Fixes #34762 (by reverting #33213)
Fixes #33944 -- I added the test from #34646

* Update comments in test
typescript-bot pushed a commit to typescript-bot/TypeScript that referenced this pull request Oct 29, 2019
Component commits:
2e0b451 Add isIntersectionConstituent to relation key
isIntersectionConstituent controls whether relation checking performs
excess property and common property checks. It is possible to fail a
relation check with excess property checks turned on, cache the result,
and then skip a relation check with excess property checks that would
have succeeded. microsoft#33133 provides an example of such a program.

Fixes microsoft#33133 the right way, so I reverted the fix at microsoft#33213
Fixes microsoft#34762 (by reverting microsoft#33213)
Fixes microsoft#33944 -- I added the test from microsoft#34646

14d7a44 Merge branch 'master' into add-isIntersectionConstituent-to-relation-key

ea80362 Update comments in test

0764275 Merge branch 'master' into add-isIntersectionConstituent-to-relation-key
sandersn pushed a commit that referenced this pull request Oct 29, 2019
Component commits:
2e0b451 Add isIntersectionConstituent to relation key
isIntersectionConstituent controls whether relation checking performs
excess property and common property checks. It is possible to fail a
relation check with excess property checks turned on, cache the result,
and then skip a relation check with excess property checks that would
have succeeded. #33133 provides an example of such a program.

Fixes #33133 the right way, so I reverted the fix at #33213
Fixes #34762 (by reverting #33213)
Fixes #33944 -- I added the test from #34646

14d7a44 Merge branch 'master' into add-isIntersectionConstituent-to-relation-key

ea80362 Update comments in test

0764275 Merge branch 'master' into add-isIntersectionConstituent-to-relation-key
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3.6 regression: Error: Debug Failure. No error for last overload signature
3 participants