Skip to content

Feature request - rule for await in async try catch return #994

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

Closed
drabinowitz opened this issue Sep 20, 2019 · 13 comments · Fixed by #1050
Closed

Feature request - rule for await in async try catch return #994

drabinowitz opened this issue Sep 20, 2019 · 13 comments · Fixed by #1050
Labels
enhancement: new plugin rule New rule request for eslint-plugin has pr there is a PR raised to close this package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin

Comments

@drabinowitz
Copy link
Contributor

A Rejected Promise returned by an async function will not be caught by the function's try-catch block. This may surprise some users and lead to unexpected behavior:

(async () => {
  try {
    // will result in an UnhandledPromiseRejection
    return Promise.reject('Error!');
  } catch (e) {
    // will not run
    handleCleanup();
  }
})();

conversely, await-ed promises work in a more intuitive fashion:

(async () => {
  try {
    // will not result in an UnhandledPromiseRejection
    return await Promise.reject('Error!');
  } catch (e) {
    // will run
    handleCleanup();
  }
})();

I understand why this is the case according to the async-await function spec, but it feels like confusing behavior and a problem that typescript-eslint could help avoid via the following rule:

disallow return-ing a Promise from inside an async function try-catch block.

Thus, the first case described above would be invalid according to this rule, but the second case would be valid. We could even, potentially, provide an auto-fix that would add the await statement to the promise.

There's also the possibility for the broader rule of disallowingreturn-ing a Promise from anywhere inside an async function, but I wanted to start with the narrowest case.

Please let me know if there's any other clarifying information I can provide, thanks so much for your time!

@drabinowitz drabinowitz added package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin triage Waiting for team members to take a look labels Sep 20, 2019
@bradzacher bradzacher added enhancement: new plugin rule New rule request for eslint-plugin and removed triage Waiting for team members to take a look labels Sep 23, 2019
@ark120202
Copy link

With eslint/eslint#12246 in mind (V8 providing better stack traces for return await) would it make sense to have a new return-await rule with a 'never' | 'in-try-catch' | 'always' option?

@drabinowitz
Copy link
Contributor Author

Yes i think that'd make more sense @ark120202. @bradzacher what are you thoughts on that change should I update the feature request?

@bradzacher
Copy link
Member

bradzacher commented Sep 30, 2019

To clarify then,

The idea would be to have a rule (return-await) which bans directly returning await statements (return await foo) (i.e. requires no type info), except in the case that you're returning a promise from within a try/catch, in which case you want to enforce that you return an await (i.e. requires type info).

Seems like a good candidate for this plugin.

declare function returnsPromise(): Promise<string>;
declare function handleException(e: Error): string;

// option: 'in-try-catch'
async function myFunc1(): Promise<string> {
  try {
    return returnsPromise(); // lint error - should await the promise so errors are handled by the surrounding try/catch
  } catch (e) {}
  return Promise.resolve('foo'); // no error
}

// option: 'never'
async function myFunc1(): Promise<string> {
  try {
    return await returnsPromise(); // lint error - should never return await
  } catch (e) {}
  return await Promise.resolve('foo'); // lint error 
}

// option: 'always'
async function myFunc1(): Promise<string> {
  try {
    return returnsPromise(); // lint error - should always await promise
  } catch(e) {}
  return Promise.resolve('foo'); // lint error 
}

@drabinowitz
Copy link
Contributor Author

that's what i was thinking, yeah! @ark120202 is the snippet above what you were thinking for this?

@ark120202
Copy link

Yeah, that sounds good to me!
One thing I'm not really sure is 'never', since currently no-return-await doesn't disallow it in try catch (but doesn't enforce it as well). So if compatibility with no-return-await is one of goals it has to be allowed.

@drabinowitz
Copy link
Contributor Author

I'm actually okay with that since the same could be said for the always rule (in terms of incompatibility with no-return-await). I think if we make the default in-try-catch then we avoid this issue for most users. It feels appropriate to expect users with customized rules to have to be more careful in understanding how those rules will interact.

@drabinowitz
Copy link
Contributor Author

@bradzacher so in terms of next steps should i put together a pr that references this issue? I haven't worked on a codebase like this in the past but I'd be happy to give it a go

@ark120202
Copy link

Actually, in that case is there a need for 'never' at all? I have originally proposed it to be more compatible with no-return-await, but I agree that there's no reason to pursue that. Would someone really want to disallow return await even in try catch?

@drabinowitz
Copy link
Contributor Author

hmm interesting thought. Performance critical applications i suppose, but even then this is a pretty narrow optimization. @bradzacher did you have a particular use case in mind or just for consistency purposes?

@bradzacher
Copy link
Member

consistency, warding off people asking for it.

I can pretty easily see an argument that people don't want return await ever, and would rather a variable assignment that gets returned a la

async function myFunc1(): Promise<string> {
  try {
    const res = await returnsPromise();
    return res;
  } catch (e) {}
  const res = await Promise.resolve('foo');
  return res;
}

Not sure - it's probably the easiest case to support, so I figure there's little overhead to supporting it.

@drabinowitz
Copy link
Contributor Author

so in terms of next steps for this, should I open a PR?

@bradzacher
Copy link
Member

Definitely - if you see value in this and want it sooner, rather than later!

It's always a lot faster and better for us if users PR the features we want; either we maintainers can try to write PRs for the ~180 issues in the repo, or we can review PRs for those issues. The latter is more scalable and easier.

@drabinowitz
Copy link
Contributor Author

sounds good ill get to work on this, this weekend!

drabinowitz added a commit to drabinowitz/typescript-eslint that referenced this issue Oct 8, 2019
drabinowitz added a commit to drabinowitz/typescript-eslint that referenced this issue Oct 8, 2019
drabinowitz added a commit to drabinowitz/typescript-eslint that referenced this issue Oct 8, 2019
@bradzacher bradzacher added the has pr there is a PR raised to close this label Oct 8, 2019
drabinowitz added a commit to drabinowitz/typescript-eslint that referenced this issue Oct 8, 2019
drabinowitz added a commit to drabinowitz/typescript-eslint that referenced this issue Nov 25, 2019
return-await is not a recommended rule for users as part of default typescript-eslint setup.

re typescript-eslint#994
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 20, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement: new plugin rule New rule request for eslint-plugin has pr there is a PR raised to close this package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants