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

Abstract methods should not allow async modifier #28516

Closed
aleph-naught2tog opened this issue Nov 14, 2018 · 8 comments · Fixed by #39963
Closed

Abstract methods should not allow async modifier #28516

aleph-naught2tog opened this issue Nov 14, 2018 · 8 comments · Fixed by #39963
Labels
Bug A bug in TypeScript Help Wanted You can do this
Milestone

Comments

@aleph-naught2tog
Copy link

aleph-naught2tog commented Nov 14, 2018

This had already been discussed here which was a different issue that was concluded to be intended behaviour, but that abstract methods allowing async was not intended.

TypeScript Version: Version 3.2.0-dev.20181113

Search Terms:
abstract async, async

Code

// assuming `--lib es2015` is passed to the compiler
abstract class ShouldFailClass {
  async abstract someMethod(): Promise<number>; // This should error, but doesn't
}

Expected behavior:
Abstract methods should not allow an async modifier in the same way an interface and other type parameters don't allow them.

Actual behavior:
Abstract methods do not error with an async modifier attached; interface methods do.

Playground Link: Both of these should error, not just the second on, as discussed in the earlier linked issue. Reproduced here

Related Issues:
Originally discussed in the comments in here, where the consensus was that the behavior was intended for that case, but that async abstract should not be allowed.

aleph-naught2tog pushed a commit to aleph-naught2tog/TypeScript that referenced this issue Nov 14, 2018
Since we do not allow `async` as a modifier on type members such as in an interface, it was decided that it should also not be allowed on abstract methods.

Fixes microsoft#28516.
@weswigham weswigham added the Bug A bug in TypeScript label Nov 14, 2018
aleph-naught2tog pushed a commit to aleph-naught2tog/TypeScript that referenced this issue Nov 15, 2018
Since we do not allow `async` as a modifier on type members such as in an interface, it was decided that it should also not be allowed on abstract methods.

Fixes microsoft#28516.
@DanielRosenwasser DanielRosenwasser added the Help Wanted You can do this label Nov 15, 2018
@weswigham weswigham modified the milestones: Community, TypeScript 3.3 Dec 8, 2018
@rbuckton
Copy link
Member

@DanielRosenwasser should we also disallow async on overloads?

class C {
  async foo(); // <- should this be disallowed?
  async foo() {
  }
}

@DanielRosenwasser
Copy link
Member

Mmm, I don't know; seems like that's okay since the following is allowed.

async function foo(): Promise<number>
async function foo() {
  return Promise.resolve(10);
}

Thoughts @RyanCavanaugh?

@Jessidhia
Copy link

There is actually a runtime difference between an async function and a function that returns a Promise.

An async function will return a rejected promise no matter what exception happens in it, whenever such exception happens, even in very early steps such as when evaluating argument defaults. In TS parlance, it's never possible for an async function to return never. A function that returns a Promise might synchronously throw and return never.

image

I just don't know if this is something TS is capable of reasoning about.

@Jessidhia
Copy link

@DanielRosenwasser your example of allowed code is just how the PromiseResolve algorithm works. PromiseResolve of a Promise will just inherit that promise's state. async () => Promise.resolve(10) returns a Promise that is not === the Promise.resolve(10) you just created, but that will have the same state.

@Jessidhia
Copy link

Jessidhia commented Jan 17, 2019

Personally, requiring the Promise<...> around the return type of async functions is unnecessary syntactic noise. As I mentioned, it is impossible to not return a Promise from an async function -- TS could just do the clever thing and present the return value as Promise<ReturnType<the called overload>>.

They even have different prototypes (that differ in @@toStringTag value so should be representable as structural types):

image

image

But that ship sailed long before I even started learning JavaScript 😓

@rbuckton
Copy link
Member

@Kovensky: That isn't the issue at question here. The issue is whether to disallow async on ambient methods (abstract/overload/etc.).

@Jessidhia
Copy link

Jessidhia commented Jan 17, 2019

It seems related to me because of the contract I just mentioned. An async function is guaranteed to never throw, only reject its Promise, while a non-async function that happens to return a Promise could throw.

I guess this is kind of a broader point about having async () => foo be itself a first class type that is a subtype of () => Promise<foo> but distinct from it. Maybe I should make an issue for that.

@RyanCavanaugh RyanCavanaugh removed this from the TypeScript 3.4.0 milestone Mar 13, 2019
@RyanCavanaugh RyanCavanaugh added this to the Backlog milestone Mar 14, 2019
@trotyl
Copy link

trotyl commented May 3, 2019

An async function is guaranteed to never throw, only reject its Promise, while a non-async function that happens to return a Promise could throw.

@Jessidhia Your argument is actually why async should be forbid from abstract method, because the implementation of an async method doesn't need to be async, making it wrong assumption of non-throwable:

abstract class Base {
    abstract async foo(): Promise<number>
}

class Derived {
    foo(): Promise<number> {
        throw new Error()
    }
}

const base: Base = new Derived()
base.foo() // Throwable

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug A bug in TypeScript Help Wanted You can do this
Projects
None yet
7 participants