duaraghav8 / solium-plugin-security

The Official Security Plugin for Ethlint (formerly Solium)
http://npmjs.com/package/solium-plugin-security
MIT License
44 stars 12 forks source link

Add no-named-returns rule #10

Closed nfeignon closed 7 years ago

nfeignon commented 7 years ago

In response to Augur bounty : "Functions must have an explicit return statement".

I'm not so sure about this one. It reports an error if there's no return statement at the top level of a function. Let me know what you think.

beaugunderson commented 7 years ago

there's another rule that disallows unreachable code, so I think for return to be correct it must be the last statement of the function (though I think it's fine to allow the no-unreachable-code rule to handle that case)

rather than iterating you could also use Array.prototype.some

duaraghav8 commented 7 years ago

@nfeignon also how should the rule behave in a case like

if (..) {
    return 100;
} else {
 return 900;
}

Above code guarantees that the function will return even though there are no top-level return statements.

(Or maybe I didn't understand the rule correctly?)

beaugunderson commented 7 years ago

maybe even easier to enforce “no unnecessary else” if there’s a return in the if?


Sent from my phone.

On Nov 11, 2017, at 21:07, Raghav Dua notifications@github.com wrote:

@nfeignon also how should the rule behave in a case like

if (..) { return 100; } else { return 900; } Above code guarantees that the function will return even though there are no top-level return statements

— You are receiving this because you commented. Reply to this email directly, view it on GitHub, or mute the thread.

nfeignon commented 7 years ago

I've implemented the handling of the If statement. I'm not sure I understand the rule correctly though.

As I've done it, it will raise an error for these contracts:

contract Foo
{ 
    function foo () { 
        while (1) { return; }
    }
}
contract Foo
{
    function foo () {
        if (1) {
            if (1) {
                return;
            } else {
                return;
            }
        } else {
            return;
        }
    }
}

I've added these two in the rejection tests but is this the wanted behavior?

What do you think?

tinybike commented 7 years ago

Hey guys, sorry for the unclear wording on this one. Clarification here: https://github.com/AugurProject/augur-bounties/issues/5#issuecomment-343760896

TLDR: this rule is just supposed to prohibit "named returns" which allow returning values without an explicit return statement. Let me know if this makes sense / if there's further clarification needed!

nfeignon commented 7 years ago

@tinybike Thanks for the clarification! It makes more sense now.

I've updated my PR accordingly. I've also renamed the rule to no-named-returns.