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

Confusing behavior with valid-jsdoc and requireReturn #4859

Closed
tschaub opened this Issue Jan 5, 2016 · 11 comments

Comments

Projects
None yet
5 participants
@tschaub
Copy link
Contributor

tschaub commented Jan 5, 2016

While looking into a way to make the valid-jsdoc rule work well with @interface (where functions may be required to have no body), I noticed some odd behavior with requireReturn.

Using ESLint @ 009a949, the following is valid:

/* eslint valid-jsdoc: [2, {"requireReturn": true}] */

/**
 * Testing valid-jsdoc.
 * @return {string} A string.
 */
exports.foo = function() {
  // noop
};

If I change requireReturn to false, I get an error:

/* eslint valid-jsdoc: [2, {"requireReturn": false}] */

/**
 * Testing valid-jsdoc.
 * @return {string} A string.
 */
exports.foo = function() {
  // noop
};

Unexpected @return tag; function has no return statement valid-jsdoc

So only if I don't require a @return tag, the function is checked for a return statement. Here is the relevant code:

if (!requireReturn && !functionData.returnPresent && (tag.type === null || !isValidReturnType(tag))) {
  context.report(jsdocNode, "Unexpected @" + tag.title + " tag; function has no return statement.");
}

The !requireReturn looks like a typo. It would make a bit more sense to say "if I don't require the return tag, don't check if a return statement is present" - but even that may be overloading requireReturn (maybe the option should be about the tags only and not about the code).

I'm also confused by this sentence in the documentation:

Note that with this option set to false, if there is a return in the function, an error will still be logged and if there is a @return specified and there are no return statements in the function an error will also be logged.

Specifically, this is confusing: "if there is a return in the function, an error will still be logged and (??) if [...] there are no return statements in the function [...]". It is not clear to me how there can be a "return in the function" and "no return statements in the function."

@briandipalma and @nzakas - can you confirm this is the behavior you intended to get out of #673?

@eslintbot

This comment has been minimized.

Copy link

eslintbot commented Jan 5, 2016

@tschaub Thanks for the issue! If you're reporting a bug, please be sure to include:

  1. The version of ESLint you are using (run eslint -v)
  2. What you did (the source code and ESLint configuration)
  3. The actual ESLint output complete with numbers
  4. What you expected to happen instead

Requesting a new rule? Please see Proposing a New Rule for instructions.

@eslintbot eslintbot added the triage label Jan 5, 2016

@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Jan 5, 2016

This is expected behavior. When requireReturn is false, it means that you should not have @returns in your JSDoc unless there is a return in the function. In your example, you get an error because you are including @return and there is no return in the function.

We could make the docs a bit clearer, but unless I'm missing something, I don't see a bug here.

@nzakas nzakas added rule documentation accepted and removed triage labels Jan 5, 2016

@tschaub

This comment has been minimized.

Copy link
Contributor Author

tschaub commented Jan 5, 2016

It makes sense to me that if I have a return statement and no @return tag, valid-jsdoc would complain.

What is confusing to me is that to disable that behavior, I would set requireReturn to true.

@tschaub

This comment has been minimized.

Copy link
Contributor Author

tschaub commented Jan 5, 2016

Or, put another way, if I want valid-jsdoc to fail when there is no @return tag for a function that has a return statement, it is confusing to me that I would set requireReturn to false. The confusing part is that the rule becomes more strict when I say "don't require a return".

@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Jan 5, 2016

The way the rule is designed, all the flags are enabled by default and you can opt-out of the ones you don't want. We had to do this for backwards compatibility as we added more features.

I think you're misunderstanding how this options works. requireReturn: true (the default) says "You must always use @returns no matter what." requireReturn: false says, "You must use @returns only if you have a return value in your function."

So by default, you will get an error if you omit @returns from a function's JSDoc regardless of what's going on in the function. You can opt-out of that behavior by setting requireReturn: false. There's never a need to manually set requireReturn: true because that's the default.

@tschaub

This comment has been minimized.

Copy link
Contributor Author

tschaub commented Jan 6, 2016

Ok, thanks for your patience @nzakas. I've made peace with requireReturn.

#4864 is my attempt to clarify the docs.

While it would be a breaking change, I think it would be unsurprising if (by default) any function with a return statement must be documented with @return, and any function with a @return tag must include a return statement. And to disable this "consistent return" checking requireReturn could be set to false (or maybe it would be called consistentReturn).

@nzakas nzakas closed this in f7b28b7 Jan 7, 2016

nzakas added a commit that referenced this issue Jan 7, 2016

Merge pull request #4864 from tschaub/require-return
Docs: clarify `requireReturn` option for valid-jsdoc rule (fixes #4859)
@Telokis

This comment has been minimized.

Copy link

Telokis commented May 12, 2017

@nzakas Is there a way to prevent the rule from generating a warning/error if there is a return statement that is not documented?
In asynchronous code return is only used to prevent the code from executing the end of the function. (For example, by doing return callback(err);.
So I'd like to not enforce the presence of @returns in my doc. That's what I expected from requireReturn.

@briandipalma

This comment has been minimized.

Copy link
Contributor

briandipalma commented May 12, 2017

This wouldn't be requireReturn's use case. In essence you want to enforce documentation apart from return statements? Or do you want to disable enforcing return documentation on statements that consist of the pattern callback(args);?

@Telokis

This comment has been minimized.

Copy link

Telokis commented May 12, 2017

@briandela Precisely, I'd like to disable @returns necessity when the return line matches a specific pattern. Would it be possible with the current eslint?

@briandipalma

This comment has been minimized.

Copy link
Contributor

briandipalma commented May 12, 2017

I'm sure it would be but I'd raise a feature request instead of commenting on a closed issue if that's what you want. Try and implement the feature.

@briandipalma

This comment has been minimized.

Copy link
Contributor

briandipalma commented May 12, 2017

Just take a look at the JSDoc rule and add a new param for the feature you want.

@eslint eslint bot locked and limited conversation to collaborators Feb 6, 2018

@eslint eslint bot added the archived due to age label Feb 6, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.