Fix shadowing error for inline assembly.

chriseth · chriseth · commit aeb2438eab8d · 2021-08-17T12:12:56.000+02:00
diff --git a/Changelog.md b/Changelog.md
@@ -2,6 +2,7 @@
 
 Breaking changes:
  * `error` is now a keyword that can only be used for defining errors.
+ * Inline Assembly: Consider functions, function parameters and return variables for shadowing checks.
 
 
 ### 0.8.8 (unreleased)
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,20 @@ 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)
+			if (decl->location().hasText())
+				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/libyul/AsmAnalysis.cpp b/libyul/AsmAnalysis.cpp
@@ -323,7 +323,6 @@ vector<YulString> AsmAnalyzer::operator()(FunctionCall const& _funCall)
 			returnTypes = &_fun.returns;
 		}
 	}))
-	else
 	{
 		if (!validateInstructions(_funCall))
 			m_errorReporter.declarationError(
diff --git a/test/libsolidity/semanticTests/inlineAssembly/external_identifier_access_shadowing.sol b/test/libsolidity/semanticTests/inlineAssembly/external_identifier_access_shadowing.sol
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/smtCheckerTests/inline_assembly/assembly_local_storage_access_inside_function.sol b/test/libsolidity/smtCheckerTests/inline_assembly/assembly_local_storage_access_inside_function.sol
@@ -1,7 +1,7 @@
 contract C {
 	uint256 public z;
 
-	function f() public {
+	function g() public {
 		z = 42;
 		uint i = 32;
 		assembly {
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/external_identifier_access_shadowing.sol b/test/libsolidity/syntaxTests/inlineAssembly/external_identifier_access_shadowing.sol
@@ -9,4 +9,5 @@ contract C {
     }
 }
 // ----
+// DeclarationError 3859: (103-104): This declaration shadows a declaration outside the inline assembly block.
 // DeclarationError 6578: (123-124): Cannot access local Solidity variables from inside an inline assembly function.
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,8 +1,8 @@
 contract C {
     function f() public pure {
         assembly {
-            function f() {}
-            f := 1
+            function g() {}
+            g := 1
         }
     }
 }
diff --git a/test/libsolidity/syntaxTests/inlineAssembly/invalid/dot_in_fun_param.sol b/test/libsolidity/syntaxTests/inlineAssembly/invalid/dot_in_fun_param.sol
@@ -1,5 +1,5 @@
 contract C {
-  function f() public pure {
+  function g() public pure {
     assembly {
       function f(a., x.b) -> t.b, b.. {}
     }
diff --git a/test/libsolidity/syntaxTests/inlineAssembly/invalid/dot_in_multi_vardecl.sol b/test/libsolidity/syntaxTests/inlineAssembly/invalid/dot_in_multi_vardecl.sol
@@ -1,5 +1,5 @@
 contract C {
-  function f() public pure {
+  function g() public pure {
     assembly {
       function f() -> x, y, z {}
       let a., aa.b := f()
diff --git a/test/libsolidity/syntaxTests/inlineAssembly/invalid/illegal_names.sol b/test/libsolidity/syntaxTests/inlineAssembly/invalid/illegal_names.sol
@@ -49,7 +49,5 @@ contract C {
 // DeclarationError 4113: (595-600): The identifier name "super" is reserved.
 // 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.
 // DeclarationError 4113: (785-790): The identifier name "super" is reserved.
-// DeclarationError 3859: (785-790): This declaration shadows a declaration outside the inline assembly block.
 // DeclarationError 4113: (812-813): The identifier name "_" is reserved.
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/inlineAssembly/shadowing/local_variable_by_fun_arg.sol b/test/libsolidity/syntaxTests/inlineAssembly/shadowing/local_variable_by_fun_arg.sol
@@ -0,0 +1,10 @@
+contract C {
+    function f() public pure {
+        uint a;
+        assembly {
+            function g(a) {}
+        }
+    }
+}
+// ----
+// DeclarationError 3859: (102-103): This declaration shadows a declaration outside the inline assembly block.
diff --git a/test/libsolidity/syntaxTests/nameAndTypeResolution/shadowsBuiltin/illegal_names_assembly_identifier.sol b/test/libsolidity/syntaxTests/nameAndTypeResolution/shadowsBuiltin/illegal_names_assembly_identifier.sol
@@ -9,7 +9,5 @@ contract C {
 }
 // ----
 // DeclarationError 4113: (74-79): The identifier name "super" is reserved.
-// DeclarationError 3859: (74-79): This declaration shadows a declaration outside the inline assembly block.
 // DeclarationError 4113: (101-105): The identifier name "this" is reserved.
-// DeclarationError 3859: (101-105): This declaration shadows a declaration outside the inline assembly block.
 // DeclarationError 4113: (127-128): 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
`@@ -323,7 +323,6 @@ vector<YulString> AsmAnalyzer::operator()(FunctionCall const& _funCall)`
`323`	`323`	`returnTypes = &_fun.returns;`
`324`	`324`	`}`
`325`	`325`	`}))`
`326`		`- else`
`327`	`326`	`{`
`328`	`327`	`if (!validateInstructions(_funCall))`
`329`	`328`	`m_errorReporter.declarationError(`
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`	`}`