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

Use of implicit union return type causes "'boolean' is not assignable to type 'false'" #19212

Closed
jimkyndemeyer opened this issue Oct 16, 2017 · 5 comments
Labels
Duplicate An existing issue was already created

Comments

@jimkyndemeyer
Copy link

TypeScript Version: 2.6.0-dev.20171015

This relates to the render method of React components, specifically the use of false in the union return type:

render(): JSX.Element | null | false

I've create a stand-alone example that demonstrates the issue. See the code comments for instructions.

Code

declare class Base {
    render(): Date | false;
}

class TestImplicitReturnType extends Base {

    render() /* ': false' is required here as explicit return type to prevent a compile error */ {
       return false;
    }

}

class TestControlFlow extends Base {

    render() {
        if (new Date().getTime() % 2 === 0) {
            return new Date(); /* removing this return statement causes the false value below to be rejected  */ 
        }
        return false;
    }

} 

Expected behavior:
For TestImplicitReturn I should not be required to manually add : false as return type.

For TestControlFlow I should not be required to also return a Date to satisfy the type checker.

Actual behavior:

For TestImplicitReturn the compiler outputs:

Class 'TestImplicitReturnType' incorrectly extends base class 'Base'.
  Types of property 'render' are incompatible.
    Type '() => boolean' is not assignable to type '() => false | Date'.
      Type 'boolean' is not assignable to type 'false | Date'.

For TestImplicitReturnType the compiler outputs the following when the date return statement is removed:

Class 'TestControlFlow' incorrectly extends base class 'Base'.
  Types of property 'render' are incompatible.
    Type '() => boolean' is not assignable to type '() => false | Date'.
      Type 'boolean' is not assignable to type 'false | Date'.
@mhegazy
Copy link
Contributor

mhegazy commented Oct 16, 2017

The issue here is that a method in a derived class is not contextually typed by the type of the base's method. This has been discussed in by #1373 (and related to #3667, #6118 and #10610).

@mhegazy
Copy link
Contributor

mhegazy commented Oct 16, 2017

Duplicate of #1373

@mhegazy mhegazy added the Duplicate An existing issue was already created label Oct 16, 2017
@jimkyndemeyer
Copy link
Author

Thanks for the detailed reply.

I still find it odd (counter-intuitive) that false produces a compilation error when boolean doesn't:

TypeScript playground using boolean

TypeScript playground using false

Does this narrow the scope of a potential fix? As it stands we're seeing the unwanted compilation error when using @graphql.

@RyanCavanaugh
Copy link
Member

RyanCavanaugh commented Oct 16, 2017

As the linked issue mentions, we don't currently use information about the base class to inform the type of a derived class method. So understand the below in that context (i.e. we think the best fix involves using that information)

There are rules around widening that attempt to figure out what behavior you're trying expose in a method. If you wrote something like

    someMethod() {
       if (...) return 3; else return 6;
    }

we think that you want the signature someMethod(): number, not someMethod(): 3 | 6 -- basically, TS shouldn't immediately expose the internals of the function without some kind of hint to do so.

We do infer Date | false as a return type when the return expressions are heterogeneous.

All these rules came from experimental data produced when we added literal types to the type system (in other words, we messed with them until we got the "best" set of breaking changes) and are based on our "best guess" about what the inferred return type should be. We already tweaked these rules to the best of our ability and any change here would move away from the local maximum.

@mhegazy
Copy link
Contributor

mhegazy commented Oct 30, 2017

Automatically closing this issue for housekeeping purposes. The issue labels indicate that it is unactionable at the moment or has already been addressed.

@mhegazy mhegazy closed this as completed Oct 30, 2017
@microsoft microsoft locked and limited conversation to collaborators Jun 14, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Duplicate An existing issue was already created
Projects
None yet
Development

No branches or pull requests

3 participants