Warn on unused local variables by roadriverrail · Pull Request #2139 · argotorg/solidity

roadriverrail · 2017-04-20T15:11:37Z

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 · 2017-04-20T15:22:43Z

libsolidity/analysis/StaticAnalyzer.h

 	/// Flag that indicates whether a public function does not contain the "payable" modifier.
 	bool m_nonPayablePublic = false;
+
+  std::map<VariableDeclaration const*, int> m_localVarUseCount;


Please indent this properly.

axic · 2017-04-20T15:23:08Z

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

roadriverrail · 2017-04-20T15:26:18Z

@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?

chriseth · 2017-04-21T09:45:56Z

test/libsolidity/SolidityNameAndTypeResolution.cpp

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


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

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.

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.

Can you add a test for silencing?

@federicobond good point
@axic I will do that

chriseth · 2017-04-21T09:46:57Z

test/libsolidity/SolidityNameAndTypeResolution.cpp

+		int errorCount = 0;
+		for (auto const& currentError: errors)
+		{
+			if (currentError->type() != Error::Type::Warning)


If possible, I would prefer to keep this as strict as it is now. How many tests would need changes?

I don't have a solid count, but it's a significant portion of the tests in SolidityNameAndTypeResolution.cpp

I've gone ahead and silenced all the unused warnings in those tests. It's mostly un-naming parameters or adding meaningless statements to code blocks. A little ugly to me, but not bad.

chriseth · 2017-04-21T09:47:37Z

libsolidity/analysis/StaticAnalyzer.cpp

+	return true;
+}
+
+void StaticAnalyzer::endVisit(Identifier const&)


Please remove this here and in the header file (also endVisit for VariableDeclaration).

chriseth · 2017-04-21T09:56:20Z

libsolidity/analysis/StaticAnalyzer.cpp

+	// There's no real way to get a good tracking on unnamed variables in someone's
+	// code.  These can show up in the returns clause of a function declaration,
+
+	if (_variable.isLocalVariable() && _variable.name() != "")


Can you change the first part to solAssert(_variable.isLocalVariable(), "") because all of them should be local variables.

I see. A VariableDeclaration is never a state variable?

VariableDeclaration "stateVariable1" Type: uint256 Gas costs: 0 Source: "uint256 stateVariable1" ElementaryTypeName uint256 Source: "uint256"

Looks like state variables are VariableDeclarations, which is what I'd thought, and that's why I was using the first part to filter them off.

Ah, I see. Since all the work is scoped inside functions, perhaps it would be better to set a separate flag that says "we are inside a function" and then assert that the flag is set if and only if the variable is a local variable.

Sure. I can do that. I'd actually thought about using a flag like that, but I don't like to add flags if I don't have to, so I was just filtering on local variables.

chriseth · 2017-04-21T09:57:26Z

libsolidity/analysis/StaticAnalyzer.cpp

 void StaticAnalyzer::endVisit(FunctionDefinition const&)
 {
 	m_nonPayablePublic = false;
+	for (auto var = m_localVarUseCount.begin(); var != m_localVarUseCount.end(); ++var)


Can you use a range-based for?

for (auto const& var: m_localVarUseCount) if (var.second == 0) warning(var.first->location(), "Unused local variable");

chriseth · 2017-04-21T09:58:29Z

libsolidity/analysis/StaticAnalyzer.cpp

+{
+	if (auto var = dynamic_cast<VariableDeclaration const*>(_identifier.annotation().referencedDeclaration))
+	{
+		if (var->isLocalVariable() && var->name() != "")


I don't think you can reference a variable that does not have a name, because then the identifier would also have no name. So can you change this to solAssert(!var->name().empty, "")?

Hm. Good point. You can't visit an identifier of nothing.

chriseth · 2017-04-21T09:59:42Z

libsolidity/analysis/StaticAnalyzer.cpp

+	{
+		if (var->isLocalVariable() && var->name() != "")
+		{
+			if (m_localVarUseCount.find(var) != m_localVarUseCount.end())


This assumes that the variable declaration is visited first. Why do you need this check at all?
Please add a test for

contract c { function f() { a = 7; uint a; } }

Understood and will fix. I am curious, though, why Solidity doesn't enforce declaration-before-use. It seems to me like this doesn't make for more readable code.

It is a stupid rule inherited from javascript. We plan to "fix" that by adding syntax for let a: uint = 7.

chriseth · 2017-04-21T10:01:48Z

libsolidity/analysis/StaticAnalyzer.cpp

+
+	if (_variable.isLocalVariable() && _variable.name() != "")
+	{
+		// We don't have to check if the variable was already inserted


I'm not sure this is actually correct. You could do something like m_localVarUseCount[&_variable] - then the slot will be created if it does not exist and set to zero.

The comment is clearly ambiguous. I was attempting to say that we don't have to worry about something like:
uint a; uint a;

Because the parser would have already rejected it. I can just remove the comment. If redundant declarations had not already been filtered out, then m_localVarUseCount[&_variable] = 0 isn't particularly safe to use because it would cause the counter to reset to 0 when the next declaration was encountered.

Of course, I can't uncritically perform m_localVarUseCount[&_variable] = 0 if variables can be used before they're declared, but I can work around that.

axic · 2017-04-21T14:48:24Z

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 · 2017-04-23T21:23:52Z

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 · 2017-04-24T21:51:12Z

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 · 2017-04-24T21:52:04Z

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

axic · 2017-04-24T21:52:40Z

Changelog.md

 Features:
 * Implement the Standard JSON Input / Output API
 * Support ``interface`` contracts.
+ * Warns of unused local variables, parameters, and return parameters.


Should we prefix this with Static Analyser?

roadriverrail · 2017-04-24T21:59:55Z

Sure. I'll add the test. That shouldn't be an issue. I can also change Token to an interface. I think rather the rule of turning off the unused var warnings for all functions with no implementation will catch it all pretty cleanly.

…

On Mon, Apr 24, 2017 at 2:51 PM Alex Beregszaszi ***@***.***> wrote: 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); } — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#2139 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAlhYGImjOLIC8NcsOBbqUGj7708zS-pks5rzRlTgaJpZM4NDK84> .

roadriverrail · 2017-04-24T22:00:27Z

Yep. I'll do so after adding the test you requested and fixing up Changelog.

…

On Mon, Apr 24, 2017 at 2:53 PM Alex Beregszaszi ***@***.***> wrote: Can you please rebase this pull request (and also remove any extra merge commits while doing so)? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#2139 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAlhYMmqTjr__s22bfwJnKaD1Y6UdPyVks5rzRmGgaJpZM4NDK84> .

axic · 2017-04-24T22:06:37Z

libsolidity/analysis/StaticAnalyzer.cpp

+		{
+			warning(var.first->location(), "Unused local variable");
+		}
+	}


I think none of these brackets are needed.

axic · 2017-04-24T22:14:33Z

libsolidity/analysis/StaticAnalyzer.h

+
+	std::map<VariableDeclaration const*, int> m_localVarUseCount;
+
+  bool m_inFunction = false;


Have to be indented.

axic · 2017-04-24T22:15:02Z

libsolidity/analysis/StaticAnalyzer.h

 	/// Flag that indicates whether a public function does not contain the "payable" modifier.
 	bool m_nonPayablePublic = false;
+
+	std::map<VariableDeclaration const*, int> m_localVarUseCount;


Maybe a comment wouldn't hurt here saying counter starts from 1.

Really, the counter starts at 0 and is incremented on every use.

Yeah, it was a typo. Declaration is not part of "use".

So than a comment saying the counter starts from 0 :)

axic · 2017-04-24T22:15:39Z

test/libsolidity/SolidityNameAndTypeResolution.cpp

-			function fun() { uint x; }
-			function fun() { uint x; }
+			function fun() { uint x; x; }
+			function fun() { uint x; x; }


Cannot we actually just have an empty function body here?

axic · 2017-04-24T22:16:44Z

test/libsolidity/SolidityNameAndTypeResolution.cpp

+				uint a;
+			}
+		}
+  )";


axic · 2017-04-24T22:16:48Z

test/libsolidity/SolidityNameAndTypeResolution.cpp

+				var a = 1;
+			}
+		}
+  )";


axic · 2017-04-24T22:16:57Z

test/libsolidity/SolidityNameAndTypeResolution.cpp

+	char const* text = R"(
+		contract C {
+			function f() {
+			a = 7;


axic · 2017-04-24T22:18:01Z

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.

chriseth · 2017-04-24T17:27:25Z

libsolidity/analysis/StaticAnalyzer.cpp

 {
+	m_inFunction = false;
 	m_nonPayablePublic = false;
+	for (auto const& var : m_localVarUseCount)


Please remove space in front of : and braces below.

chriseth · 2017-04-25T08:32:56Z

libsolidity/analysis/StaticAnalyzer.cpp

+		if (auto var = dynamic_cast<VariableDeclaration const*>(_identifier.annotation().referencedDeclaration))
+		{
+			solAssert(!var->name().empty(), "");
+			if (var->isLocalVariable() )


Please remove space before closing parenthesis and braces below.

chriseth · 2017-04-25T08:35:40Z

std/StandardToken.sol

 		return supply;
 	}

-	function transfer(address _to, uint256 _value) returns (bool success) {


Hm, as you already also said at some point: It might be quite descriptive to keep these names (the variable might also just be named failure).
Perhaps we can count "return with value" as usage of the return variables?

I can code that up, but it feels like a real kludge that is avoiding the issue. The issue here is that return variables are getting used for two different things. The first is that they are used to create a variable you can assign a value to. The second is that they are strings which document the meaning of the return value. The compiler is very much written with the first use in mind. This is to say that the parser does not make a very strong distinction between an output parameter and any other locally declared variable.

So, I feel like if you're going to create a variable to document your output's meaning, you should probably also have to use the variable. I've gone ahead and made a change here to actually use success rather than remove it. It doesn't feel much harder to assign your output parameters and let the return happen automatically than to do a simple return.

If you don't like the way it looks, I'll look into "return with value" counting as a use of the return variables.

chriseth · 2017-04-25T08:36:30Z

test/libsolidity/SolidityNameAndTypeResolution.cpp

 		contract test {
 			uint256 stateVariable1;
-			function fun(uint256 arg1) { uint256 y; }
+			function fun(uint256) { uint256 y; }


This should still flag y as unused. Can you change that to use the variables instead?

I will. By the way, the reason the test passes is because there is only 1 warning, so the "multiple errors" check doesn't fail, and since the only reported "error" is a warning, it doesn't count as an error on the final check.

chriseth · 2017-04-25T08:37:12Z

test/libsolidity/SolidityNameAndTypeResolution.cpp

 		contract test {
-			function fun() { uint x; }
-			function fun() { uint x; }
+			function fun() { uint x; x; }


This will actually trigger another warning (in a different PR): Side-effect free statement.
I think you can silence both by using x = 1.

@axic noted we can also just make these empty function bodies.

chriseth · 2017-04-25T08:39:30Z

test/libsolidity/SolidityNameAndTypeResolution.cpp

 	ASTPointer<SourceUnit> sourceUnit;
 	char const* text = R"(
 		contract test {
-			function functionName(bytes32 input) returns (bytes32 out);


This change should not be necessary, right?

In light of a recent change to waive unused variable checks, that is correct.

chriseth · 2017-04-25T08:42:09Z

test/libsolidity/SolidityNameAndTypeResolution.cpp

-		contract C { function f() { var (x,y); } }
+		contract C { function f() { var (x,y); x;y;} }
 	)";
 	CHECK_ERROR(text, TypeError, "");


Please add the correct error message if you change this, so it will not change to a different error without us noticing.

chriseth · 2017-04-25T08:43:40Z

test/libsolidity/SolidityNameAndTypeResolution.cpp

+				uint a;
+			}
+		}
+  )";


Please indent this one tab (also in the other tests below).

roadriverrail · 2017-04-25T19:40:45Z

This should now be rebased, cleaned up, etc.

roadriverrail · 2017-04-25T20:23:36Z

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

roadriverrail · 2017-04-27T16:49:03Z

@chriseth @axic Any further comments?

chriseth · 2017-04-27T20:06:11Z

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

roadriverrail · 2017-04-27T20:45:31Z

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 · 2017-05-02T12:27:45Z

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

axic · 2017-05-02T14:51:18Z

@chriseth can you rebase please?

chriseth · 2017-05-02T14:57:43Z

Rebased.

axic · 2017-05-02T15:00:33Z

libsolidity/analysis/StaticAnalyzer.cpp

+	for (auto const& var: m_localVarUseCount)
+		if (var.second == 0)
+			warning(var.first->location(), "Unused local variable");
+}


m_localVarUseCount.clear() could be moved here.

It is safer to at least also keep it in visit(), but I'll add other cleaning calls here.

Following that logic shouldn't then the function be set to nullptr in an else case?

The new commit makes it consistent and will throw once we add nested function declarations.

axic · 2017-05-02T15:01:26Z

libsolidity/analysis/StaticAnalyzer.cpp

+bool StaticAnalyzer::visit(Identifier const& _identifier)
+{
+	if (m_currentFunction)
+	{


Is this parentheses needed?

will remove

axic · 2017-05-02T15:02:35Z

libsolidity/analysis/StaticAnalyzer.cpp

+			// but since [] will insert the default 0, we really just
+			// need to access the map here and let it do the rest on its
+			// own.
+			m_localVarUseCount[&_variable];


Would m_localVarUseCount[_variable] += 0; less confusing than the big text?

Will change and add a comment that it is not a no-op.

axic · 2017-05-02T15:04:41Z

std/StandardToken.sol

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


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

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

axic · 2017-05-02T15:06:05Z

test/libsolidity/SolidityNameAndTypeResolution.cpp

 		contract test {
 			uint256 variable;
-			function f() { uint32 variable; }
+			function f() { uint32 variable; variable = 2; }


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

axic · 2017-05-02T15:06:56Z

test/libsolidity/SolidityNameAndTypeResolution.cpp

 				uint[] storage x;
 				uint[] memory y;
 				uint[] memory z;
+				x;y;z;


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

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

axic · 2017-05-02T15:08:12Z

test/libsolidity/SolidityNameAndTypeResolution.cpp


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


Extra space after //?

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

axic · 2017-05-02T15:45:09Z

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"
}

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

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

axic reviewed Apr 20, 2017

View reviewed changes

chriseth reviewed Apr 21, 2017

View reviewed changes

axic reviewed Apr 24, 2017

View reviewed changes

test/libsolidity/SolidityNameAndTypeResolution.cpp Outdated

uint a;

}

}

)";

Copy link

Contributor

axic Apr 24, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indent.

axic reviewed Apr 24, 2017

View reviewed changes

test/libsolidity/SolidityNameAndTypeResolution.cpp Outdated

var a = 1;

}

}

)";

Copy link

Contributor

axic Apr 24, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indent.

axic reviewed Apr 24, 2017

View reviewed changes

test/libsolidity/SolidityNameAndTypeResolution.cpp Outdated

char const* text = R"(

contract C {

function f() {

a = 7;

Copy link

Contributor

axic Apr 24, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indent.

chriseth suggested changes Apr 25, 2017

View reviewed changes

chriseth added the nextrelease label May 2, 2017

chriseth approved these changes May 2, 2017

View reviewed changes

axic reviewed May 2, 2017

View reviewed changes

axic approved these changes May 2, 2017

View reviewed changes

roadriverrail and others added 7 commits May 3, 2017 11:26

Warn on unused local variables

a40c8cf

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

a6faa5a

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

Refactor: Combine bool and function pointer.

e0266b7

Revert changes to standard token.

1f058ea

More strict tests.

e3ed362

Cleanup, style and additional test.

230f51e

Fix tests.

f3bb735

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

chriseth removed the nextrelease label May 3, 2017


		std::map<VariableDeclaration const*, int> m_localVarUseCount;

		bool m_inFunction = false;

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

Conversation

roadriverrail commented Apr 20, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

axic commented Apr 20, 2017

Uh oh!

roadriverrail commented Apr 20, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

axic commented Apr 21, 2017

Uh oh!

roadriverrail commented Apr 23, 2017

Uh oh!

axic commented Apr 24, 2017

Uh oh!

axic commented Apr 24, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

roadriverrail commented Apr 24, 2017 via email

Uh oh!

roadriverrail commented Apr 24, 2017 via email

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment