From 838514a718d0435c919bee65abef9f37fd2136bd Mon Sep 17 00:00:00 2001 From: chriseth Date: Thu, 12 Aug 2021 14:47:30 +0200 Subject: [PATCH 1/2] Undo backwards-compatibly change that prevents external identifer resolving. --- libsolidity/analysis/TypeChecker.cpp | 7 ------- libyul/AsmAnalysis.cpp | 26 +------------------------- libyul/backends/evm/AbstractAssembly.h | 2 +- 3 files changed, 2 insertions(+), 33 deletions(-) diff --git a/libsolidity/analysis/TypeChecker.cpp b/libsolidity/analysis/TypeChecker.cpp index 7f36746d3..89f769420 100644 --- 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 index 1c0867568..f7378463a 100644 --- a/libyul/AsmAnalysis.cpp +++ b/libyul/AsmAnalysis.cpp @@ -146,13 +146,6 @@ vector 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 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,15 +323,6 @@ vector 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)) @@ -556,14 +540,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(*var)) m_errorReporter.typeError(2657_error, _variable.debugData->location, "Assignment requires variable."); else if (!m_activeVariables.count(&std::get(*var))) diff --git a/libyul/backends/evm/AbstractAssembly.h b/libyul/backends/evm/AbstractAssembly.h index 60a745bee..9e1a28dab 100644 --- a/libyul/backends/evm/AbstractAssembly.h +++ b/libyul/backends/evm/AbstractAssembly.h @@ -117,7 +117,7 @@ public: 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). From aeb2438eab8d494a63ba8a32a0ba59980f52dc0a Mon Sep 17 00:00:00 2001 From: chriseth Date: Wed, 4 Aug 2021 12:10:49 +0200 Subject: [PATCH 2/2] Fix shadowing error for inline assembly. --- Changelog.md | 1 + libsolidity/analysis/ReferencesResolver.cpp | 36 +++++++++---------- libyul/AsmAnalysis.cpp | 1 - .../external_identifier_access_shadowing.sol | 12 ------- ...ssembly_storage_access_inside_function.sol | 4 +-- .../semanticTests/inlineAssembly/leave.sol | 4 +-- ...y_local_storage_access_inside_function.sol | 2 +- .../controlFlow/leave_inside_function.sol | 2 +- .../returning_function_declaration.sol | 2 +- .../reverting_function_declaration.sol | 2 +- .../assembly/returning_function.sol | 2 +- .../assembly/reverting_function.sol | 4 +-- .../external_identifier_access_shadowing.sol | 1 + .../function_call_invalid_argument_count.sol | 2 +- .../inlineAssembly/function_definition.sol | 2 +- .../invalid/assignment_to_function.sol | 4 +-- .../invalid/dot_in_fun_param.sol | 2 +- .../invalid/dot_in_multi_vardecl.sol | 2 +- .../inlineAssembly/invalid/illegal_names.sol | 2 -- .../syntaxTests/inlineAssembly/leave.sol | 2 +- .../shadowing/function_arguments.sol | 11 ++++++ .../shadowing/function_name.sol | 9 +++++ .../shadowing/local_variable_by_fun_arg.sol | 10 ++++++ .../illegal_names_assembly_identifier.sol | 2 -- .../syntaxTests/viewPureChecker/assembly.sol | 2 +- 25 files changed, 67 insertions(+), 56 deletions(-) delete mode 100644 test/libsolidity/semanticTests/inlineAssembly/external_identifier_access_shadowing.sol create mode 100644 test/libsolidity/syntaxTests/inlineAssembly/shadowing/function_arguments.sol create mode 100644 test/libsolidity/syntaxTests/inlineAssembly/shadowing/function_name.sol create mode 100644 test/libsolidity/syntaxTests/inlineAssembly/shadowing/local_variable_by_fun_arg.sol diff --git a/Changelog.md b/Changelog.md index 42af34693..b338285e9 100644 --- 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 index abaeada03..625336742 100644 --- 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 index f7378463a..43b78d6b2 100644 --- a/libyul/AsmAnalysis.cpp +++ b/libyul/AsmAnalysis.cpp @@ -323,7 +323,6 @@ vector 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 deleted file mode 100644 index f21dddfb4..000000000 --- a/test/libsolidity/semanticTests/inlineAssembly/external_identifier_access_shadowing.sol +++ /dev/null @@ -1,12 +0,0 @@ -contract C { - function f() public returns (uint x) { - assembly { - function g() -> f { f := 2 } - x := g() - } - } -} -// ==== -// compileViaYul: also -// ---- -// f() -> 2 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 index d94ac2637..f9038252b 100644 --- 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 index 7efabfcda..fab19f230 100644 --- 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 index 797f6ae76..753c4ec23 100644 --- 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 index 245106e13..9e990656a 100644 --- 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 index 6c4ee97d6..781851256 100644 --- 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 index 5b5738523..2dde607cc 100644 --- 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 index fbff48f46..065ff2260 100644 --- 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 index 11d8edfb8..023f92a6d 100644 --- 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 index fb073bd38..f0c41214c 100644 --- 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 index f3b35f8a3..f69223fac 100644 --- 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 index 4ad606f54..0f9c88fb6 100644 --- 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 index 9ce487295..b16e411ff 100644 --- 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 index e6644e9da..0ec2d8d69 100644 --- 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 index 58efb2de6..122eb2919 100644 --- 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 index 6c41b2296..64fb125d7 100644 --- 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 index 27edba47e..87cb3515b 100644 --- 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 new file mode 100644 index 000000000..ffaa1bb60 --- /dev/null +++ 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 new file mode 100644 index 000000000..8f19065dd --- /dev/null +++ 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 new file mode 100644 index 000000000..8d2959146 --- /dev/null +++ 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 index 4fc9b245b..5aee183ed 100644 --- 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 index cc8ac5e83..0926d2e14 100644 --- 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)) }