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

Common properties check:Fix intersection-parent check #34646

Closed

Conversation

sandersn
Copy link
Member

The check for common properties ("weak type check") when the target is an intersection is supposed to skip the common property check for constituents and instead check assignability of properties for the entire type. The current code correctly skips the check for constituents, but fails to check the whole intersection sometimes. That's because the whole-intersection check uses isPerformingCommonPropertyChecks, but that reports the state of the intersection itself, not its constituents.

This PR changes the boolean flag isApparentIntersectionConstituent to a box, { skipped: boolean }, that lets children report whether they skipped the common property check or not.

The meat of the PR is in the first page or so; everything else is just changing the type of isApparentIntersection.

This PR is not done:

  1. I'm not sure this approach is correct. It might be better to always run the whole-intersection property check, even when no constituent is weak.
  2. I haven't cleaned up the parameter-passing code; in particular, I didn't introduce a type alias for the result box, and most calls create a box and then don't look at the result.
  3. The new, weaker isPerformingExcessPropertyChecks and isPerformingCommonPropertyChecks need to be checked with !isApparentIntersection, but probably not everywhere.

And I don't attempt to fix known problems with the existing approach, like the caching problem or the fact that source intersections also have their children skip common property checks.

Fixes #33944

@sandersn sandersn changed the title Common props check:Fix intersection-parent check Common properties check:Fix intersection-parent check Oct 22, 2019
@sandersn
Copy link
Member Author

I'd like some commentary from others on the approach before I clean this up.

@sandersn
Copy link
Member Author

sandersn commented Oct 22, 2019

I updated a failing baseline, which I believe is correct. I can't tell if it's desirable but I suspect not:

export function myHigherOrderComponent<P>(Inner: React.ComponentClass<P & {name: string}>, props: Readonly<P>) {
    <Inner {...props} name="Matt"/>;
     ~~~~~
!!! error TS2326: Types of property 'name' are incompatible.
!!! error TS2326:   Type '"Matt"' is not assignable to type 'P["name"] & string'.
!!! error TS2326:     Type '"Matt"' is not assignable to type 'P["name"]'.
}

The error comes from assigning

Readonly<P> & { name: "Matt" }

   ==>

IntrinsicAttributes 
& IntrinsicClassAttributes<Component<P & { name: string; }, any, any>> 
& Readonly<{ children?: ReactNode }> 
& Readonly<P & { name: string }>

Previously the common property check was skipped, correctly, for all constituents of the target. The whole-intersection check was also skipped, incorrectly, since the whole intersection was not a weak type because of { name: string }.

However, now that the constituents record when they skip a common property check, the whole-intersection check runs and checks that the four properties of the target are assignable from the source. This is true for key,ref,children, but not for name, which has type "Matt" in the source but (P & { name: string })['name'] in the target.

As far as I can tell, this hole is required for React HOCs to compile, because the error seems correct to me.
Edit: Well, except that Readonly<P & { name: "Matt" } is assignable to Readonly<P & { name: string }>. So maybe the whole-intersection check just needs to cover more of assignability than just propertiesRelatedTo.

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
Copy link
Member Author

#34789 is a better fix

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
@weswigham
Copy link
Member

@sandersn should this be closed now?

@sandersn sandersn closed this Oct 30, 2019
@sandersn sandersn deleted the fix-common-property-intersection-parent-check branch October 30, 2019 20:16
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.4 Partial field matching does not work for conditional type
2 participants