Merge pull request #7817 from ethereum/bail-on-shadowing-state-vars

Report error on shadowing state variables

Changelog.md

@@ -22,6 +22,7 @@ Breaking changes:
  * Source mappings: Add "modifier depth" as a fifth field in the source mappings.
  * AST: Inline assembly is exported as structured JSON instead of plain string.
  * General: ``private`` cannot be used together with ``virtual``.
+ * Inheritance: State variable shadowing is now disallowed.
 
 Language Features:
  * Allow global enums and structs.

docs/060-breaking-changes.rst

@@ -38,6 +38,10 @@ This section lists purely syntactic changes that do not affect the behavior of e
   If the name contains a dot, its prefix up to the dot may not conflict with any declaration outside the inline
   assembly block.
 
+* State variable shadowing is now disallowed.  A derived contract can only
+  declare a state variable ``x``, if there is no visible state variable with
+  the same name in any of its bases.
+
 
 Semantic Only Changes
 =====================

docs/contracts/inheritance.rst

@@ -19,6 +19,10 @@ is compiled into the created contract. This means that all internal calls
 to functions of base contracts also just use internal function calls
 (``super.f(..)`` will use JUMP and not a message call).
 
+State variable shadowing is considered as an error.  A derived contract can
+only declare a state variable ``x``, if there is no visible state variable
+with the same name in any of its bases.
+
 The general inheritance system is very similar to
 `Python's <https://docs.python.org/3/tutorial/classes.html#inheritance>`_,
 especially concerning multiple inheritance, but there are also

libsolidity/analysis/NameAndTypeResolver.cpp

@@ -345,10 +345,6 @@ void NameAndTypeResolver::importInheritedScope(ContractDefinition const& _base)
 					Declaration const* conflictingDeclaration = m_currentScope->conflictingDeclaration(*declaration);
 					solAssert(conflictingDeclaration, "");
 
-					// Usual shadowing is not an error
-					if (dynamic_cast<VariableDeclaration const*>(declaration) && dynamic_cast<VariableDeclaration const*>(conflictingDeclaration))
-						continue;
-
 					// Usual shadowing is not an error
 					if (dynamic_cast<ModifierDefinition const*>(declaration) && dynamic_cast<ModifierDefinition const*>(conflictingDeclaration))
 						continue;

test/libsolidity/SolidityEndToEndTest.cpp

@@ -5972,38 +5972,6 @@ BOOST_AUTO_TEST_CASE(invalid_enum_as_external_arg)
 	ABI_CHECK(callContractFunction("test()"), encodeArgs());
 }
 
-
-BOOST_AUTO_TEST_CASE(proper_order_of_overwriting_of_attributes)
-{
-	// bug #1798
-	char const* sourceCode = R"(
-		contract init {
-			function isOk() public virtual returns (bool) { return false; }
-			bool public ok = false;
-		}
-		contract fix {
-			function isOk() public virtual returns (bool) { return true; }
-			bool public ok = true;
-		}
-
-		contract init_fix is init, fix {
-			function checkOk() public returns (bool) { return ok; }
-			function isOk() public override (init, fix) returns (bool) { return super.isOk(); }
-		}
-		contract fix_init is fix, init {
-			function checkOk() public returns (bool) { return ok; }
-			function isOk() public override (init, fix) returns (bool) { return super.isOk(); }
-		}
-	)";
-	compileAndRun(sourceCode, 0, "init_fix");
-	ABI_CHECK(callContractFunction("isOk()"), encodeArgs(true));
-	ABI_CHECK(callContractFunction("ok()"), encodeArgs(true));
-
-	compileAndRun(sourceCode, 0, "fix_init");
-	ABI_CHECK(callContractFunction("isOk()"), encodeArgs(false));
-	ABI_CHECK(callContractFunction("ok()"), encodeArgs(false));
-}
-
 BOOST_AUTO_TEST_CASE(struct_assign_reference_to_struct)
 {
 	char const* sourceCode = R"(
@@ -8052,42 +8020,6 @@ BOOST_AUTO_TEST_CASE(inherited_constant_state_var)
 	ABI_CHECK(callContractFunction("f()"), encodeArgs(u256(7)));
 }
 
-BOOST_AUTO_TEST_CASE(multiple_inherited_state_vars)
-{
-	char const* sourceCode = R"(
-		contract A {
-			uint x = 7;
-		}
-		contract B {
-			uint x = 9;
-		}
-		contract C is A, B {
-			function a() public returns (uint) {
-				return A.x;
-			}
-			function b() public returns (uint) {
-				return B.x;
-			}
-			function a_set(uint _x) public returns (uint) {
-				A.x = _x;
-				return 1;
-			}
-			function b_set(uint _x) public returns (uint) {
-				B.x = _x;
-				return 1;
-			}
-		}
-	)";
-
-	compileAndRun(sourceCode, 0, "C");
-	ABI_CHECK(callContractFunction("a()"), encodeArgs(u256(7)));
-	ABI_CHECK(callContractFunction("b()"), encodeArgs(u256(9)));
-	ABI_CHECK(callContractFunction("a_set(uint256)", u256(1)), encodeArgs(u256(1)));
-	ABI_CHECK(callContractFunction("b_set(uint256)", u256(3)), encodeArgs(u256(1)));
-	ABI_CHECK(callContractFunction("a()"), encodeArgs(u256(1)));
-	ABI_CHECK(callContractFunction("b()"), encodeArgs(u256(3)));
-}
-
 BOOST_AUTO_TEST_CASE(constant_string_literal)
 {
 	char const* sourceCode = R"(

test/libsolidity/smtCheckerTests/inheritance/state_variables_2.sol

@@ -2,12 +2,10 @@ pragma experimental SMTChecker;
 
 contract Base1 {
 	uint x;
-	uint private t;
 }
 
 contract Base2 is Base1 {
 	uint z;
-	uint private t;
 }
 
 contract C is Base2 {

test/libsolidity/syntaxTests/inheritance/override/override.sol

@@ -9,3 +9,4 @@ contract X is A {
 	function test2() internal override(A) returns (uint256) {}
 }
 // ----
+// DeclarationError: (171-198): Identifier already declared.

test/libsolidity/syntaxTests/inheritance/override/override_ambiguous.sol

@@ -11,4 +11,5 @@ abstract contract X is A, B {
 	function test() internal override returns (uint256) {}
 }
 // ----
+// DeclarationError: (257-284): Identifier already declared.
 // TypeError: (226-343): Derived contract must override function "foo". Function with the same name and parameter types defined in two or more base classes.

test/libsolidity/syntaxTests/inheritance/override/override_multiple.sol

@@ -1,5 +1,4 @@
 abstract contract A {
-	int public testvar;
 	function foo() internal virtual returns (uint256);
 	function test(uint8 _a) internal virtual returns (uint256) {}
 }
@@ -18,4 +17,3 @@ contract X is A, B, C, D {
 	function test() internal override returns (uint256) {}
 	function foo() internal override(A, B, C, D) returns (uint256) {}
 }
-// ----

test/libsolidity/syntaxTests/inheritance/override/override_multiple_duplicated.sol

@@ -20,6 +20,7 @@ abstract contract X is A, B, C, D {
 	function foo() internal override(A, C, B, B, B, D ,D) virtual returns (uint256);
 }
 // ----
+// DeclarationError: (529-556): Identifier already declared.
 // TypeError: (599-600): Duplicate contract "D" found in override list of "test".
 // TypeError: (672-673): Duplicate contract "B" found in override list of "foo".
 // TypeError: (675-676): Duplicate contract "B" found in override list of "foo".

test/libsolidity/syntaxTests/inheritance/override/override_multiple_missing.sol

@@ -1,5 +1,4 @@
 abstract contract A {
-	int public testvar;
 	function foo() internal virtual returns (uint256);
 	function test(uint8 _a) virtual internal returns (uint256);
 }
@@ -20,5 +19,5 @@ abstract contract X is A, B, C, D {
 	function foo() internal override(A, C) virtual returns (uint256);
 }
 // ----
-// TypeError: (584-601): Invalid contract specified in override list: C.
+// TypeError: (563-580): Invalid contract specified in override list: C.
-// TypeError: (654-668): Function needs to specify overridden contracts B and D.
+// TypeError: (633-647): Function needs to specify overridden contracts B and D.

test/libsolidity/syntaxTests/inheritance/override/override_public_vars.sol

@@ -1,7 +0,0 @@
-abstract contract A {
-	int public testvar;
-}
-abstract contract X is A {
-	int public override testvar;
-}
-// ----

test/libsolidity/syntaxTests/inheritance/override/override_return_mismatch.sol

@@ -1,5 +1,4 @@
 abstract contract A {
-	int public testvar;
 	function foo() internal virtual returns (uint256);
 }
 abstract contract B {
@@ -11,4 +10,4 @@ abstract contract X is A, B {
 	function test() internal override virtual returns (uint256);
 }
 // ----
-// TypeError: (224-347): Derived contract must override function "foo". Function with the same name and parameter types defined in two or more base classes.
+// TypeError: (203-326): Derived contract must override function "foo". Function with the same name and parameter types defined in two or more base classes.

test/libsolidity/syntaxTests/inheritance/override/override_type_mismatch.sol

@@ -1,5 +1,4 @@
 abstract contract A {
-	int public testvar;
 	function foo() internal virtual returns (uint256);
 	function test(uint8 _a) internal virtual returns (uint256);
 }
@@ -22,5 +21,5 @@ abstract contract X is A, B, C, D {
 	function foo() internal override(MyStruct, ENUM, A, B, C, D) virtual returns (uint256);
 }
 // ----
-// TypeError: (653-661): Expected contract but got struct X.MyStruct.
+// TypeError: (632-640): Expected contract but got struct X.MyStruct.
-// TypeError: (663-667): Expected contract but got enum X.ENUM.
+// TypeError: (642-646): Expected contract but got enum X.ENUM.

test/libsolidity/syntaxTests/inheritance/shadowing_base_state_vars.sol

@@ -0,0 +1,8 @@
+contract A {
+	uint i;
+}
+contract B is A {
+	uint i;
+}
+// ----
+// DeclarationError: (43-49): Identifier already declared.

test/libsolidity/syntaxTests/inheritance/shadowing_private_base_state_vars.sol

@@ -0,0 +1,6 @@
+contract A {
+	uint private i;
+}
+contract B is A {
+	uint i;
+}