`await` completions should auto-add an `async` modifier to containing method

[DanielRosenwasser]

Let's say you have a function like the following:

function foo() {
   /*$*/
}

If you type the word await, we should consider adding the async keyword to the beginning of the function declaration.

The motivation here was another snippet completion for object literal completions that @gabritto is working on (#46590) where it's not clear whether you really want a method to be async or not. For example, you might not want to make a function async if it's just going to return a Promise.

interface Foo {
  doIt(): Promise<any>;
}

const obj: Foo = {
  doIt() {
    return fetch(...);
  }
}

Because there's not enough context, even with the Promise annotation, some on the team would prefer to be conservative and not complete this as async, instead relying on later editor functionality to make a function async.

Today, if a person does wants to add await to a function that doesn't have an async modifier, we provide a quick fix to do this.

interface Foo {
  doIt(): Promise<any>;
}

const obj: Foo = {
  doIt() {
//~~~~
//
// Did you mean to mark this function as 'async'?

    await fetch(...);
//  ~~~~~
//
//   'await' expressions are only allowed within async functions and at the top levels of modules.
//   Quick fix: Add async modifier to containing function.
  }
}

This quick fix is nice, but the idea of the suggestion in this issue here is to just do the same thing as the quick fix upon completion.

Now are there any reasons to not do this?

Mostly that the idea of being "conservative" in when we decide to make a method async is not being applied here. A person can accidentally make their functions async, and that might not be desirable either, especially if any callers are not awaiting that function. Comparatively, there's relatively little harm in marking a Promise-returning function async.

[fatcerberus]

A person can accidentally make their functions async, and that might not be desirable either, especially if any callers are not awaiting that function.

My sense is, if you're explicitly writing await inside a function, that's a pretty strong indicator that you do want it to be async (particularly because it's a syntax error otherwise).

[DanielRosenwasser]

I agree that writing await inside a function that has no existing callers is a good signal that it should become async.

But if usages of that function aren't updated, I think it's a possible refactoring hazard.

@amcasey @RyanCavanaugh @andrewbranch might've had some thoughts here too.

[AlCalzone]

But if usages of that function aren't updated, I think it's a possible refactoring hazard.

Not if the function already has an explicit Promise<...> as the return type though?

[RyanCavanaugh]

Not a huge fan. I think in general, if I complete something that requires await, I want to be told that I'm not currently in an async function, and decide what to do about it -- maybe I'm in a code path that is necessarily sync, maybe not, and the quick-fix to add missing async is very readily available. Changing a function's color is not a decision to be made lightly and I don't like it when editors make big decisions on my behalf; making those changes should be low-friction, but not done with consulting the user.

[andrewbranch]

@RyanCavanaugh out of curiosity, do you feel the same way if the function is already annotated as returning a Promise?

[RyanCavanaugh]

No

[philkunz]

auto adding async keyword is a bad idea.