Deprecate the var keyword in SyntaxChecker by jevogel · Pull Request #3425 · argotorg/solidity

jevogel · 2018-01-23T16:50:32Z

Fix #3301 by extending libsolidity/analysis/SyntaxChecker with a visitor for VariableDeclaration where the typeName is empty (means "var"). The compiler emits a warning for such instances, unless the experimental mode V050 is turned on, in which case it emits an error. This addition causes 7 errors in SolidityTests due to their use of the var keyword.

axic · 2018-01-24T02:16:08Z

Thank you!

Can you please add test cases (in test/libsolidity/SolidityNameAndTypeChecker) and update the change log?

jevogel · 2018-01-24T02:48:46Z

Sure. Should I do anything about the existing test cases that are failing due to their use of var?

axic · 2018-01-24T02:58:26Z

Please fix them :)

The ones which are specifically made to test var should use CHECK_MULTI_WARNING or something similar and must keep the var keyword.

In cases it is used by mistake, it should be replaced with a proper type (probably there aren't any cases of this).

chriseth · 2018-01-24T11:16:37Z

I wonder whether it might be better to already do that at parsing time, but I guess the problem is that we do not yet have access to the pragma at that point - is that correct?

axic · 2018-01-24T11:41:47Z

Yes, pragma is evaluated later. But this should be fine.

jevogel · 2018-01-24T17:43:57Z

I edited all test cases that were failing to check for the var deprecated warning. However, it looks like some of these might require modification to replace var in them because otherwise the test will not be checking what it should.

If you could take a look at the changes and let me know which test cases should be reworked, I can take a shot at it.

chriseth · 2018-01-24T19:30:14Z

test/libsolidity/SolidityNameAndTypeResolution.cpp

 		contract B is A { }
 	)";
+	// Error (TypeError): Definition of base has to precede definition of derived contract
+	// Warning: Use of var keyword is deprecated


Why did you put this here?

Not sure how that got in there, but I'll get rid of it.

chriseth · 2018-01-24T19:31:11Z

test/libsolidity/SolidityNameAndTypeResolution.cpp

 			}
 		}
 	)";
-	CHECK_WARNING(sourceCode, "uint8, which can hold values between 0 and 255");


It should be checked that this error message is present until we disallow var completely.

Can you clarify? CHECK_WARNING_ALLOW_MULTI seems to only allow the first warning to be checked, so if var is used in this test it will cause the first warning and it will not be possible to check for the size warning. Is there a way to check both?

I fear someone has to extend the macro...

I could look into this within the next week or so. Do you want to file it as a separate issue?

chriseth · 2018-01-24T19:31:45Z

test/libsolidity/SolidityNameAndTypeResolution.cpp

 			}
 		}
 	)";
-	CHECK_WARNING(text, "\"callcode\" has been deprecated in favour of \"delegatecall\"");


You can replace var with bool in this example.

Changing var to bool here gives me the message

Type function () payable returns (bool) is not implicitly convertible to expected type bool.

You might be able to go with just function f() pure public { address(0x12).callcode; } for now.

OK, then probably best to just remove the variable.

chriseth · 2018-01-24T19:32:10Z

test/libsolidity/SolidityNameAndTypeResolution.cpp

@@ -4838,7 +4850,10 @@ BOOST_AUTO_TEST_CASE(warn_about_callcode)
 			}


chriseth · 2018-01-24T19:32:36Z

test/libsolidity/SolidityNameAndTypeResolution.cpp

 		}
 	)";
-	CHECK_SUCCESS_NO_WARNINGS(text);
+	CHECK_WARNING(text, "Use of var keyword is deprecated");


function pure external g = this.h should work

Looks like the next block below tests this exactly, so I'll just remove this block.

chriseth · 2018-01-24T19:33:16Z

test/libsolidity/SolidityNameAndTypeResolution.cpp

@@ -6817,7 +6832,7 @@ BOOST_AUTO_TEST_CASE(function_types_sig)
 			}


I think you don't modify the test by adding a type for i.

chriseth · 2018-01-24T19:33:33Z

test/libsolidity/SolidityNameAndTypeResolution.cpp

 			}
 		}
 	)";
-	CHECK_WARNING(text, "\"sha3\" has been deprecated in favour of \"keccak256\"");


Please replace var by bytes32

chriseth · 2018-01-24T19:35:12Z

test/libsolidity/ViewPureChecker.cpp

+	    // Error (TypeError):
+		//	Function declared as pure, but this expression (potentially) reads from the environment or state and thus requires "view"
+		CHECK_WARNING_ALLOW_MULTI(
 			"contract C { function f() pure public { var x = " + x + "; x; } }",


This is a tricky one. Perhaps you could turn this vector of expressions into a vector of pairs of expressions and their type, but it might not work for all of them because some types do not have a name.

I took a stab at this but ran into some problems. Maybe you can spot the root cause? gist.

chriseth · 2018-01-24T19:35:38Z

test/libsolidity/ViewPureChecker.cpp

 			function() payable public {}
 		}
 	)";
-	CHECK_SUCCESS_NO_WARNINGS(text);


Please replace var by the proper types here.

jevogel · 2018-01-24T22:05:37Z

Made some progress on fixing the tests, but ran into some problems with the ViewPureChecker. Here are the errors I'm getting when running the tests: gist.

jevogel · 2018-02-02T16:55:14Z

I am a git noob and inadvertently polluted this pull request with a ton of commits from develop. Should I just close this PR and start over?

axic · 2018-02-02T17:21:31Z

Please don't close. Just run git rebase upstream/develop if upstream is the upstream.

Assuming you're local git copy has origin as your repo, then do the following:

$ git remote add upstream https://github.com/ethereum/solidity
$ git fetch upstream
$ get rebase upstream/develop

jevogel · 2018-02-02T18:14:46Z

I did the rebase but all of the commits are still showing up in the history of this PR. Is that a problem?

axic · 2018-02-02T18:17:29Z

It seems to be correct for me. Refresh github a few times, sometimes it can take a while.

jevogel · 2018-02-02T18:24:20Z

Awesome, that worked. Thanks for your patience!

chriseth · 2018-02-09T22:58:43Z

Started #3479 to allow searching in a set of errors.

chriseth · 2018-02-10T12:45:59Z

All tests should work now, but we still need to take a look at the expectations:

Some tests now search for "var is deprecated", although what they actually should do is check that there is no other warning apart from "var is deprecated".

We should change the 'multi-error' feature such that we list all errors/warnings that occur (i.e. the exact same set), and not only search for one to be present.

chriseth · 2018-02-10T12:46:55Z

test/libsolidity/ViewPureChecker.cpp

 	{
 		CHECK_ERROR(
-			"contract C { function f() pure public { var x = " + x + "; x; } }",
+			"contract C { function f() pure public { " + x + "; } }",


The code was initially like that to avoid the "statement does not have an effect" warning, but it seems to be no problem.

chriseth · 2018-02-12T21:25:09Z

Rebased onto the new multi-error testing framework. Will adjust the expectations tomorrow.

axic · 2018-02-12T21:33:52Z

@chriseth was it intended to drop all the other changes it had? I assume all of that was obsoleted by the new framework.

chriseth · 2018-02-12T21:36:27Z

Yes, it was intended, I would rather restart adding the expectations instead of going back and forth in the previous commits.

… feature)

axic · 2018-02-13T02:30:11Z

Made the bear minimum changes required in tests. Many other tests could be cleaned up too.

LianaHus added the needs review label Jan 23, 2018

axic added the nextrelease label Jan 24, 2018

chriseth reviewed Jan 24, 2018

View reviewed changes

test/libsolidity/SolidityNameAndTypeResolution.cpp

@@ -4838,7 +4850,10 @@ BOOST_AUTO_TEST_CASE(warn_about_callcode)

}

Copy link

Contributor

chriseth Jan 24, 2018

Choose a reason for hiding this comment

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

same here

chriseth reviewed Jan 24, 2018

View reviewed changes

chriseth reviewed Feb 10, 2018

View reviewed changes

Deprecate the var keyword (and mark it an error as experimental 0.5.0…

dc5ad89

… feature)

axic added 2 commits February 12, 2018 22:37

Do not use var where not neccessary in tests

3ddbf16

Adjust tests for multiple errors with the var keyword

573aa01

axic approved these changes Feb 13, 2018

View reviewed changes

chriseth approved these changes Feb 13, 2018

View reviewed changes

chriseth merged commit 729c6a9 into argotorg:develop Feb 13, 2018

LianaHus removed needs review labels Feb 13, 2018

		@@ -4838,7 +4850,10 @@ BOOST_AUTO_TEST_CASE(warn_about_callcode)
		}

		@@ -6817,7 +6832,7 @@ BOOST_AUTO_TEST_CASE(function_types_sig)
		}

Conversation

jevogel commented Jan 23, 2018

Uh oh!

axic commented Jan 24, 2018

Uh oh!

jevogel commented Jan 24, 2018

Uh oh!

axic commented Jan 24, 2018

Uh oh!

chriseth commented Jan 24, 2018

Uh oh!

axic commented Jan 24, 2018

Uh oh!

jevogel commented Jan 24, 2018

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!

jevogel commented Jan 24, 2018

Uh oh!

jevogel commented Feb 2, 2018

Uh oh!

axic commented Feb 2, 2018

Uh oh!

jevogel commented Feb 2, 2018

Uh oh!

axic commented Feb 2, 2018

Uh oh!

jevogel commented Feb 2, 2018

Uh oh!

chriseth commented Feb 9, 2018

Uh oh!

chriseth commented Feb 10, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chriseth commented Feb 12, 2018

Uh oh!

axic commented Feb 12, 2018

Uh oh!

chriseth commented Feb 12, 2018

Uh oh!

axic commented Feb 13, 2018

Uh oh!

Reviewers

Assignees

Labels