checkRedundantReturns: thought about reworking rule

[cowwoc]

/**
 * @function
 * @this AuthenticationsPage
 * @return {Promise} a Promise that returns {@code undefined} on success and {@code Error} on
 *   failure
 */

This is a function, not a constructor, yet I am getting the Redundant JsDoc @returns error on the @return line.

You asked what kind of JsDoc I am using... I'm not actually producing any documentation from this yet. I am currently coding this (mostly) blind using http://usejsdoc.org/ as reference and https://netbeans.org/ for some light syntax validation (they have some built-in JsDoc engine but it's not clear which).

[qfox]

Can you show a function?

[qfox]

Actually to understand a problem I need just all blocks and scopes inside your function. Can you left just ifs, whiles, switch-cases, functions and show your func?

[cowwoc]

Here you go:

            /**
             * @function
             * @this AuthenticationsPage
             * @return {Promise} a Promise that returns {@code undefined} on success and {@code Error} on
             *   failure
             */
            function onSubmit()
            {
                // jshint validthis:true
                Preconditions.requireThat(this, "this").isInstanceOf(AuthenticationsPage);
                var $form = this.loginForm.$form;
                var email = $form.find("input[name='email']").val();
                var password = $form.find("input[name='password']").val();
                return this.login(email, password).then(function(token)
                {
                    Authentications.login(token);
                    redirectToReferrer();
                });
            }

[qfox]

Great! I'm almost sure it's a bug in doc to code matching logic. Thanks for your time and code ;-)

[qfox]

@cowwoc I have a problem. I can't repeat this ;-) Can you try to clone gist and test in your env? https://gist.github.com/zxqfox/4754742b9eb412560535

[qfox]

Can you try to update to v0.0.20 ?

[cowwoc]

@zxqfox It seems this issue needs more context (multiple functions). I am working on a minimal testcase now.

[cowwoc]

Okay, I got it. The file in question had 2 functions named onSubmit() which did different things.

The function I posted above was not the one that triggered the error The second function had documentation that said it would @return a value but I forgot to actually do so in the code. Meaning, in the above code, if you remove return in front of return this.login(email, password) then you should get the error message I mentioned (complaining about a redundant return value).

So technically speaking the warning is correct, but the message is misleading. The @return statement is not redundant, but rather there is a mismatch between the documented and actual return types (Promise and undefined respectively). I hope that helps.

[qfox]

@cowwoc Well... ;-D

Did you mean this.login is undefined? I see return this.login(...) in the code so I can't understand what triggers an error and why it's type mismatch ;-\

Can you provide this part of code?

[qfox]

You mean this case?

/**
 * @return {undefined} something specie
 */
function () {
  // dummy...
}

checkRedundantReturns rule was made for @returns statements for functions without returns. So if there are no returns in code it will report an error. So it works like it does except case I've shown. But @return {undefined} looks strange at least.

Right?

[cowwoc]

@zxqfox You see return this.login(...) in the code I gave you, but I gave you the wrong code. The real code that was triggering this error was missing return in front of this.login(...). If you take your testcase and remove the return keyword in front of this.login(...) I believe you will get the same error message.

I thought that checkRedundantReturns focused exclusively on return {undefined} and that in all other cases, checkReturnTypes should get triggered instead and warn that there is a mismatch between the declared and actual return types.

[qfox]

@cowwoc There are three separated options for now.

requireReturnTypes just checking existence of type in jsdoc; checkReturnTypes makes code scan for inner content of function (scope) and matching inner return values (if possible) with declared in jsdoc; checkRedundantReturns also makes code scan and looks for @returns in jsdoc and reports statements if there are no returns in code;

Also there are checkTypes for trying to parse types in bunch of tags.

If you think that's not right let's discuss about it.

[cowwoc]

I think that checkReturnTypes when there is a documented @return and the code actually returns a value: just make sure the types match.

I would also have it cover two additional use-cases:

1. Documentation returns but code does not.
2. Code returns but documentation does not.

In these cases, I think it is wrong to assume the @return documentation is redundant. There is an equal probability that the developer forgot to return in the code. When this condition is detected, warn the user and let them figure out which one is wrong.

I would then remove the requireReturnTypes rule (it's covered by the above rule) and have checkRedundantReturns only warn about @return {undefined}.

[qfox]

@cowwoc I've wrote some additional information about rules in readme. I know that all these rules are not perfect but to go ahead we need to formalize issues.

I think that checkReturnTypes when there is a documented @return and the code actually returns a value: just make sure the types match.

It works only if both exists. It doesn't check doctype for been valid, it doesn't force function to have return. It just matches if they both exists.

I would also have it cover two additional use-cases:

1. Documentation returns but code does not.
2. Code returns but documentation does not.

Case (1) now handled by checkRedundantReturns and reports for it as: dude, you have forgotten return tag!. Case (2) doesn't handled at all (afaik).

In these cases, I think it is wrong to assume the @return documentation is redundant. There is an equal probability that the developer forgot to return in the code. When this condition is detected, warn the user and let them figure out which one is wrong.

I believe it was named bad but there are a lot of rules with bad names. I think it's an epic story to rework them all to have simple and intuitive naming. But I personally need much more rules to know what we should merge or split and how we should rename, what properties we should make for them, etc.

I would then remove the requireReturnTypes rule (it's covered by the above rule)

requireReturnTypes is stupid rule for now that just warns user about return statements without types like: @return Description. ;-\

and have checkRedundantReturns only warn about @return {undefined}.

ais you proposes to have in mind that function returns void in any case. It means merging current checkRedundantReturns and checkReturnTypes rules.

I think it's non sense, it will be the same size as requireReturnTypes rule. Does these rules have sense without some other return checks?

I mean: who da hell forgetting to write type in return tag and who typing {undefined} there? ;-D And why we should split this logic out of other rules?