microsoft / TypeScript

TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
https://www.typescriptlang.org
Apache License 2.0
100.77k stars 12.46k forks source link

JS ES6 class derived member functions typed with literals don't get assigned the correct type #44640

Open Paril opened 3 years ago

Paril commented 3 years ago

Bug Report

In rare cases (such as when literals are used in a type constraint), ES6 class overridden functions will cause strange issues when returning said literals from the derived function.

🔎 Search Terms

ts es6 class derived

🕗 Version & Regression Information

⏯ Playground Link

Playground link with relevant code

💻 Code

class Base
{
    test()
    {
        if (Math.random() > 0.5)
            return 5;

        return false;
    }
}

class Derived extends Base
{
    // error here: for some reason, Derived::test() is typed as "() => boolean"
    // even though the literal false is being returned, and as such it says it can't
    // match Base::test
    test()
    {
        return false;
    }
}

🙁 Actual behavior

The derived function's auto-typing assumes it is the literals' actual type, such as number or boolean, instead of the literals themselves. Derived::test, if not overridden, is typed as () => false, which should match () => 5 | false, but because it's typed as boolean instead it breaks (this only seems to happen if we're overriding a base member function)

🙂 Expected behavior

The derived function should be able to match if the typing is handled as normal.

RyanCavanaugh commented 3 years ago

Well, this is unfortunate. There are three rules in play (really two rules and a non-rule):

It's not clear which of these we could change in a way that isn't a very disruptive breaking change.

Paril commented 3 years ago

Why does Derived#test get widened, though? That's the question here, because in theory it should stay as false which should match Base#test's 5 | false return type.

RyanCavanaugh commented 3 years ago

Why does Derived#test get widened, though?

If you have a hierarchy like this

class Base {
  getClassName() {
    return "base";
  }
}

class Derived extends Base {
  getClassName() {
    return "derived";
  }
}

The intent of this code is very clear -- it's not a subtyping violation, because the implied contract of Base#getClassName is to return some opaque string, rather than a function you would call to obtain some hardcoded string (since this is not useful). So when a function only returns a single string, we widen it to its base primitive type.

Paril commented 3 years ago

Oooh, I see. Okay. That makes sense.

My workaround ended up being to just throw on a @returns {false} which does silence the error. Given what you're saying about widening, it makes sense... but at the same time, if I wanted it to return an opaque string I feel like I'd have marked it with @returns {string} if it returns a literal. Maybe that's just me, though. I can see both sides of this one. It might not be something that can really be solved here.

fatcerberus commented 3 years ago

IMO if you know in advance the exact string a function is going to return to the point it’s encoded into your API contract and verified by the compiler, it’s probably not useful to have a function call in the first place.

Paril commented 3 years ago

The example they provided was not to show that that was something anybody would do, but rather a rationale for the behavior. There are actually legitimate uses for literals to be in return types like that - and this also applies to objects returned by the base & derived, too. For instance, if the base type returns an object like { width: 'auto' | number }, if the derived object has it returning an object whose width is auto without casting it into that type beforehand, it widens to a string, which fails the interface of 'auto' | number.

fatcerberus commented 3 years ago

Right, I was simply pointing out that there’s very little reason for a function’s return type to be a single literal value (e.g. function getFoo(): "foo")—otherwise why have the function at all—so TS won’t automatically infer such a return type either. Thus the automatic widening behavior you see here.

ravin00 commented 1 week ago

In the base class the test method is typed as test(): number | false this means it can return either 5 a number or false as a boolean. When you override this method from the derived class and return only false, TypeScript infers the return type of Derived::test as boolean, which is broader than false. This causes the type mismatch with Base::test, which is number | false

solution 01. You can explicitly define the return type of derived::test to ensure it matches the return type of Base::test

`class Base { test(): number | false { if (Math.random() > 0.5) return 5; return false; } }

class Derived extends Base { test(): number | false { // Explicitly define return type to match Base::test return false; } }`

Solution 02. Use the superclass call if you want to extend base logic

class Derived extends Base { test(): number | false { return super.test(); } }

This is the way Dervied::test will inherit the return type and the logic from Base::test

This Error is encountered because of the typescript type inference pf method overrides is based on the actual values returned in the body.