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

Warn on unused local variables #2139

Merged
merged 7 commits into from May 3, 2017

Conversation

Projects
None yet
4 participants
@roadriverrail
Collaborator

roadriverrail commented Apr 20, 2017

Analyze functions for all local variables, parameters, and named
return variables which are never used in the function, and issue
a warning.

This partially resolves issue #718

@axic

This comment has been minimized.

Show comment
Hide comment
@axic

axic Apr 20, 2017

Member

Can you add a test for var a = 1; ?

Member

axic commented Apr 20, 2017

Can you add a test for var a = 1; ?

@roadriverrail

This comment has been minimized.

Show comment
Hide comment
@roadriverrail

roadriverrail Apr 20, 2017

Collaborator

@axic Sure. Just to ensure that we're all clear, I would claim that "var a = 1;" on its own does not count as using the variable. Correct?

Collaborator

roadriverrail commented Apr 20, 2017

@axic Sure. Just to ensure that we're all clear, I would claim that "var a = 1;" on its own does not count as using the variable. Correct?

{
char const* text = R"(
contract C {
function f(uint a) {

This comment has been minimized.

@chriseth

chriseth Apr 21, 2017

Contributor

Is it possible to silence the warning by using function f(uint) { ... }?

@chriseth

chriseth Apr 21, 2017

Contributor

Is it possible to silence the warning by using function f(uint) { ... }?

This comment has been minimized.

@roadriverrail

roadriverrail Apr 21, 2017

Collaborator

I checked, and it is. It's pretty nonsensical to have a parameter you don't name and don't use, but it does silence the warning.

@roadriverrail

roadriverrail Apr 21, 2017

Collaborator

I checked, and it is. It's pretty nonsensical to have a parameter you don't name and don't use, but it does silence the warning.

This comment has been minimized.

@federicobond

federicobond Apr 21, 2017

Contributor

It can be useful if you have either default implementations or overriden methods that don't make use of all parameters in the declared interface.

@federicobond

federicobond Apr 21, 2017

Contributor

It can be useful if you have either default implementations or overriden methods that don't make use of all parameters in the declared interface.

This comment has been minimized.

@axic

axic Apr 21, 2017

Member

Can you add a test for silencing?

@axic

axic Apr 21, 2017

Member

Can you add a test for silencing?

This comment has been minimized.

@roadriverrail

roadriverrail Apr 21, 2017

Collaborator

@federicobond good point
@axic I will do that

@roadriverrail

roadriverrail Apr 21, 2017

Collaborator

@federicobond good point
@axic I will do that

@axic

This comment has been minimized.

Show comment
Hide comment
@axic

axic Apr 21, 2017

Member

Just to ensure that we're all clear, I would claim that "var a = 1;" on its own does not count as using the variable. Correct?

Correct in my view. a is not used.

Member

axic commented Apr 21, 2017

Just to ensure that we're all clear, I would claim that "var a = 1;" on its own does not count as using the variable. Correct?

Correct in my view. a is not used.

@roadriverrail

This comment has been minimized.

Show comment
Hide comment
@roadriverrail

roadriverrail Apr 23, 2017

Collaborator

So, this is blowing the Travis CI build because Token.sol is full of "unused variables." I personally think that, in a function with no implementation, there's no point to policing unused variables, since often variable names help document the semantic value of the parameter to people who end up implementing the function. I'll see if I can find a way to suppress the warnings in this case.

Collaborator

roadriverrail commented Apr 23, 2017

So, this is blowing the Travis CI build because Token.sol is full of "unused variables." I personally think that, in a function with no implementation, there's no point to policing unused variables, since often variable names help document the semantic value of the parameter to people who end up implementing the function. I'll see if I can find a way to suppress the warnings in this case.

@axic

This comment has been minimized.

Show comment
Hide comment
@axic

axic Apr 24, 2017

Member

So, this is blowing the Travis CI build because Token.sol is full of "unused variables."

I think Token.sol should be an interface.

Can you add a test that interfaces are not checked for unused variables? Functions in interfaces cannot have bodies, so I assume it already works without problems.

Interface example:

interface Token {
  function transfer(address from, address to, uint value);
}
Member

axic commented Apr 24, 2017

So, this is blowing the Travis CI build because Token.sol is full of "unused variables."

I think Token.sol should be an interface.

Can you add a test that interfaces are not checked for unused variables? Functions in interfaces cannot have bodies, so I assume it already works without problems.

Interface example:

interface Token {
  function transfer(address from, address to, uint value);
}
@axic

This comment has been minimized.

Show comment
Hide comment
@axic

axic Apr 24, 2017

Member

Can you please rebase this pull request (and also remove any extra merge commits while doing so)?

Member

axic commented Apr 24, 2017

Can you please rebase this pull request (and also remove any extra merge commits while doing so)?

Show outdated Hide outdated Changelog.md
@roadriverrail

This comment has been minimized.

Show comment
Hide comment
@roadriverrail

roadriverrail Apr 24, 2017

Collaborator
Collaborator

roadriverrail commented Apr 24, 2017

@roadriverrail

This comment has been minimized.

Show comment
Hide comment
@roadriverrail

roadriverrail Apr 24, 2017

Collaborator
Collaborator

roadriverrail commented Apr 24, 2017

@axic

This comment has been minimized.

Show comment
Hide comment
@axic

axic Apr 24, 2017

Member

I can also change Token to an interface.

I wouldn't do that in this PR. That wasn't done yet because there's no release which has interfaces and also that would break older compilers relying on github.com/ethereum/solidity/std/Token.sol.

Member

axic commented Apr 24, 2017

I can also change Token to an interface.

I wouldn't do that in this PR. That wasn't done yet because there's no release which has interfaces and also that would break older compilers relying on github.com/ethereum/solidity/std/Token.sol.

@roadriverrail

This comment has been minimized.

Show comment
Hide comment
@roadriverrail

roadriverrail Apr 25, 2017

Collaborator

This should now be rebased, cleaned up, etc.

Collaborator

roadriverrail commented Apr 25, 2017

This should now be rebased, cleaned up, etc.

@roadriverrail

This comment has been minimized.

Show comment
Hide comment
@roadriverrail

roadriverrail Apr 25, 2017

Collaborator

(Also, I promise that I've fixed my .vimrc to see spaces/tabs correctly)

Collaborator

roadriverrail commented Apr 25, 2017

(Also, I promise that I've fixed my .vimrc to see spaces/tabs correctly)

@roadriverrail

This comment has been minimized.

Show comment
Hide comment
@roadriverrail

roadriverrail Apr 27, 2017

Collaborator

@chriseth @axic Any further comments?

Collaborator

roadriverrail commented Apr 27, 2017

@chriseth @axic Any further comments?

@chriseth

This comment has been minimized.

Show comment
Hide comment
@chriseth

chriseth Apr 27, 2017

Contributor

Could you also add code that return <something>; marks the return variable as used, while return; does not?

Contributor

chriseth commented Apr 27, 2017

Could you also add code that return <something>; marks the return variable as used, while return; does not?

@roadriverrail

This comment has been minimized.

Show comment
Hide comment
@roadriverrail

roadriverrail Apr 27, 2017

Collaborator

I can add that. I'm sure there's enough legacy code out there doing its returns that way that we kind of have to.

Collaborator

roadriverrail commented Apr 27, 2017

I can add that. I'm sure there's enough legacy code out there doing its returns that way that we kind of have to.

@chriseth

This comment has been minimized.

Show comment
Hide comment
@chriseth

chriseth May 2, 2017

Contributor

I hope you don't mind me hijacking this PR, I would like to get this into the next release.

Contributor

chriseth commented May 2, 2017

I hope you don't mind me hijacking this PR, I would like to get this into the next release.

@chriseth chriseth added the nextrelease label May 2, 2017

@axic

This comment has been minimized.

Show comment
Hide comment
@axic

axic May 2, 2017

Member

@chriseth can you rebase please?

Member

axic commented May 2, 2017

@chriseth can you rebase please?

@chriseth

This comment has been minimized.

Show comment
Hide comment
@chriseth

chriseth May 2, 2017

Contributor

Rebased.

Contributor

chriseth commented May 2, 2017

Rebased.

@@ -53,7 +53,7 @@ contract StandardToken is Token {
return true;

This comment has been minimized.

@axic

axic May 2, 2017

Member

Why is this function not caught by it? It has success, but is never set. Same goes for doTransfer.

@axic

axic May 2, 2017

Member

Why is this function not caught by it? It has success, but is never set. Same goes for doTransfer.

This comment has been minimized.

@chriseth

chriseth May 2, 2017

Contributor

We said that returning a value counts as using a variable.

@chriseth

chriseth May 2, 2017

Contributor

We said that returning a value counts as using a variable.

@@ -262,7 +262,7 @@ BOOST_AUTO_TEST_CASE(name_shadowing)
char const* text = R"(
contract test {
uint256 variable;
function f() { uint32 variable; }
function f() { uint32 variable; variable = 2; }

This comment has been minimized.

@axic

axic May 2, 2017

Member

Perhaps uint32 variable = 2; would be enough, though it makes no difference.

@axic

axic May 2, 2017

Member

Perhaps uint32 variable = 2; would be enough, though it makes no difference.

@@ -2570,6 +2570,7 @@ BOOST_AUTO_TEST_CASE(storage_location_local_variables)
uint[] storage x;
uint[] memory y;
uint[] memory z;
x;y;z;

This comment has been minimized.

@axic

axic May 2, 2017

Member

Will these trigger the empty statement warning? Also probably it could be spaced in three lines?

@axic

axic May 2, 2017

Member

Will these trigger the empty statement warning? Also probably it could be spaced in three lines?

This comment has been minimized.

@chriseth

chriseth May 2, 2017

Contributor

I actually think those might not even be necessary because most tests do not require "no warnings".

@chriseth

chriseth May 2, 2017

Contributor

I actually think those might not even be necessary because most tests do not require "no warnings".

// string literal
var i = true ? "hello" : "world";
i = "used"; //Avoid unused var warning

This comment has been minimized.

@axic

axic May 2, 2017

Member

Extra space after //?

@axic

axic May 2, 2017

Member

Extra space after //?

This comment has been minimized.

@chriseth

chriseth May 2, 2017

Contributor

We do not have a proper style in the tests anyway....

@chriseth

chriseth May 2, 2017

Contributor

We do not have a proper style in the tests anyway....

@axic

axic approved these changes May 2, 2017

@axic

This comment has been minimized.

Show comment
Hide comment
@axic

axic May 2, 2017

Member

Fails on this:

/home/travis/build/ethereum/solidity/test/libsolidity/SolidityNameAndTypeResolution.cpp(106): fatal error in "rational_unary_operation": Multiple errors found
/home/travis/build/ethereum/solidity/test/libsolidity/SolidityNameAndTypeResolution.cpp(129): fatal error in "rational_unary_operation": Unexpected exception.
{
	"component" : "general",
	"formattedMessage" : "Only \"Solidity\" is supported as a language.",
	"message" : "Only \"Solidity\" is supported as a language.",
	"severity" : "error",
	"type" : "JSONError"
}
Member

axic commented May 2, 2017

Fails on this:

/home/travis/build/ethereum/solidity/test/libsolidity/SolidityNameAndTypeResolution.cpp(106): fatal error in "rational_unary_operation": Multiple errors found
/home/travis/build/ethereum/solidity/test/libsolidity/SolidityNameAndTypeResolution.cpp(129): fatal error in "rational_unary_operation": Unexpected exception.
{
	"component" : "general",
	"formattedMessage" : "Only \"Solidity\" is supported as a language.",
	"message" : "Only \"Solidity\" is supported as a language.",
	"severity" : "error",
	"type" : "JSONError"
}

roadriverrail and others added some commits Apr 14, 2017

Warn on unused local variables
Analyze functions for all local variables, parameters, and named
return variables which are never used in the function, and issue
a warning.
Treat returns with expressions as return param use
There are many cases of code where the return parameters exist mostly
as a form of documentation.  This change ensures that they do not have
to be used in the function body so long as there is a return supplying
values

@chriseth chriseth merged commit 1aa0f77 into ethereum:develop May 3, 2017

1 of 2 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@chriseth chriseth removed the nextrelease label May 3, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment