Warn about unused local variables

[ethers]

• warn about unused arguments
• warn about local variables that are never accessed as rvalues
• warn about return values that are never assigned
• warn about unnamed return values in case there is only a return statement without arguments.

---

Related to #708

    function foo() returns (uint r) {
        // bunch of code and r is never set
        return 10;
    }

Can the compiler force an error that r is unused?

Even worse is if an explicit return was forgotten:

    function foo() returns (uint r) {
        // bunch of code and r is never set
    }

0 would always be returned without any warning.

The code would be much easier to read as:

    function foo() returns (uint) {
        // bunch of code
        return 10;
    }

[Denton-L]

I don't think this should throw an error because errors should only be reserved for when the compiler can't go on.

However, you bring up a good point. I believe that there should be warnings for unused local variables in general. This is a common thing that most other compilers will warn about.

[redsquirrel]

Related #677

[axic]

Should this be extended to unreferenced, non-public storage variables?

[ethers]

Should this be extended to unreferenced, non-public storage variables?

Yes I think so. If it's all fixable in the same commit then this issue is fine, otherwise a separate issue could be better and I can improve the title on this one, to mention the unused variable in the return declaration.

[chriseth]

Unreferenced local variables have their use in base contracts.

[roadriverrail]

I started to look into this, focusing on the local variables case first. I was able, I think, to ensure the expression compiler marks every variable it uses. It looks like I'd then need to actually implement a new AST visitor to sweep through the AST and make note of all the variables not marked as used by the ExpressionCompiler. Before I go crazy writing a new visitor, though, I thought it might be prudent to ask if there's a simpler way I didn't notice.

[chriseth]

That sounds about right, great!
The expression compiler is not the correct place to add this check, all these checks should be done before the code generation phase.

I guess the correct class to add these two passes is StaticAnalyzer. You might want to refactor it so that the different aspects of the analysis are better separated, but I guess it is still fine that way for now.

[roadriverrail]

I had thought about using the StaticAnalyzer, but I know there is some amount of optimization in solc, so I wanted to try to catch it late in the game. But I see your point. I'll switch things up.

…

On Wed, Jan 25, 2017 at 11:36 AM chriseth ***@***.***> wrote: That sounds about right, great! The expression compiler is not the correct place to add this check, all these checks should be done before the code generation phase. I guess the correct class to add these two passes is StaticAnalyzer. You might want to refactor it so that the different aspects of the analysis are better separated, but I guess it is still fine that way for now. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#718 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAlhYMDtycXKPjWbIY83k7Ee58Cr2-x3ks5rV3oOgaJpZM4JL7Ve> .

[roadriverrail]

I've got a naive implementation of this, I think. Played around with it on some test code and it seems to be able to detect unused locals, unused parameters, and unused named return variables. I see you want warnings about unnamed return variables, which could be added, since I currently suppress them by refusing to track an unnamed variable. All of the above features were pretty easy because the language currently treats all parameters and return variables as just more local variables.

As for warning if you never assign to a named return variable, I don't think that'd be too hard to track now that I've got a feel for things. It seems like a logically different concern from unused variable tracking, so do we want that put in the same commit or a different one?

[chriseth]

@roadriverrail in my view, this is fully resolved although your PR states that it only partially solves this issue. Please reopen if you think something is still missing.

[pash7ka]

There has to be some way to dissable this warnings. Overwise we have issues like this: OpenZeppelin/openzeppelin-contracts#367
Also Ethereum Wallet 0.9.0 does not compile contracts with warnings.

[axic]

@pash7ka that has ben raised on Mist ("Ethereum Wallet") already: ethereum/mist#2797

[pash7ka]

@axic That issue in Mist is about handling warnings. But there are usecases where having unused variables in a function declaration is not a bug, but a part of contract's public API.
There has to be some way to declare it to compiler so that no warning is raised.

[axic]

One can always use uint256 /*value*/ if it needs to be stated.

I think also we should lift the unused variable requirement for interfaces:

interface I {
  function transfer(address from, uint amount);
}

[chriseth]

@axic yes, unused variables should not be warned about if the function does not have an implementation (also outside of interfaces).

[pash7ka]

@axic Thank you, uint256 /*value*/ is fine!