Merge pull request #7483 from ethereum/import-shadowing-source-location

Fixes source location in warning for shadowing import delcarations.

Changelog.md

@@ -16,6 +16,7 @@ Bugfixes:
  * Fix internal error when popping a dynamic storage array of mappings.
  * Yul Optimizer: Fix reordering bug in connection with shifted one and mul/div-instructions in for loop conditions.
  * Scanner: Fix multi-line natspec comment parsing with triple slashes when file is encoded with CRLF instead of LF.
+ * Name Resolver: Fix wrong source location when warning on shadowed aliases in import declarations.
 
 
 ### 0.5.11 (2019-08-12)

libsolidity/analysis/NameAndTypeResolver.cpp

@@ -91,13 +91,13 @@ bool NameAndTypeResolver::performImports(SourceUnit& _sourceUnit, map<string, So
 			if (!imp->symbolAliases().empty())
 				for (auto const& alias: imp->symbolAliases())
 				{
-					auto declarations = scope->second->resolveName(alias.first->name(), false);
+					auto declarations = scope->second->resolveName(alias.symbol->name(), false);
 					if (declarations.empty())
 					{
 						m_errorReporter.declarationError(
 							imp->location(),
 							"Declaration \"" +
-							alias.first->name() +
+							alias.symbol->name() +
 							"\" not found in \"" +
 							path +
 							"\" (referenced as \"" +
@@ -109,7 +109,7 @@ bool NameAndTypeResolver::performImports(SourceUnit& _sourceUnit, map<string, So
 					else
 						for (Declaration const* declaration: declarations)
 							if (!DeclarationRegistrationHelper::registerDeclaration(
-								target, *declaration, alias.second.get(), &imp->location(), true, false, m_errorReporter
+								target, *declaration, alias.alias.get(), &alias.location, true, false, m_errorReporter
 							))
 								error = true;
 				}
@@ -523,7 +523,7 @@ bool DeclarationRegistrationHelper::registerDeclaration(
 	{
 		if (dynamic_cast<MagicVariableDeclaration const*>(shadowedDeclaration))
 			_errorReporter.warning(
-				_declaration.location(),
+				*_errorLocation,
 				"This declaration shadows a builtin symbol."
 			);
 		else

libsolidity/ast/AST.h

@@ -280,22 +280,31 @@ private:
 class ImportDirective: public Declaration
 {
 public:
+	struct SymbolAlias
+	{
+		ASTPointer<Identifier> symbol;
+		ASTPointer<ASTString> alias;
+		SourceLocation location;
+	};
+
+	using SymbolAliasList = std::vector<SymbolAlias>;
+
 	ImportDirective(
 		SourceLocation const& _location,
 		ASTPointer<ASTString> const& _path,
 		ASTPointer<ASTString> const& _unitAlias,
-		std::vector<std::pair<ASTPointer<Identifier>, ASTPointer<ASTString>>>&& _symbolAliases
+		SymbolAliasList _symbolAliases
 	):
 		Declaration(_location, _unitAlias),
 		m_path(_path),
-		m_symbolAliases(_symbolAliases)
+		m_symbolAliases(move(_symbolAliases))
 	{ }
 
 	void accept(ASTVisitor& _visitor) override;
 	void accept(ASTConstVisitor& _visitor) const override;
 
 	ASTString const& path() const { return *m_path; }
-	std::vector<std::pair<ASTPointer<Identifier>, ASTPointer<ASTString>>> const& symbolAliases() const
+	SymbolAliasList const& symbolAliases() const
 	{
 		return m_symbolAliases;
 	}
@@ -306,9 +315,9 @@ public:
 private:
 	ASTPointer<ASTString> m_path;
 	/// The aliases for the specific symbols to import. If non-empty import the specific symbols.
-	/// If the second component is empty, import the identifier unchanged.
+	/// If the `alias` component is empty, import the identifier unchanged.
 	/// If both m_unitAlias and m_symbolAlias are empty, import all symbols into the current scope.
-	std::vector<std::pair<ASTPointer<Identifier>, ASTPointer<ASTString>>> m_symbolAliases;
+	SymbolAliasList m_symbolAliases;
 };
 
 /**

libsolidity/ast/ASTJsonConverter.cpp

@@ -243,9 +243,9 @@ bool ASTJsonConverter::visit(ImportDirective const& _node)
 	for (auto const& symbolAlias: _node.symbolAliases())
 	{
 		Json::Value tuple(Json::objectValue);
-		solAssert(symbolAlias.first, "");
-		tuple["foreign"] = nodeId(*symbolAlias.first);
-		tuple["local"] =  symbolAlias.second ? Json::Value(*symbolAlias.second) : Json::nullValue;
+		solAssert(symbolAlias.symbol, "");
+		tuple["foreign"] = nodeId(*symbolAlias.symbol);
+		tuple["local"] =  symbolAlias.alias ? Json::Value(*symbolAlias.alias) : Json::nullValue;
 		symbolAliases.append(tuple);
 	}
 	attributes.emplace_back("symbolAliases", std::move(symbolAliases));

libsolidity/parsing/Parser.cpp

@@ -180,7 +180,7 @@ ASTPointer<ImportDirective> Parser::parseImportDirective()
 	expectToken(Token::Import);
 	ASTPointer<ASTString> path;
 	ASTPointer<ASTString> unitAlias = make_shared<string>();
-	vector<pair<ASTPointer<Identifier>, ASTPointer<ASTString>>> symbolAliases;
+	ImportDirective::SymbolAliasList symbolAliases;
 
 	if (m_scanner->currentToken() == Token::StringLiteral)
 	{
@@ -198,14 +198,16 @@ ASTPointer<ImportDirective> Parser::parseImportDirective()
 			m_scanner->next();
 			while (true)
 			{
-				ASTPointer<Identifier> id = parseIdentifier();
 				ASTPointer<ASTString> alias;
+				SourceLocation aliasLocation = SourceLocation{position(), endPosition(), source()};
+				ASTPointer<Identifier> id = parseIdentifier();
 				if (m_scanner->currentToken() == Token::As)
 				{
 					expectToken(Token::As);
+					aliasLocation = SourceLocation{position(), endPosition(), source()};
 					alias = expectIdentifierToken();
 				}
-				symbolAliases.emplace_back(move(id), move(alias));
+				symbolAliases.emplace_back(ImportDirective::SymbolAlias{move(id), move(alias), aliasLocation});
 				if (m_scanner->currentToken() != Token::Comma)
 					break;
 				m_scanner->next();

test/libsolidity/syntaxTests/imports/library_name_clash.sol

@@ -5,4 +5,4 @@ library A {}
 ==== Source: c ====
 import {A} from "./a"; import {A} from "./b";
 // ----
-// DeclarationError: (c:23-45): Identifier already declared.
+// DeclarationError: (c:31-32): Identifier already declared.

test/libsolidity/syntaxTests/imports/shadowing_builtins_with_alias.sol

@@ -3,4 +3,4 @@ contract C {}
 ==== Source: b ====
 import {C as msg} from "B.sol";
 // ----
-// Warning: (B.sol:0-13): This declaration shadows a builtin symbol.
+// Warning: (b:13-16): This declaration shadows a builtin symbol.

test/libsolidity/syntaxTests/imports/shadowing_builtins_with_multiple_imports.sol

@@ -7,5 +7,5 @@ contract C {
 // ----
 // Warning: (B.sol:0-15): This declaration shadows a builtin symbol.
 // Warning: (B.sol:16-32): This declaration shadows a builtin symbol.
-// Warning: (B.sol:0-15): This declaration shadows a builtin symbol.
-// Warning: (B.sol:16-32): This declaration shadows a builtin symbol.
+// Warning: (b:8-11): This declaration shadows a builtin symbol.
+// Warning: (b:13-18): This declaration shadows a builtin symbol.

test/libsolidity/syntaxTests/imports/shadowing_via_import.sol

@@ -5,4 +5,4 @@ library A {}
 ==== Source: c ====
 import {A} from "./a"; import {A} from "./b";
 // ----
-// DeclarationError: (c:23-45): Identifier already declared.
+// DeclarationError: (c:31-32): Identifier already declared.