C99/C++ scoping rules by chriseth · Pull Request #3476 · argotorg/solidity

chriseth · 2018-02-09T15:57:26Z

Still to do:

update ExpressionCompiler tests
only activate new rules for 0.5.0
for loops: make body and post-expression sub-scope of init statement
documentation
check that int a = a is impossible. (Self assignment at declaration (ambiguous construct) #2495)
check that { a = 7; uint a;} is impossible.

chriseth · 2018-02-15T08:21:07Z

What should be the semantics of the following snippet? With the current scoping, it will create an error, but only a warning with the new scoping, because in the new scoping, it is shadowing and not redeclaring. With the current implementation, though, the assignment (*) will assign to the second variable, not the first. I guess at least that last behaviour should be fixed.

uint x = 3;
{
  x = 2; // (*)
  uint x = 8;
}

axic · 2018-02-15T09:48:31Z

It is debatable whether we want to support shadowing.

With the current implementation, though, the assignment (*) will assign to the second variable, not the first.

That sounds like a bug.

chriseth · 2018-02-15T10:13:25Z

Yes, I hope I just fixed it.

chriseth · 2018-02-15T14:46:50Z

Sorry, changed it again because a similar functionality is needed in another PR.

This is ready for review.

axic · 2018-02-26T19:48:22Z

test/libsolidity/SolidityEndToEndTest.cpp

+				{
+					x = 3; // This should still assign to the outer variable
+					uint x;
+					x = 4; // This should assign to the new one


Perhaps another test could do a return x here.

Added some more weird cases.

I think you forgot to push.

axic · 2018-02-26T19:51:10Z

test/libsolidity/SolidityNameAndTypeResolution.cpp

+		pragma experimental "v0.5.0";
+		contract test {
+			function f() public {
+				uint a = a;


Do we need a test for non-0.5.0 for this case? Perhaps it is covered somewhere.

axic · 2018-02-26T19:52:03Z

test/libsolidity/SolidityNameAndTypeResolution.cpp

+			}
+		}
+	)";
+	CHECK_WARNING(text, "Experimental features");


Will need to rebase and fix this if #3600 is merged first.

axic · 2018-02-26T19:52:57Z

test/libsolidity/SolidityNameAndTypeResolution.cpp

+		contract test {
+			function f() pure public {
+				for (uint x = 0; x < 10; x ++)
+					x = 2;


The only difference in this test compared to scoping_for is the lack of curly brackets. Is that intended?

Yes, that is exactly the intention.

axic · 2018-02-26T19:55:12Z

test/libsolidity/SolidityNameAndTypeResolution.cpp

+	char const* text = R"(
+		pragma experimental "v0.5.0";
+		contract B {
+			function f() mod(x) pure public { uint x = 7; }


Was this working before?

not sure, I can add a test.

axic · 2018-02-26T19:59:23Z

docs/control-structures.rst

+
+These rules are already introduced now as an experimental feature.
+
+As a consequence, the following examples will compile without warnings (apart from


Maybe worth mentioning this is not compiling currently due to variable redeclaration.

argh, weird, that should not happen.

Ah sorry, I used the wrong solc version, everything is fine.

axic · 2018-02-26T20:40:36Z

libsolidity/analysis/ReferencesResolver.h

@@ -90,6 +94,7 @@ class ReferencesResolver: private ASTConstVisitor
 	std::vector<ParameterList const*> m_returnParameters;
 	bool const m_resolveInsideCode;


What exactly is this setting?

Reference resolution happens in two stages: In the first, all declarations at contract level are registered, but nothing inside code. In the second, code is also examined. This is necessary since at contract level, any declaration can appear anywhere, and for some parts - I don't remember anymore what exactly - it is important at this stage whether an identifier is a type name or e.g. a function, but only inside code.

axic · 2018-02-27T14:47:40Z

test/libsolidity/SolidityEndToEndTest.cpp

+				x = 7;
+				{
+					x = 3;
+					uint x = x; // This should read from the outer and assign to the inner


That is super convoluted :)

I take that as a compliment :)
It will generate a shadowing warning, so I think we do not need to make it more strict.

LianaHus added the in progress label Feb 9, 2018

axic mentioned this pull request Feb 10, 2018

Compiler doesn't recognise declared variable scope #3480

Closed

chriseth force-pushed the scoping branch 5 times, most recently from 8e180f5 to d98bc4b Compare February 15, 2018 07:16

chriseth force-pushed the scoping branch 4 times, most recently from 81b3d6e to 3e8ab71 Compare February 15, 2018 14:46

chriseth mentioned this pull request Feb 15, 2018

Warn about using loose assembly. #3527

Merged

chriseth requested a review from axic February 15, 2018 17:03

chriseth force-pushed the scoping branch from 3e8ab71 to db7133c Compare February 21, 2018 17:05

axic reviewed Feb 26, 2018

View reviewed changes

chriseth force-pushed the scoping branch from db7133c to d0198fd Compare February 26, 2018 22:32

chriseth added 4 commits February 27, 2018 12:17

Make all lookups recursive by default.

53289e1

Scopes do not have to be declarations.

5f20129

Blocks and for loops can be scopes.

e6d48bb

Adjust tests.

69f5f2e

chriseth added 4 commits February 27, 2018 12:17

Tests for new scoping behaviour.

0005990

Changelog entry.

e227bdb

Enable C99-scoping with the 0.5.0-experimental pragma.

6b9dda0

Only active variables at the point of their declaration.

88a5c66

chriseth force-pushed the scoping branch from d0198fd to 0e4e804 Compare February 27, 2018 11:34

chriseth added 2 commits February 27, 2018 12:41

Documentation about new scoping rules.

6391a36

Some more scoping tests.

d64aa0e

chriseth force-pushed the scoping branch from 0e4e804 to d64aa0e Compare February 27, 2018 11:41

axic reviewed Feb 27, 2018

View reviewed changes

axic approved these changes Feb 27, 2018

View reviewed changes

chriseth merged commit 908b46e into develop Feb 27, 2018

axic deleted the scoping branch February 28, 2018 15:48

axic mentioned this pull request Apr 17, 2018

Self assignment at declaration (ambiguous construct) #2495

Closed


		These rules are already introduced now as an experimental feature.

		As a consequence, the following examples will compile without warnings (apart from

		@@ -90,6 +94,7 @@ class ReferencesResolver: private ASTConstVisitor
		std::vector<ParameterList const*> m_returnParameters;
		bool const m_resolveInsideCode;

Conversation

chriseth commented Feb 9, 2018 • edited by axic Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chriseth commented Feb 15, 2018

Uh oh!

axic commented Feb 15, 2018

Uh oh!

chriseth commented Feb 15, 2018

Uh oh!

chriseth commented Feb 15, 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!

chriseth Feb 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Comments

chriseth commented Feb 9, 2018 •

edited by axic

Loading

chriseth Feb 27, 2018 •

edited

Loading