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

Reinterpret a type parameter constrained to any as an upper bound constraint #29571

Merged
merged 5 commits into from
Mar 3, 2020

Conversation

weswigham
Copy link
Member

Fixes #29509

Note, that we have a test that asserts that a type parameter explicitly extending any behaves like any when accessing its members (which IMO is strange). This would be a break to that behavior.

@weswigham weswigham added the Breaking Change Would introduce errors in existing code label Jan 24, 2019
@RyanCavanaugh RyanCavanaugh added this to This Week in Design Meeting Docket Jan 25, 2019
@RyanCavanaugh
Copy link
Member

@typescript-bot test this

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jan 25, 2019

Heya @RyanCavanaugh, I've started to run the extended test suite on this PR at 570c756. You can monitor the build here. It should now contribute to this PR's status checks.

@RyanCavanaugh
Copy link
Member

Also run on DT

@weswigham
Copy link
Member Author

@typescript-bot run dt

@typescript-bot
Copy link
Collaborator

typescript-bot commented Mar 8, 2019

Heya @weswigham, I've started to run the Definitely Typed test suite on this PR at 7c13b70. You can monitor the build here. It should now contribute to this PR's status checks.

@weswigham
Copy link
Member Author

Updated the test that needed changing to a constraint that actually typechecks, and looked at DT - failures are in ramda (and are also failing in master due to the recent function generic inference changes) and in a package callec victory - the diff in victory appears unrelated and already fixed, given that a local dtslint on it run doesn't show any errors.

So this seems pretty good - if anything, like the tests I just changed, it's going to identify places we were silently allowing unsafe operations.

@RyanCavanaugh
Copy link
Member

@weswigham let's merge for 3.5

@sandersn
Copy link
Member

@weswigham this sounds like it's ready to go, right?

@sandersn sandersn added this to Needs merge in PR Backlog Feb 12, 2020
@sandersn sandersn removed this from Gallery in Pall Mall Feb 12, 2020
@weswigham
Copy link
Member Author

Probably. The change itself is small, so barring merge conflicts, should still be OK.

@sandersn
Copy link
Member

After a merge from master: toString is now available on type parameters that extend any. That's the only difference I can see.

@sandersn
Copy link
Member

Let's wait until @RyanCavanaugh is back to merge this.

@sandersn
Copy link
Member

sandersn commented Mar 3, 2020

I talked to @RyanCavanaugh about this just now, so I'm going to merge it.

@sandersn sandersn merged commit 67c6ceb into microsoft:master Mar 3, 2020
PR Backlog automation moved this from Needs merge to Done Mar 3, 2020
@sandersn sandersn deleted the any-constraint-as-upper-bound branch March 4, 2020 19:00
@sandersn
Copy link
Member

sandersn commented Mar 4, 2020

@weswigham this causes a failure in DT on wordpress__block-editor-tests.tsx. Can you try to fix it? It looks like the offending signature is this:

declare function withColorContext<
    ProvidedProps extends Partial<withColorContext.Props>,
    OwnProps extends any,
    T extends ComponentType<ProvidedProps & OwnProps>
>(component: T):
    T extends ComponentType<infer U> ? ComponentType<
        Omit<U, 'colors' | 'disableCustomColors' | 'hasColorsToChoose'> &
        Omit<ProvidedProps, 'hasColorsToChoose'>> :
    never;

Previously the second type argument was inferred OwnProps=any; now it's OwnProps=unknown (as expected).

If there's a mechanical fix to update existing code, it'd worthwhile to have that in the release notes for 3.9.

@weswigham
Copy link
Member Author

If there's a mechanical fix to update existing code, it'd worthwhile to have that in the release notes for 3.9.

Write an Extract everywhere we now (correctly) flag as a type error, `(or, better yet, write types that actually check out without contortions). It's the typesystem equivalent of a typecast, but it's much more explicit.

eg,

type GetClass<T extends any> = T["class"];

// could be
type GetClass<T> = Extract<T, {class: any}>["class"];

obviously, you can replace Extract with a helper that provides a more reasonable default on failure than never for your given situation, too.

this causes a failure in DT on wordpress__block-editor-tests.tsx. Can you try to fix it? It looks like the offending signature is this:

What's broken about it? the signature itself has nothing wrong as far as I can tell, so you're talking about a specific usage, right?

@weswigham
Copy link
Member Author

weswigham commented Mar 4, 2020

What's broken about it?

If the answer is "we now infer unknown instead of any at use sites because we never make inferences", adding an = any default will silence the error and change the default only back to any. But I'd still consider an any-defaulting inference questionable, myself. You probably shouldn't have type parameters you can never infer...

@sandersn
Copy link
Member

sandersn commented Mar 6, 2020

Yes, line 142 of the tests. Probably = any to silence the error is good enough really.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Author: Team Breaking Change Would introduce errors in existing code For Milestone Bug PRs that fix a bug with a specific milestone
Projects
Archived in project
PR Backlog
  
Done
Development

Successfully merging this pull request may close these issues.

Missing error after spreading generic props into JSX
4 participants