Merge pull request #6622 from ethereum/warn-this-super

Issue warning for variables called `super` or `this`

Changelog.md

@@ -50,6 +50,7 @@ Bugfixes:
  * SMTChecker: SSA control-flow did not take into account state variables that were modified inside inlined functions that were called inside branches.
  * Type System: Use correct type name for contracts in event parameters when used in libraries. This affected code generation.
  * Type System: Allow direct call to base class functions that have overloads.
+ * Type System: Warn about shadowing builtin variables if user variables are named ``this`` or ``super``.
  * Yul: Properly register functions and disallow shadowing between function variables and variables in the outside scope.
 
 

libsolidity/analysis/NameAndTypeResolver.cpp

@@ -37,16 +37,17 @@ namespace solidity
 {
 
 NameAndTypeResolver::NameAndTypeResolver(
-	vector<Declaration const*> const& _globals,
+	GlobalContext& _globalContext,
 	map<ASTNode const*, shared_ptr<DeclarationContainer>>& _scopes,
 	ErrorReporter& _errorReporter
 ) :
 	m_scopes(_scopes),
-	m_errorReporter(_errorReporter)
+	m_errorReporter(_errorReporter),
+	m_globalContext(_globalContext)
 {
 	if (!m_scopes[nullptr])
 		m_scopes[nullptr].reset(new DeclarationContainer());
-	for (Declaration const* declaration: _globals)
+	for (Declaration const* declaration: _globalContext.declarations())
 	{
 		solAssert(m_scopes[nullptr]->registerDeclaration(*declaration), "Unable to register global declaration.");
 	}
@@ -57,7 +58,7 @@ bool NameAndTypeResolver::registerDeclarations(SourceUnit& _sourceUnit, ASTNode
 	// The helper registers all declarations in m_scopes as a side-effect of its construction.
 	try
 	{
-		DeclarationRegistrationHelper registrar(m_scopes, _sourceUnit, m_errorReporter, _currentScope);
+		DeclarationRegistrationHelper registrar(m_scopes, _sourceUnit, m_errorReporter, m_globalContext, _currentScope);
 	}
 	catch (langutil::FatalError const&)
 	{
@@ -272,6 +273,10 @@ bool NameAndTypeResolver::resolveNamesAndTypesInternal(ASTNode& _node, bool _res
 		setScope(contract->scope());
 		solAssert(!!m_currentScope, "");
 
+		m_globalContext.setCurrentContract(*contract);
+		updateDeclaration(*m_globalContext.currentSuper());
+		updateDeclaration(*m_globalContext.currentThis());
+
 		for (ASTPointer<InheritanceSpecifier> const& baseContract: contract->baseContracts())
 			if (!resolveNamesAndTypes(*baseContract, true))
 				success = false;
@@ -452,11 +457,13 @@ DeclarationRegistrationHelper::DeclarationRegistrationHelper(
 	map<ASTNode const*, shared_ptr<DeclarationContainer>>& _scopes,
 	ASTNode& _astRoot,
 	ErrorReporter& _errorReporter,
+	GlobalContext& _globalContext,
 	ASTNode const* _currentScope
 ):
 	m_scopes(_scopes),
 	m_currentScope(_currentScope),
-	m_errorReporter(_errorReporter)
+	m_errorReporter(_errorReporter),
+	m_globalContext(_globalContext)
 {
 	_astRoot.accept(*this);
 	solAssert(m_currentScope == _currentScope, "Scopes not correctly closed.");
@@ -560,6 +567,10 @@ bool DeclarationRegistrationHelper::visit(ImportDirective& _import)
 
 bool DeclarationRegistrationHelper::visit(ContractDefinition& _contract)
 {
+	m_globalContext.setCurrentContract(_contract);
+	m_scopes[nullptr]->registerDeclaration(*m_globalContext.currentThis(), nullptr, false, true);
+	m_scopes[nullptr]->registerDeclaration(*m_globalContext.currentSuper(), nullptr, false, true);
+
 	registerDeclaration(_contract, true);
 	_contract.annotation().canonicalName = currentCanonicalName();
 	return true;

libsolidity/analysis/NameAndTypeResolver.h

@@ -23,6 +23,7 @@
 #pragma once
 
 #include <libsolidity/analysis/DeclarationContainer.h>
+#include <libsolidity/analysis/GlobalContext.h>
 #include <libsolidity/analysis/ReferencesResolver.h>
 #include <libsolidity/ast/ASTAnnotations.h>
 #include <libsolidity/ast/ASTVisitor.h>
@@ -53,7 +54,7 @@ public:
 	/// @param _scopes mapping of scopes to be used (usually default constructed), these
 	/// are filled during the lifetime of this object.
 	NameAndTypeResolver(
-		std::vector<Declaration const*> const& _globals,
+		GlobalContext& _globalContext,
 		std::map<ASTNode const*, std::shared_ptr<DeclarationContainer>>& _scopes,
 		langutil::ErrorReporter& _errorReporter
 	);
@@ -131,6 +132,7 @@ private:
 
 	DeclarationContainer* m_currentScope = nullptr;
 	langutil::ErrorReporter& m_errorReporter;
+	GlobalContext& m_globalContext;
 };
 
 /**
@@ -148,6 +150,7 @@ public:
 		std::map<ASTNode const*, std::shared_ptr<DeclarationContainer>>& _scopes,
 		ASTNode& _astRoot,
 		langutil::ErrorReporter& _errorReporter,
+		GlobalContext& _globalContext,
 		ASTNode const* _currentScope = nullptr
 	);
 
@@ -200,6 +203,7 @@ private:
 	ASTNode const* m_currentScope = nullptr;
 	VariableScope* m_currentFunction = nullptr;
 	langutil::ErrorReporter& m_errorReporter;
+	GlobalContext& m_globalContext;
 };
 
 }

libsolidity/interface/CompilerStack.cpp

@@ -260,7 +260,7 @@ bool CompilerStack::analyze()
 				noErrors = false;
 
 		m_globalContext = make_shared<GlobalContext>();
-		NameAndTypeResolver resolver(m_globalContext->declarations(), m_scopes, m_errorReporter);
+		NameAndTypeResolver resolver(*m_globalContext, m_scopes, m_errorReporter);
 		for (Source const* source: m_sourceOrder)
 			if (!resolver.registerDeclarations(*source->ast))
 				return false;
@@ -278,11 +278,8 @@ bool CompilerStack::analyze()
 			for (ASTPointer<ASTNode> const& node: source->ast->nodes())
 				if (ContractDefinition* contract = dynamic_cast<ContractDefinition*>(node.get()))
 				{
-					m_globalContext->setCurrentContract(*contract);
-					if (!resolver.updateDeclaration(*m_globalContext->currentThis())) return false;
-					if (!resolver.updateDeclaration(*m_globalContext->currentSuper())) return false;
-					if (!resolver.resolveNamesAndTypes(*contract)) return false;
 
+					if (!resolver.resolveNamesAndTypes(*contract)) return false;
 					// Note that we now reference contracts by their fully qualified names, and
 					// thus contracts can only conflict if declared in the same source file.  This
 					// already causes a double-declaration error elsewhere, so we do not report

test/libsolidity/Assembly.cpp

@@ -63,7 +63,8 @@ eth::AssemblyItems compileContract(std::shared_ptr<CharStream> _sourceCode)
 	BOOST_CHECK(!!sourceUnit);
 
 	map<ASTNode const*, shared_ptr<DeclarationContainer>> scopes;
-	NameAndTypeResolver resolver({}, scopes, errorReporter);
+	GlobalContext globalContext;
+	NameAndTypeResolver resolver(globalContext, scopes, errorReporter);
 	solAssert(Error::containsOnlyWarnings(errorReporter.errors()), "");
 	resolver.registerDeclarations(*sourceUnit);
 	for (ASTPointer<ASTNode> const& node: sourceUnit->nodes())

test/libsolidity/SolidityExpressionCompiler.cpp

@@ -94,8 +94,7 @@ Declaration const& resolveDeclaration(
 bytes compileFirstExpression(
 	const string& _sourceCode,
 	vector<vector<string>> _functions = {},
-	vector<vector<string>> _localVariables = {},
-	vector<shared_ptr<MagicVariableDeclaration const>> _globalDeclarations = {}
+	vector<vector<string>> _localVariables = {}
 )
 {
 	ASTPointer<SourceUnit> sourceUnit;
@@ -113,15 +112,11 @@ bytes compileFirstExpression(
 		BOOST_FAIL(msg);
 	}
 
-	vector<Declaration const*> declarations;
-	declarations.reserve(_globalDeclarations.size() + 1);
-	for (ASTPointer<Declaration const> const& variable: _globalDeclarations)
-		declarations.push_back(variable.get());
-
 	ErrorList errors;
 	ErrorReporter errorReporter(errors);
+	GlobalContext globalContext;
 	map<ASTNode const*, shared_ptr<DeclarationContainer>> scopes;
-	NameAndTypeResolver resolver(declarations, scopes, errorReporter);
+	NameAndTypeResolver resolver(globalContext, scopes, errorReporter);
 	resolver.registerDeclarations(*sourceUnit);
 
 	vector<ContractDefinition const*> inheritanceHierarchy;
@@ -598,10 +593,7 @@ BOOST_AUTO_TEST_CASE(blockhash)
 		}
 	)";
 
-	auto blockhashFun = TypeProvider::function(strings{"uint256"}, strings{"bytes32"},
-		FunctionType::Kind::BlockHash, false, StateMutability::View);
-
-	bytes code = compileFirstExpression(sourceCode, {}, {}, {make_shared<MagicVariableDeclaration>("blockhash", blockhashFun)});
+	bytes code = compileFirstExpression(sourceCode, {}, {});
 
 	bytes expectation({uint8_t(Instruction::PUSH1), 0x03,
 					   uint8_t(Instruction::BLOCKHASH)});
@@ -617,10 +609,7 @@ BOOST_AUTO_TEST_CASE(gas_left)
 			}
 		}
 	)";
-	bytes code = compileFirstExpression(
-		sourceCode, {}, {},
-		{make_shared<MagicVariableDeclaration>("gasleft", TypeProvider::function(strings(), strings{"uint256"}, FunctionType::Kind::GasLeft))}
-	);
+	bytes code = compileFirstExpression(sourceCode, {}, {});
 
 	bytes expectation = bytes({uint8_t(Instruction::GAS)});
 	BOOST_CHECK_EQUAL_COLLECTIONS(code.begin(), code.end(), expectation.begin(), expectation.end());

test/libsolidity/syntaxTests/nameAndTypeResolution/shadowsBuiltin/this_super.sol

@@ -0,0 +1,11 @@
+contract C {
+    function f() pure public {
+        uint super = 3;
+        uint this = 4;
+    }
+}
+// ----
+// Warning: (52-62): This declaration shadows a builtin symbol.
+// Warning: (76-85): This declaration shadows a builtin symbol.
+// Warning: (52-62): Unused local variable.
+// Warning: (76-85): Unused local variable.