Merge pull request #11735 from ethereum/shadowing_inlineasm

chriseth · web-flow · commit d247cd5b9ffa · 2021-08-17T13:18:18.000+02:00
Disallow shadowing for all identifiers in inline assembly
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/libsolidity/analysis/TypeChecker.cpp b/libsolidity/analysis/TypeChecker.cpp
@@ -744,13 +744,6 @@ bool TypeChecker::visit(InlineAssembly const& _inlineAssembly)
 		bool
 	) -> bool
 	{
-		if (_context == yul::IdentifierContext::NonExternal)
-		{
-			// Hack until we can disallow any shadowing: If we found an internal reference,
-			// clear the external references, so that codegen does not use it.
-			_inlineAssembly.annotation().externalReferences.erase(& _identifier);
-			return false;
-		}
 		auto ref = _inlineAssembly.annotation().externalReferences.find(&_identifier);
 		if (ref == _inlineAssembly.annotation().externalReferences.end())
 			return false;
diff --git a/libyul/AsmAnalysis.cpp b/libyul/AsmAnalysis.cpp
@@ -146,13 +146,6 @@ vector<YulString> AsmAnalyzer::operator()(Identifier const& _identifier)
 		}
 	}))
 	{
-		if (m_resolver)
-			// We found a local reference, make sure there is no external reference.
-			m_resolver(
-				_identifier,
-				yul::IdentifierContext::NonExternal,
-				m_currentScope->insideFunction()
-			);
 	}
 	else
 	{
@@ -315,7 +308,7 @@ vector<YulString> AsmAnalyzer::operator()(FunctionCall const& _funCall)
 
 		validateInstructions(_funCall);
 	}
-	else if (m_currentScope->lookup(_funCall.functionName.name, GenericVisitor{
+	else if (!m_currentScope->lookup(_funCall.functionName.name, GenericVisitor{
 		[&](Scope::Variable const&)
 		{
 			m_errorReporter.typeError(
@@ -330,16 +323,6 @@ vector<YulString> AsmAnalyzer::operator()(FunctionCall const& _funCall)
 			returnTypes = &_fun.returns;
 		}
 	}))
-	{
-		if (m_resolver)
-			// We found a local reference, make sure there is no external reference.
-			m_resolver(
-				_funCall.functionName,
-				yul::IdentifierContext::NonExternal,
-				m_currentScope->insideFunction()
-			);
-	}
-	else
 	{
 		if (!validateInstructions(_funCall))
 			m_errorReporter.declarationError(
@@ -556,14 +539,6 @@ void AsmAnalyzer::checkAssignment(Identifier const& _variable, YulString _valueT
 	bool found = false;
 	if (Scope::Identifier const* var = m_currentScope->lookup(_variable.name))
 	{
-		if (m_resolver)
-			// We found a local reference, make sure there is no external reference.
-			m_resolver(
-				_variable,
-				yul::IdentifierContext::NonExternal,
-				m_currentScope->insideFunction()
-			);
-
 		if (!holds_alternative<Scope::Variable>(*var))
 			m_errorReporter.typeError(2657_error, _variable.debugData->location, "Assignment requires variable.");
 		else if (!m_activeVariables.count(&std::get<Scope::Variable>(*var)))
diff --git a/libyul/backends/evm/AbstractAssembly.h b/libyul/backends/evm/AbstractAssembly.h
@@ -117,7 +117,7 @@ class AbstractAssembly
 	virtual void markAsInvalid() = 0;
 };
 
-enum class IdentifierContext { LValue, RValue, VariableDeclaration, NonExternal };
+enum class IdentifierContext { LValue, RValue, VariableDeclaration };
 
 /// Object that is used to resolve references and generate code for access to identifiers external
 /// to inline assembly (not used in standalone assembly mode).
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
`@@ -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`	`}`