Eslint-plugin-jsdoc: Examine `Promise` executor for `undefined` `resolve` value (including when used in `async` functions)

[yvele]

Expected behavior

jsdoc/require-returns should not require a @returns JSDoc element when the async function returns a Promise that itself returns nothing.

Was working before https://github.com/gajus/eslint-plugin-jsdoc/releases/tag/v25.4.2 see https://github.com/gajus/eslint-plugin-jsdoc/issues/518

This should work:

/**
 * Description.
 */
async function foo() {
  return new Promise(resolve => resolve());
}

Actual behavior

This is not working:

/**
 * Description.
 */
async function foo() {
  return new Promise(resolve => resolve());
}

error Missing JSDoc @returns declaration jsdoc/require-returns

ESLint Config

    "jsdoc/require-returns" : "error"

ESLint sample

/**
 * Description.
 */
async function foo() {
  return new Promise(resolve => resolve());
}

error Missing JSDoc @returns declaration jsdoc/require-returns

Environment

• Node version: v12.16.2
• ESLint version v6.8.0
• eslint-plugin-jsdoc version: v25.4.2

[brettz9]

See https://github.com/gajus/eslint-plugin-jsdoc/issues/518#issuecomment-630035220

[yvele]

There may be some cases where a return ultimately resolves only to undefined, but as we don't auto-detect such possibilities, indeed, I think it is pretty clear that the expectation should be changed here to be safe. Even i it returns undefined, those looking at the function will appreciate knowing what it resolves to.

I'm not sure to understand... 🤔

The function is async and doesn't returns anything (of course an async function always returns a promise).. Why should I care about @returns?

This looks to me as a bug

[brettz9]

While we could add some analysis to check within the specific examples you gave, the general concern is that we are not handling such deep analysis as to check the likes of:

async function foo() {
  return new Promise(resolve => someCb(resolve));
}

The change is not a bug, at least not relative to how the rule has been intended and had working code along such lines.

It may have seemed to work before because we wrongly had a simple toggle that would fail to cause reporting at all--unless a method was not async, was a virtual function (i.e., @callback without any attached real function) or if it was async and had forceReturnsWithAsync set (the intent of this latter option being to require docs even if undefined were known to be returned by an async function). But otherwise, before the recent fix, if the function were async, it would simply not report at all, which was contrary to the intent of the rule.

Our current code only examines return values of the function. it has not been delving into Promise executors. If you would like to add a PR to dive inside the executor code--such as for your simpler examples--ensuring that there is either no chaining, and resolves to undefined or there is chaining but it always resolves to undefined, then I think it would be consistent to add such checking to this rule.

[yvele]

Isn't possible to "simply" not require JSDoc @returns on async functions when only a Promise is explicitly returned?

It's a bit hacky but does the trick.

With this new eslint-plugin-jsdoc I'm forced to gradually update my code base with useless @returns {Promise} declarations.

[brettz9]

Isn't possible to "simply" not require JSDoc @returns on async functions when only a Promise is explicitly returned?

But when a Promise is returned, there is just as much reason to know the return value type as when a regular value is returned. Unless again, it is known to resolve to undefined--but that is hardly to be expected (unless again, we dive into the executor to find out).

(IMO, it would be a better stylistic choice to only return Promise inside of non-async functions anyways, where new Promise is reserved for creating promisified functions and allowing all other (async) functions to use them with await. FWIW, this practice can be encouraged by eslint-plugin-promise: https://github.com/xjamundx/eslint-plugin-promise/blob/master/docs/rules/avoid-new.md though one has to disable the rule when one has to manually promisify functions. Just my two cents...)

It's a bit hacky but does the trick.

With this new eslint-plugin-jsdoc I'm forced to gradually update my code base with useless @returns {Promise} declarations.

If necessary, you could also disable this particular rule.

[yvele]

But when a Promise is returned, there is just as much reason to know the return value type as when a regular value is returned. Unless again, it is known to resolve to undefined--but that is hardly to be expected (unless again, we dive into the executor to find out).

Sure technically there in some cases there is no way to determine the exact returned resolved value.

But I think that this ESLint plugin should better focus on JSDoc being as clean "as possible". The plugin should never make JSDoc worse 🤔 don't you agree?

Maybe a "hack" and having a rule that does not find 100% of infringements is better dans a rule that force writing useless JSDoc..

I have many use cases when I'm returning directly a Promise, sometimes with new Promise when using legacy callback libraries OR event driven libraries. And also when I'm using promise coordination libraries like https://github.com/sindresorhus/promise-fun or Promise.all, Promise.any, etc.

A possible workaround may be to await instead of returning:

async function foo() {
  await new Promise();
}

But it's a bad practice and hurts performances, see https://eslint.org/docs/rules/no-return-await

You can avoid the extra microtask by not awaiting the return value, with the trade off of the function no longer being a part of the stack trace if an error is thrown asynchronously from the Promise being returned. This can make debugging more difficult.

---

(IMO, it would be a better stylistic choice to only return Promise inside of non-async functions anyways

Sure! But in all my code base 100% of the time when a function returns a Promise it is marked as async anyway.

---

where new Promise is reserved for creating promisified functions and allowing all other (async) functions to use them with await. FWIW, this practice can be encouraged by eslint-plugin-promise: xjamundx/eslint-plugin-promise:docs/rules/avoid-new.md@master though one has to disable the rule when one has to manually promisify functions. Just my two cents...)

I 100% agree and I'm already enforcing that.

---

If necessary, you could also disable this particular rule.

Of course (as an unpleasant workaround) but that rule is super useful for my whole team and myself. I just don't want it to require useless declaration 🤷

This plugin should help developer to write clean code not the opposite...

[brettz9]

But when a Promise is returned, there is just as much reason to know the return value type as when a regular value is returned. Unless again, it is known to resolve to undefined--but that is hardly to be expected (unless again, we dive into the executor to find out).

Sure technically there in some cases there is no way to determine the exact returned resolved value.

But I think that this ESLint plugin should better focus on JSDoc being as clean "as possible". The plugin should never make JSDoc worse 🤔 don't you agree?

Agreed. My personal taste is to use forceRequireReturn/forceReturnsWithAsync as:

1. It makes clear that the function is indeed intended to give or resolve to undefined.
2. One can just look quickly at the jsdoc block above the code to read the full API in a consistent manner.

But I understand others are more minimalist and the reason I am fine with your desired changes to the rule.

Maybe a "hack" and having a rule that does not find 100% of infringements is better dans a rule that force writing useless JSDoc..

Sure.

I have many use cases when I'm returning directly a Promise, sometimes with new Promise when using legacy callback libraries OR event driven libraries. And also when I'm using promise coordination libraries like https://github.com/sindresorhus/promise-fun or Promise.all, Promise.any, etc.

A possible workaround may be to await instead of returning:

async function foo() {
  await new Promise();
}

But it's a bad practice and hurts performances, see https://eslint.org/docs/rules/no-return-await

You can avoid the extra microtask by not awaiting the return value, with the trade off of the function no longer being a part of the stack trace if an error is thrown asynchronously from the Promise being returned. This can make debugging more difficult.

Fair point re: performance--though debugging may sometimes outweigh the disadvantage of an extra microtask.

(IMO, it would be a better stylistic choice to only return Promise inside of non-async functions anyways

Sure! But in all my code base 100% of the time when a function returns a Promise it is marked as async anyway.

I see. (There is also jsdoc's @async, btw, if you wanted to drive that home in a different way. But I see how it could be nicely self-documenting.)

If necessary, you could also disable this particular rule.

Of course (as an unpleasant workaround) but that rule is super useful for my whole team and myself. I just don't want it to require useless declaration 🤷

This plugin should help developer to write clean code not the opposite...

We are in agreement on the general need, and I'm personally ok with the added behavior. Just not my priority now to implement, though PRs as usual are welcome.

[brettz9]

Btw, if return new Promise(resolve => resolve()); is the only case you really need, it isn't too hard to add support for this particular construct. The complexity would be in parsing a Promise executor block with any number of branches that may or may not resolve. But if it's just this one case (used to avoid an await), that wouldn't be hard to add support.

[brettz9]

Guess I didn't think that through as that would really serve no purpose within an async function. But if there were a particular pattern, that wouldn't be the issue.

[brettz9]

I've filed #683 which should address this issue. Its detection for resolve use is not perfect ( #682 shows some additional AST which this PR (and some other rules, albeit with yield and throws) still need to support (e.g., false || resolve(true) would not currently be detected by that PR)), but I hope I think it may be better than nothing for now. (It's not too difficult to support, but needs grunt work, including preparing tests to give coverage.)

[brettz9]

Besides avoiding the triggering of require-returns-check when return is not present, in #684, I'm seeking to offer an option whereby you can also get notified if there is a @returns and no non-undefined resolve/async return.

This can ensure that require-returns-check will take cases like your void-resolving example with a @returns on it...

        /**
         * Description.
         * @returns {string}
         */
        async function foo() {
          return new Promise(resolve => resolve());
        }

...and report it as an error (since this does not match).

I've also added an option reportMissingReturnForUndefinedTypes so that even if void or undefined is used as the type (in async or not), you can get this reported except when undefined is explicitly returned (since by the way your issue appears to wants to treat promises, such @returns would be extraneous and you might want what you see as cruft reported rather than tolerated).

[gajus]

:tada: This issue has been resolved in version 31.5.0 :tada:

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot :package::rocket: