require-returns-check returning false errors with loops

[JasonXJ]

This is similar to #924 but doesn't seem to be fixed.

Expected behavior

There should be no jsdoc/require-returns-check errors reported for the functions below. This was working for eslint-plugin-jsdoc v39.3.6.

/**
 * @return {number}
 */
function foo() {
  while (true) {
    const n = Math.random();
    if (n < 0.5) {
      return n;
    }
  }
}

Actual behavior

  58:1  error  JSDoc @return declaration present but return expression not available in function  jsdoc/require-returns-check

Environment

• Node version: v16.13.0
• ESLint version: 8.26.0
• eslint-plugin-jsdoc version: 39.6.2

[brettz9]

The reason this hadn't erred previously is because the rule was not checking all branches; it would be satisfied if a return were discovered somewhere. The current approach is to try to check all branches. Of course, with a while (true), this never ends, and we don't need all branches if eventually a condition is inevitably met within the loop which returns.

I am open to suggestions, but I'm not sure how frequent a problem this is, and what the best detection algorithm should be (especially without also leading to false negatives). We of course don't want to have to add complex code to determine ourselves whether incrementing will automatically lead to the condition being fulfilled or not.

We could perhaps just assume that the programmer knows what they're doing with while (true) since it will provide a more serious problem (becoming blocking) if the condition doesn't fulfill. Or we could let it err and let users suppress it to ensure they are manually confirming the code is ok. WDYT?

[JasonXJ]

The current approach is to try to check all branches

Can we only check reachable branches? And it seems that the linter does know that the branch outside of the loop is unreachable --- if I add a line throw new Error('...') after the while loop, the linter complaints Unreachable code no-unreachable

[brettz9]

Yes, I meant it is trying to check all reachable branches, but we have to have code in place to do so.

That is a good find on the the no-unreachable rule, but that is from ESLint itself which does some of its own analysis to determine whether code is reachable. I'm not sure whether we can leverage this analysis though because we are doing our own traversal here as we need to do in order to check for our main task, return statements (we might be able to store the code path segments and then check their reachability status as we are doing our own traversal).

Notice that ESLint's analysis is not perfect either--it appears to be aware of simple while (true) or while (1) loops (without break), but not say while (1 || true) without a break; notice the lack of errors here:

https://eslint.org/play/#eyJ0ZXh0IjoiZnVuY3Rpb24gYSAoKSB7XG4gIHdoaWxlICgxIHx8IHRydWUpIHtcbiAgICBjb25zdCBuID0gTWF0aC5yYW5kb20oKTtcbiAgICBpZiAobiA8IDAuNSkge1xuICAgICAgcmV0dXJuIG47XG4gICAgfVxuICB9XG4gIHRocm93IG5ldyBFcnJvcignYWJjJyk7XG59Iiwib3B0aW9ucyI6eyJwYXJzZXJPcHRpb25zIjp7ImVjbWFWZXJzaW9uIjoxMiwic291cmNlVHlwZSI6InNjcmlwdCIsImVjbWFGZWF0dXJlcyI6e319LCJydWxlcyI6eyJuby11bnJlYWNoYWJsZSI6WyJlcnJvciJdfSwiZW52Ijp7ImJyb3dzZXIiOmZhbHNlfX19

The ESlint algorithm is at least good enough for the more common cases, but we need to find out how to tap into this algorithm or perhaps, if needed, reimplement it ourselves.

[Shinigami92]

Here are three code examples that run into this issue:

/**
 * Returns the previous tag token if there was one.
 *
 * @param tokens The token array.
 * @param index The current index within the token array..
 * @returns Previous tag token if there was one.
 */
export function previousTagToken(
  tokens: ReadonlyArray<Token>,
  index: number,
): TagToken | undefined {
  for (let i: number = index - 1; i >= 0; i--) {
    const token: Token | undefined = tokens[i];
    if (!token) {
      return;
    }
    if (token.type === 'tag') {
      return token;
    }
  }
  return;
}

/**
 * Returns the previous attribute token between the current token and the last occurrence of a `start-attributes` token.
 *
 * @param tokens A reference to the whole token array.
 * @param index The current index on which the cursor is in the token array.
 * @returns Previous attribute token if there was one.
 */
export function previousNormalAttributeToken(
  tokens: ReadonlyArray<Token>,
  index: number,
): AttributeToken | undefined {
  for (let i: number = index - 1; i > 0; i--) {
    const token: Token | undefined = tokens[i];
    if (!token || token.type === 'start-attributes') {
      return;
    }
    if (token.type === 'attribute') {
      if (token.name !== 'class' && token.name !== 'id') {
        return token;
      }
    }
  }
  return;
}

/**
 * Returns the previous type attribute token or undefined if no attribute is present.
 *
 * @param tokens A reference to the whole token array.
 * @param index The current index on which the cursor is in the token array.
 * @returns Previous attribute token if there was one.
 */
export function previousTypeAttributeToken(
  tokens: ReadonlyArray<Token>,
  index: number,
): AttributeToken | undefined {
  for (let i: number = index - 1; i > 0; i--) {
    const token: Token | undefined = tokens[i];
    if (!token || token.type === 'start-attributes' || token.type === 'tag') {
      return;
    }
    if (token.type === 'attribute') {
      if (token.name === 'type') {
        return token;
      }
    }
  }
  return;
}

https://github.com/prettier/plugin-pug/blob/1c3a085aefbfd8a24996fa526d18f582eec08a11/src/utils/common.ts#L5-L76

[brettz9]

@Shinigami92 : Those are all expected behavior per our expectations of the rule (it was not always reporting previously because the rule had mistakenly not been checking every branch.) You could get out of the issue with an explicit return undefined or by including void or undefined as one of the return types.

If you want to allow for empty return statements so long as a single non-empty one exists, feel free to file another issue as a feature request as that is different from the particular (more obscure) problem reported in this issue.

[brettz9]

@Shinigami92 : FWIW, your particular issues should be fixed by #949