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

Can't implement interface by throwing error #16874

Closed
ghost opened this issue Jun 30, 2017 · 4 comments
Closed

Can't implement interface by throwing error #16874

ghost opened this issue Jun 30, 2017 · 4 comments
Labels
Fixed A PR has been merged for this issue Suggestion An idea for TypeScript

Comments

@ghost
Copy link

ghost commented Jun 30, 2017

TypeScript Version: nightly (2.5.0-dev.20170629)

Code

interface I { m(): number; }
const o: I = { m() { throw new Error("not implemented"); } }

Expected behavior:

No error.

Actual behavior:

Type '() => void' is not assignable to type '() => number'.

The same problem occurs inside a class. Manually annotating m(): number fixes the problem, but shouldn't we get that from the contextual type anyway?

@DanielRosenwasser
Copy link
Member

DanielRosenwasser commented Jun 30, 2017

Check out #16608 and #8767 for changes/discussion related to this. We could fix this with a voidWideningNever type, or by just checking for a contextual type.

I'm only half-joking.

@DanielRosenwasser DanielRosenwasser added In Discussion Not yet reached consensus Suggestion An idea for TypeScript labels Jun 30, 2017
@RyanCavanaugh
Copy link
Member

These should be identical:

const o1: I = { m: function () { throw new Error('nyi'); } }
const o2: I = { m() { throw new Error("not implemented"); } }

@masaeedu
Copy link
Contributor

masaeedu commented Jul 9, 2017

@DanielRosenwasser Probably need more input from other people but I think it'd be better to have a flag that undoes #8767 rather than a mashup type of void and never.

I'm actually fine with getting an error in this case:

class Base {
    overrideMe() {
        throw new Error("You forgot to override me!");
    }
}

class Derived extends Base {
    overrideMe() {
        // Code that actually returns here
    }
}

From my perspective, it's better to have to explicitly annotate the return type in the base class, because it might be the case that my to-be-overridden function is actually supposed to return string, or an array, or any number of other things, and an inferred return type of void will just mislead someone who has a reference of type Base.

@RyanCavanaugh
Copy link
Member

@masaeedu we talked about this quite a bit last week. The problem with erroring in the above code is that it would be a breaking change. The only reason we even did #8767 was because the number of breaks it induced was too significant.

Breaking change concerns aside, the good thing with giving a type void here is that no one can meaningfully misuse a void value obtained via a Base instance reference. Conversely, you can trivially misuse a never value because it's e.g. a valid function argument to any parameter position. It's worth considering the case where the author of Base does not have any Deriveds in their codebase (because they're shipping a base class library) -- they won't realize they have a "missing" type annotation until one of their consumers shows up complaining.

Re: another flag... this is not something that would be worth doubling the configuration space of TypeScript over.

@ghost ghost closed this as completed in #20052 Jan 5, 2018
@mhegazy mhegazy added Fixed A PR has been merged for this issue and removed In Discussion Not yet reached consensus labels Jan 6, 2018
@mhegazy mhegazy added this to the TypeScript 2.7 milestone Jan 6, 2018
@microsoft microsoft locked and limited conversation to collaborators Jul 3, 2018
This issue was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Fixed A PR has been merged for this issue Suggestion An idea for TypeScript
Projects
None yet
Development

No branches or pull requests

4 participants