Fix shadowing error for inline assembly.

chriseth · chriseth · commit 85448c8bb46b · 2021-08-04T12:23:53.000+02:00
diff --git a/Changelog.md b/Changelog.md
@@ -17,6 +17,7 @@ Bugfixes:
  * Code Generator: Fix internal compiler error when passing a 32-byte hex literal or a zero literal to ``bytes.concat()`` by disallowing such literals.
  * Commandline Interface: Fix crash when a directory path is passed to ``--standard-json``.
  * Commandline Interface: Read JSON from standard input when ``--standard-json`` gets ``-`` as a file name.
+ * Inline Assembly: Consider functions, function parameters and return variables for shadowing checks.
  * Standard JSON: Include source location for errors in files with empty name.
  * Type Checker: Fix internal error and prevent static calls to unimplemented modifiers.
  * Yul Code Generator: Fix internal compiler error when using a long literal with bitwise negation.
diff --git a/libsolidity/analysis/ReferencesResolver.cpp b/libsolidity/analysis/ReferencesResolver.cpp
@@ -274,28 +274,8 @@ void ReferencesResolver::operator()(yul::Identifier const& _identifier)
 void ReferencesResolver::operator()(yul::VariableDeclaration const& _varDecl)
 {
 	for (auto const& identifier: _varDecl.variables)
-	{
 		validateYulIdentifierName(identifier.name, identifier.debugData->location);
 
-
-		if (
-			auto declarations = m_resolver.nameFromCurrentScope(identifier.name.str());
-			!declarations.empty()
-		)
-		{
-			SecondarySourceLocation ssl;
-			for (auto const* decl: declarations)
-				ssl.append("The shadowed declaration is here:", decl->location());
-			if (!ssl.infos.empty())
-				m_errorReporter.declarationError(
-					3859_error,
-					identifier.debugData->location,
-					ssl,
-					"This declaration shadows a declaration outside the inline assembly block."
-				);
-		}
-	}
-
 	if (_varDecl.value)
 		visit(*_varDecl.value);
 }
@@ -385,4 +365,19 @@ void ReferencesResolver::validateYulIdentifierName(yul::YulString _name, SourceL
 			_location,
 			"The identifier name \"" + _name.str() + "\" is reserved."
 		);
+
+	auto declarations = m_resolver.nameFromCurrentScope(_name.str());
+	if (!declarations.empty())
+	{
+		SecondarySourceLocation ssl;
+		for (auto const* decl: declarations)
+			ssl.append("The shadowed declaration is here:", decl->location());
+		if (!ssl.infos.empty())
+			m_errorReporter.declarationError(
+				3859_error,
+				_location,
+				ssl,
+				"This declaration shadows a declaration outside the inline assembly block."
+			);
+	}
 }
diff --git a/test/libsolidity/semanticTests/inlineAssembly/inline_assembly_storage_access_inside_function.sol b/test/libsolidity/semanticTests/inlineAssembly/inline_assembly_storage_access_inside_function.sol
@@ -7,11 +7,11 @@ contract C {
         uint256 off1;
         uint256 off2;
         assembly {
-            function f() -> o1 {
+            function g() -> o1 {
                 sstore(z.slot, 7)
                 o1 := y.offset
             }
-            off2 := f()
+            off2 := g()
         }
         assert(off2 == 2);
         return true;
diff --git a/test/libsolidity/semanticTests/inlineAssembly/leave.sol b/test/libsolidity/semanticTests/inlineAssembly/leave.sol
@@ -1,5 +1,5 @@
 contract C {
-    function f() public pure returns (uint w) {
+    function g() public pure returns (uint w) {
         assembly {
             function f() -> t {
                 t := 2
@@ -14,4 +14,4 @@ contract C {
 // compileToEwasm: also
 // compileViaYul: also
 // ----
-// f() -> 2
+// g() -> 2
diff --git a/test/libsolidity/syntaxTests/controlFlow/leave_inside_function.sol b/test/libsolidity/syntaxTests/controlFlow/leave_inside_function.sol
@@ -1,7 +1,7 @@
 contract C {
     function f() public pure {
         assembly {
-            function f() {
+            function g() {
                 // Make sure this doesn't trigger the unimplemented assertion in the control flow builder.
                 leave
             }
diff --git a/test/libsolidity/syntaxTests/controlFlow/localStorageVariables/assembly/returning_function_declaration.sol b/test/libsolidity/syntaxTests/controlFlow/localStorageVariables/assembly/returning_function_declaration.sol
@@ -1,7 +1,7 @@
 contract C {
     struct S { bool f; }
     S s;
-    function f() internal pure {
+    function g() internal pure {
         S storage c;
         // this should warn about unreachable code, but currently function flow is ignored
         assembly {
diff --git a/test/libsolidity/syntaxTests/controlFlow/localStorageVariables/assembly/reverting_function_declaration.sol b/test/libsolidity/syntaxTests/controlFlow/localStorageVariables/assembly/reverting_function_declaration.sol
@@ -1,7 +1,7 @@
 contract C {
     struct S { bool f; }
     S s;
-    function f() internal pure {
+    function g() internal pure {
         S storage c;
         // this could be allowed, but currently control flow for functions is not analysed
         assembly {
diff --git a/test/libsolidity/syntaxTests/controlFlow/storageReturn/assembly/returning_function.sol b/test/libsolidity/syntaxTests/controlFlow/storageReturn/assembly/returning_function.sol
@@ -1,7 +1,7 @@
 contract C {
     struct S { bool f; }
     S s;
-    function f() internal pure returns (S storage c) {
+    function g() internal pure returns (S storage c) {
         // this should warn about unreachable code, but currently function flow is ignored
         assembly {
             function f() { return(0, 0) }
diff --git a/test/libsolidity/syntaxTests/controlFlow/storageReturn/assembly/reverting_function.sol b/test/libsolidity/syntaxTests/controlFlow/storageReturn/assembly/reverting_function.sol
@@ -4,8 +4,8 @@ contract C {
     function f() internal pure returns (S storage c) {
         // this could be allowed, but currently control flow for functions is not analysed
         assembly {
-            function f() { revert(0, 0) }
-            f()
+            function g() { revert(0, 0) }
+            g()
         }
     }
 }
diff --git a/test/libsolidity/syntaxTests/inlineAssembly/function_call_invalid_argument_count.sol b/test/libsolidity/syntaxTests/inlineAssembly/function_call_invalid_argument_count.sol
@@ -1,5 +1,5 @@
 contract C {
-  function f() public pure {
+  function g() public pure {
     assembly {
       function f(a) {}
 
diff --git a/test/libsolidity/syntaxTests/inlineAssembly/function_definition.sol b/test/libsolidity/syntaxTests/inlineAssembly/function_definition.sol
@@ -1,7 +1,7 @@
 contract C {
   function f() pure public {
     assembly {
-      function f (a, b  , c ) ->    y,x,z {
+      function g (a, b  , c ) ->    y,x,z {
       }
     }
   }
diff --git a/test/libsolidity/syntaxTests/inlineAssembly/invalid/assignment_to_function.sol b/test/libsolidity/syntaxTests/inlineAssembly/invalid/assignment_to_function.sol
@@ -1,10 +1,10 @@
 contract C {
     function f() public pure {
         assembly {
-            function f() {}
-            f := 1
+            function g() {}
+            g := 1
         }
     }
 }
 // ----
-// TypeError 2657: (103-104): Assignment requires variable.
+// DeclarationError 3859: (75-90): This declaration shadows a declaration outside the inline assembly block.
diff --git a/test/libsolidity/syntaxTests/inlineAssembly/invalid/dot_in_fun_param.sol b/test/libsolidity/syntaxTests/inlineAssembly/invalid/dot_in_fun_param.sol
@@ -6,6 +6,7 @@ contract C {
   }
 }
 // ----
+// DeclarationError 3859: (63-97): This declaration shadows a declaration outside the inline assembly block.
 // DeclarationError 3927: (74-76): User-defined identifiers in inline assembly cannot contain '.'.
 // DeclarationError 3927: (78-81): User-defined identifiers in inline assembly cannot contain '.'.
 // DeclarationError 3927: (86-89): User-defined identifiers in inline assembly cannot contain '.'.
diff --git a/test/libsolidity/syntaxTests/inlineAssembly/invalid/dot_in_multi_vardecl.sol b/test/libsolidity/syntaxTests/inlineAssembly/invalid/dot_in_multi_vardecl.sol
@@ -7,5 +7,6 @@ contract C {
   }
 }
 // ----
+// DeclarationError 3859: (63-89): This declaration shadows a declaration outside the inline assembly block.
 // DeclarationError 3927: (100-102): User-defined identifiers in inline assembly cannot contain '.'.
 // DeclarationError 3927: (104-108): User-defined identifiers in inline assembly cannot contain '.'.
diff --git a/test/libsolidity/syntaxTests/inlineAssembly/invalid/illegal_names.sol b/test/libsolidity/syntaxTests/inlineAssembly/invalid/illegal_names.sol
@@ -40,13 +40,19 @@ contract C {
 }
 // ----
 // DeclarationError 4113: (105-136): The identifier name "this" is reserved.
+// DeclarationError 3859: (105-136): This declaration shadows a declaration outside the inline assembly block.
 // DeclarationError 4113: (149-181): The identifier name "super" is reserved.
+// DeclarationError 3859: (149-181): This declaration shadows a declaration outside the inline assembly block.
 // DeclarationError 4113: (194-222): The identifier name "_" is reserved.
 // DeclarationError 4113: (323-327): The identifier name "this" is reserved.
+// DeclarationError 3859: (323-327): This declaration shadows a declaration outside the inline assembly block.
 // DeclarationError 4113: (368-373): The identifier name "super" is reserved.
+// DeclarationError 3859: (368-373): This declaration shadows a declaration outside the inline assembly block.
 // DeclarationError 4113: (414-415): The identifier name "_" is reserved.
 // DeclarationError 4113: (546-550): The identifier name "this" is reserved.
+// DeclarationError 3859: (546-550): This declaration shadows a declaration outside the inline assembly block.
 // DeclarationError 4113: (595-600): The identifier name "super" is reserved.
+// DeclarationError 3859: (595-600): This declaration shadows a declaration outside the inline assembly block.
 // DeclarationError 4113: (645-646): The identifier name "_" is reserved.
 // DeclarationError 4113: (759-763): The identifier name "this" is reserved.
 // DeclarationError 3859: (759-763): This declaration shadows a declaration outside the inline assembly block.
diff --git a/test/libsolidity/syntaxTests/inlineAssembly/leave.sol b/test/libsolidity/syntaxTests/inlineAssembly/leave.sol
@@ -1,7 +1,7 @@
 contract C {
   function f() public pure {
     assembly {
-      function f() {
+      function g() {
         leave
       }
     }
diff --git a/test/libsolidity/syntaxTests/inlineAssembly/shadowing/function_arguments.sol b/test/libsolidity/syntaxTests/inlineAssembly/shadowing/function_arguments.sol
@@ -0,0 +1,11 @@
+contract C {
+    uint x;
+    function f() public pure {
+        assembly {
+            function g(f) -> x {}
+        }
+    }
+}
+// ----
+// DeclarationError 3859: (98-99): This declaration shadows a declaration outside the inline assembly block.
+// DeclarationError 3859: (104-105): This declaration shadows a declaration outside the inline assembly block.
diff --git a/test/libsolidity/syntaxTests/inlineAssembly/shadowing/function_name.sol b/test/libsolidity/syntaxTests/inlineAssembly/shadowing/function_name.sol
@@ -0,0 +1,9 @@
+contract C {
+    function f() public pure {
+        assembly {
+            function f() {}
+        }
+    }
+}
+// ----
+// DeclarationError 3859: (75-90): This declaration shadows a declaration outside the inline assembly block.
diff --git a/test/libsolidity/syntaxTests/nameAndTypeResolution/shadowsBuiltin/illegal_names_assembly_functions.sol b/test/libsolidity/syntaxTests/nameAndTypeResolution/shadowsBuiltin/illegal_names_assembly_functions.sol
@@ -12,5 +12,7 @@ contract C {
 }
 // ----
 // DeclarationError 4113: (75-106): The identifier name "this" is reserved.
+// DeclarationError 3859: (75-106): This declaration shadows a declaration outside the inline assembly block.
 // DeclarationError 4113: (119-151): The identifier name "super" is reserved.
+// DeclarationError 3859: (119-151): This declaration shadows a declaration outside the inline assembly block.
 // DeclarationError 4113: (164-192): The identifier name "_" is reserved.
diff --git a/test/libsolidity/syntaxTests/viewPureChecker/assembly.sol b/test/libsolidity/syntaxTests/viewPureChecker/assembly.sol
@@ -12,7 +12,7 @@ contract C {
         assembly { for {} 1 { pop(sload(0)) } { } pop(gas()) }
     }
     function h() view public {
-        assembly { function g() { pop(blockhash(20)) } }
+        assembly { function g1() { pop(blockhash(20)) } }
     }
     function i() public {
         assembly { pop(call(0, 1, 2, 3, 4, 5, 6)) }

Original file line number	Diff line number	Diff line change
`@@ -7,11 +7,11 @@ contract C {`
`7`	`7`	`uint256 off1;`
`8`	`8`	`uint256 off2;`
`9`	`9`	`assembly {`
`10`		`- function f() -> o1 {`
	`10`	`+ function g() -> o1 {`
`11`	`11`	`sstore(z.slot, 7)`
`12`	`12`	`o1 := y.offset`
`13`	`13`	`}`
`14`		`- off2 := f()`
	`14`	`+ off2 := g()`
`15`	`15`	`}`
`16`	`16`	`assert(off2 == 2);`
`17`	`17`	`return true;`
Original file line number	Diff line number	Diff line change
`@@ -1,7 +1,7 @@`
`1`	`1`	`contract C {`
`2`	`2`	`function f() public pure {`
`3`	`3`	`assembly {`
`4`		`- function f() {`
	`4`	`+ function g() {`
`5`	`5`	`// Make sure this doesn't trigger the unimplemented assertion in the control flow builder.`
`6`	`6`	`leave`
`7`	`7`	`}`
Original file line number	Diff line number	Diff line change
`@@ -4,8 +4,8 @@ contract C {`
`4`	`4`	`function f() internal pure returns (S storage c) {`
`5`	`5`	`// this could be allowed, but currently control flow for functions is not analysed`
`6`	`6`	`assembly {`
`7`		`- function f() { revert(0, 0) }`
`8`		`- f()`
	`7`	`+ function g() { revert(0, 0) }`
	`8`	`+ g()`
`9`	`9`	`}`
`10`	`10`	`}`
`11`	`11`	`}`