Applying code review.

Changelog.md

@@ -7,6 +7,7 @@ Compiler Features:
 * Commandline Interface: Add `--no-cbor-metadata` that skips CBOR metadata from getting appended at the end of the bytecode.
 * Standard JSON: Add a boolean field `settings.metadata.appendCBOR` that skips CBOR metadata from getting appended at the end of the bytecode.
 * Yul Optimizer: Allow replacing the previously hard-coded cleanup sequence by specifying custom steps after a colon delimiter (``:``) in the sequence string.
+* Language Server: Implements finding all references as well as semantic highlighting of symbols.
 
 
 Bugfixes:
@@ -22,7 +23,6 @@ Compiler Features:
  * Code Generator: More efficient overflow checks for multiplication.
  * Language Server: Analyze all files in a project by default (can be customized by setting ``'file-load-strategy'`` to ``'directly-opened-and-on-import'`` in LSP settings object).
  * Yul Optimizer: Simplify the starting offset of zero-length operations to zero.
- * Language Server: Implements finding all references as well as semantic highlighting of symbols.
 
 
 Bugfixes:

libsolidity/ast/AST.h

@@ -595,6 +595,7 @@ public:
 	):
 		ASTNode(_id, _location), m_path(std::move(_path)), m_pathLocations(std::move(_pathLocations))
 	{
+		solAssert(!m_pathLocations.empty());
 		solAssert(m_pathLocations.size() == m_path.size());
 	}
 

libsolidity/lsp/LanguageServer.cpp

@@ -548,18 +548,23 @@ void LanguageServer::handleTextDocumentDidClose(Json::Value const& _args)
 }
 
 
-ASTNode const* LanguageServer::astNodeAtSourceLocation(std::string const& _sourceUnitName, LineColumn const& _filePos)
+tuple<ASTNode const*, int> LanguageServer::astNodeAndOffsetAtSourceLocation(std::string const& _sourceUnitName, langutil::LineColumn const& _filePos)
 {
 	if (m_compilerStack.state() < CompilerStack::AnalysisPerformed)
-		return nullptr;
+		return {nullptr, -1};
 
 	if (!m_fileRepository.sourceUnits().count(_sourceUnitName))
-		return nullptr;
+		return {nullptr, -1};
 
 	if (optional<int> sourcePos =
 		m_compilerStack.charStream(_sourceUnitName).translateLineColumnToPosition(_filePos))
-		return locateInnermostASTNode(*sourcePos, m_compilerStack.ast(_sourceUnitName));
+		return {locateInnermostASTNode(*sourcePos, m_compilerStack.ast(_sourceUnitName)), *sourcePos};
 	else
-		return nullptr;
+		return {nullptr, -1};
+}
+
+ASTNode const* LanguageServer::astNodeAtSourceLocation(std::string const& _sourceUnitName, LineColumn const& _filePos)
+{
+	return get<0>(astNodeAndOffsetAtSourceLocation(_sourceUnitName, _filePos));
 }
 

libsolidity/lsp/LanguageServer.h

@@ -79,6 +79,7 @@ public:
 	Transport& client() noexcept { return m_client; }
 	frontend::SourceUnit const& ast(std::string const& _sourceUnitName) const { return m_compilerStack.ast(_sourceUnitName); }
 	frontend::ASTNode const* astNodeAtSourceLocation(std::string const& _sourceUnitName, langutil::LineColumn const& _filePos);
+	std::tuple<frontend::ASTNode const*, int> astNodeAndOffsetAtSourceLocation(std::string const& _sourceUnitName, langutil::LineColumn const& _filePos);
 	frontend::CompilerStack const& compilerStack() const noexcept { return m_compilerStack; }
 
 private:

libsolidity/lsp/ReferenceCollector.cpp

@@ -23,6 +23,8 @@
 
 #include <libsolutil/Common.h>
 
+#include <typeinfo>
+
 using namespace solidity::frontend;
 using namespace solidity::langutil;
 using namespace std::string_literals;
@@ -31,159 +33,133 @@ using namespace std;
 namespace solidity::lsp
 {
 
-namespace
-{
-
-vector<Declaration const*> allAnnotatedDeclarations(Identifier const* _identifier)
-{
-	vector<Declaration const*> output;
-	output.push_back(_identifier->annotation().referencedDeclaration);
-	output += _identifier->annotation().candidateDeclarations;
-	return output;
-}
-
-}
-
 ReferenceCollector::ReferenceCollector(
 	Declaration const& _declaration,
-	std::string const& _sourceIdentifierName
+	string const& _sourceIdentifierName
 ):
 	m_declaration{_declaration},
 	m_sourceIdentifierName{_sourceIdentifierName.empty() ? _declaration.name() : _sourceIdentifierName}
 {
 }
 
-std::vector<Reference> ReferenceCollector::collect(
+vector<Reference> ReferenceCollector::collect(
 	Declaration const* _declaration,
-	ASTNode const& _ast,
+	ASTNode const& _astSearchRoot,
-	std::string const& _sourceIdentifierName
+	string const& _sourceIdentifierName
 )
 {
 	if (!_declaration)
 		return {};
 
 	ReferenceCollector collector(*_declaration, _sourceIdentifierName);
-	_ast.accept(collector);
+	_astSearchRoot.accept(collector);
-	return std::move(collector.m_resultingReferences);
+	return std::move(collector.m_collectedReferences);
 }
 
-std::vector<Reference> ReferenceCollector::collect(
+vector<Reference> ReferenceCollector::collect(
 	ASTNode const* _sourceNode,
+	int _sourceOffset,
 	SourceUnit const& _sourceUnit
 )
 {
 	if (!_sourceNode)
 		return {};
 
-	auto output = vector<Reference>{};
+	auto references = vector<Reference>{};
 
 	if (auto const* identifier = dynamic_cast<Identifier const*>(_sourceNode))
-	{
+		references += collect(identifier->annotation().referencedDeclaration, _sourceUnit, identifier->name());
-		for (auto const* declaration: allAnnotatedDeclarations(identifier))
-			output += collect(declaration, _sourceUnit, identifier->name());
-	}
 	else if (auto const* identifierPath = dynamic_cast<IdentifierPath const*>(_sourceNode))
 	{
 		solAssert(identifierPath->path().size() >= 1, "");
-		output += collect(identifierPath->annotation().referencedDeclaration, _sourceUnit, identifierPath->path().back());
+		for (size_t i = 0; i < identifierPath->pathLocations().size(); ++i)
+		{
+			SourceLocation const location = identifierPath->pathLocations()[i];
+			if (location.containsOffset(_sourceOffset))
+			{
+				Declaration const* declaration = identifierPath->annotation().pathDeclarations.at(i);
+				ASTString const& name = identifierPath->path().at(i);
+				references += collect(declaration, _sourceUnit, name);
+				break;
+			}
+		}
 	}
 	else if (auto const* memberAccess = dynamic_cast<MemberAccess const*>(_sourceNode))
-		output += collect(memberAccess->annotation().referencedDeclaration, _sourceUnit, memberAccess->memberName());
+		references += collect(memberAccess->annotation().referencedDeclaration, _sourceUnit, memberAccess->memberName());
 	else if (auto const* declaration = dynamic_cast<Declaration const*>(_sourceNode))
-		output += collect(declaration, _sourceUnit, declaration->name());
+		references += collect(declaration, _sourceUnit, declaration->name());
 	else
-		lspAssert(false, ErrorCode::InternalError, "Unhandled AST node "s + typeid(*_sourceNode).name());
+		lspRequire(false, ErrorCode::InternalError, "Unhandled AST node "s + typeid(*_sourceNode).name());
 
-	return output;
+	return references;
 }
 
 bool ReferenceCollector::visit(ImportDirective const& _import)
 {
-	if (_import.name() == m_sourceIdentifierName)
+	for (ImportDirective::SymbolAlias const& symbolAlias: _import.symbolAliases())
-		return true;
-	return false;
-}
-
-void ReferenceCollector::endVisit(ImportDirective const& _import)
-{
-	for (auto const& symbolAlias: _import.symbolAliases())
 	{
 		if (
-			m_sourceIdentifierName == *symbolAlias.alias &&
+			symbolAlias.alias && *symbolAlias.alias == m_sourceIdentifierName &&
-			symbolAlias.symbol &&
+			symbolAlias.symbol && symbolAlias.symbol->annotation().referencedDeclaration == &m_declaration
-			symbolAlias.symbol->annotation().referencedDeclaration == &m_declaration
 		)
-		{
+			m_collectedReferences.emplace_back(symbolAlias.location, DocumentHighlightKind::Read);
-			m_resultingReferences.emplace_back(symbolAlias.location, DocumentHighlightKind::Read);
-			break;
-		}
-		else if (m_sourceIdentifierName == *symbolAlias.alias)
-		{
-			m_resultingReferences.emplace_back(symbolAlias.location, DocumentHighlightKind::Text);
-			break;
-		}
 	}
-}
+	return false;
-
-bool ReferenceCollector::tryAddReference(Declaration const* _declaration, SourceLocation const& _location)
-{
-	if (&m_declaration != _declaration)
-		return false;
-
-	m_resultingReferences.emplace_back(_location, m_kind);
-	return true;
 }
 
 void ReferenceCollector::endVisit(Identifier const& _identifier)
 {
-	if (auto const* declaration = _identifier.annotation().referencedDeclaration)
+	if (Declaration const* declaration = _identifier.annotation().referencedDeclaration; declaration == &m_declaration)
-		tryAddReference(declaration, _identifier.location());
+		m_collectedReferences.emplace_back(_identifier.location(), m_kind);
-
-	for (auto const* declaration: _identifier.annotation().candidateDeclarations + _identifier.annotation().overloadedDeclarations)
-		tryAddReference(declaration, _identifier.location());
 }
 
-
 void ReferenceCollector::endVisit(IdentifierPath const& _identifierPath)
 {
-	tryAddReference(_identifierPath.annotation().referencedDeclaration, _identifierPath.location());
+	for (size_t i = 0; i < _identifierPath.path().size(); ++i)
+	{
+		ASTString const& name = _identifierPath.path()[i];
+		Declaration const* declaration = i < _identifierPath.annotation().pathDeclarations.size() ?
+			_identifierPath.annotation().pathDeclarations.at(i) :
+			nullptr;
+
+		if (declaration == &m_declaration && name == m_sourceIdentifierName)
+			m_collectedReferences.emplace_back(_identifierPath.pathLocations().at(i), m_kind);
+	}
 }
 
 void ReferenceCollector::endVisit(MemberAccess const& _memberAccess)
 {
 	if (_memberAccess.annotation().referencedDeclaration == &m_declaration)
-		m_resultingReferences.emplace_back(_memberAccess.location(), m_kind);
+		m_collectedReferences.emplace_back(_memberAccess.location(), m_kind);
 }
 
-bool ReferenceCollector::visit(Assignment const& _node)
+bool ReferenceCollector::visit(Assignment const& _assignment)
 {
 	auto const restoreKind = ScopeGuard{[this, savedKind=m_kind]() { m_kind = savedKind; }};
 
 	m_kind = DocumentHighlightKind::Write;
-	_node.leftHandSide().accept(*this);
+	_assignment.leftHandSide().accept(*this);
 
 	m_kind = DocumentHighlightKind::Read;
-	_node.rightHandSide().accept(*this);
+	_assignment.rightHandSide().accept(*this);
 
 	return false;
 }
 
-bool ReferenceCollector::visit(VariableDeclaration const& _node)
+bool ReferenceCollector::visit(VariableDeclaration const& _variableDeclaration)
 {
-	if (&_node == &m_declaration)
+	if (&_variableDeclaration == &m_declaration && _variableDeclaration.name() == m_sourceIdentifierName)
-		m_resultingReferences.emplace_back(_node.nameLocation(), DocumentHighlightKind::Write);
+		m_collectedReferences.emplace_back(_variableDeclaration.nameLocation(), DocumentHighlightKind::Write);
-
 	return true;
 }
 
-bool ReferenceCollector::visitNode(ASTNode const& _node)
+bool ReferenceCollector::visitNode(ASTNode const& _genericNode)
 {
-	if (&_node == &m_declaration)
+	if (&_genericNode == &m_declaration)
 	{
-		if (auto const* declaration = dynamic_cast<Declaration const*>(&_node))
+		Declaration const* declaration = dynamic_cast<Declaration const*>(&_genericNode);
-			m_resultingReferences.emplace_back(declaration->nameLocation(), m_kind);
+		if (declaration->name() == m_sourceIdentifierName)
-		else
+			m_collectedReferences.emplace_back(declaration->nameLocation(), m_kind);
-			m_resultingReferences.emplace_back(_node.location(), DocumentHighlightKind::Read);
 	}
 
 	return true;

libsolidity/lsp/ReferenceCollector.h

@@ -23,6 +23,9 @@
 namespace solidity::lsp
 {
 
+/**
+ * See https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#documentHighlightKind
+ */
 enum class DocumentHighlightKind
 {
 	Unspecified = 0,    //!< could be for example a highlight found in a comment
@@ -31,24 +34,41 @@ enum class DocumentHighlightKind
 	Write = 3,          //!< write access to a variable
 };
 
-// Represents a symbol / AST node that is to be highlighted, with some context associated.
+/**
+ * Represents a symbol / AST node that is to be highlighted, with some context associated.
+ */
 using Reference = std::tuple<langutil::SourceLocation, DocumentHighlightKind>;
 
+/**
+ * ReferenceCollector can be used to collect all source locations and their usage
+ * of a given symbol in the AST tree.
+ */
 class ReferenceCollector: public frontend::ASTConstVisitor
 {
-public:
+private:
-	ReferenceCollector(frontend::Declaration const& _declaration, std::string const& _sourceIdentifierName);
+	ReferenceCollector(frontend::Declaration const& _declaration, std::string const& _sourceIdentifierName = "");
 
+	/// Collects all occurrences of a given identifier matching the same declaration.
+	/// Just passing the AST node and infer the name from there is not enough as they might have been aliased.
+	///
+	/// @param _declaration the declaration to match against for the symbols to collect
+	/// @param _sourceOffset byte offset into the respective file reflecting the exact location of the cursor
+	/// @param _astSearchRoot the AST base root node to start the recursive traversal to collect the references
+	/// @param _sourceIdentifierName the symbolic name that must match, which is explicitly given as it might have been altered due to aliases.
+	///
+	/// @returns a vector of Reference objects that contain the source location of each match as well as their
+	/// semantic use (e.g. read access or write access).
 	static std::vector<Reference> collect(
 		frontend::Declaration const* _declaration,
-		frontend::ASTNode const& _ast,
+		frontend::ASTNode const& _astSearchRoot,
 		std::string const& _sourceIdentifierName
 	);
 
-	static std::vector<Reference> collect(frontend::ASTNode const* _sourceNode, frontend::SourceUnit const& _sourceUnit);
+public:
+	/// Collects all references (symbols with the same matching declaration as @p _sourceNode) in the given source unit.
+	static std::vector<Reference> collect(frontend::ASTNode const* _sourceNode, int _sourceOffset, frontend::SourceUnit const& _sourceUnit);
 
 	bool visit(frontend::ImportDirective const& _import) override;
-	void endVisit(frontend::ImportDirective const& _import) override;
 	void endVisit(frontend::Identifier const& _identifier) override;
 	void endVisit(frontend::IdentifierPath  const& _identifierPath) override;
 	void endVisit(frontend::MemberAccess const& _memberAccess) override;
@@ -56,14 +76,11 @@ public:
 	bool visit(frontend::VariableDeclaration const& _node) override;
 	bool visitNode(frontend::ASTNode const& _node) override;
 
-private:
-	bool tryAddReference(frontend::Declaration const* _declaration, solidity::langutil::SourceLocation const& _location);
-
 private:
 	frontend::Declaration const& m_declaration;
 	std::string const& m_sourceIdentifierName;
-	std::vector<Reference> m_resultingReferences;
+	std::vector<Reference> m_collectedReferences;
 	DocumentHighlightKind m_kind = DocumentHighlightKind::Read;
 };
 
-} // end namespace
+}

libsolidity/lsp/References.cpp

@@ -33,11 +33,11 @@ void References::operator()(MessageID _id, Json::Value const& _args)
 {
 	auto const [sourceUnitName, lineColumn] = extractSourceUnitNameAndLineColumn(_args);
 
-	ASTNode const* sourceNode = m_server.astNodeAtSourceLocation(sourceUnitName, lineColumn);
+	auto const [sourceNode, sourceOffset] = m_server.astNodeAndOffsetAtSourceLocation(sourceUnitName, lineColumn);
 	SourceUnit const& sourceUnit = m_server.ast(sourceUnitName);
 
 	Json::Value jsonReply = Json::arrayValue;
-	for (auto const& reference: ReferenceCollector::collect(sourceNode, sourceUnit))
+	for (auto const& reference: ReferenceCollector::collect(sourceNode, sourceOffset, sourceUnit))
 		jsonReply.append(toJson(get<SourceLocation>(reference)));
 
 	client().reply(_id, jsonReply);

libsolidity/lsp/References.h

@@ -20,6 +20,9 @@
 namespace solidity::lsp
 {
 
+/**
+ * Implements JSON RPC for `textDocument/references`.
+ */
 class References: public HandlerBase
 {
 public:

libsolidity/lsp/SemanticHighlight.cpp

@@ -26,10 +26,10 @@ using namespace solidity::frontend;
 void SemanticHighlight::operator()(MessageID _id, Json::Value const& _args)
 {
 	auto const [sourceUnitName, lineColumn] = extractSourceUnitNameAndLineColumn(_args);
-	ASTNode const* sourceNode = m_server.astNodeAtSourceLocation(sourceUnitName, lineColumn);
+	auto const [sourceNode, sourceOffset] = m_server.astNodeAndOffsetAtSourceLocation(sourceUnitName, lineColumn);
 
 	Json::Value jsonReply = Json::arrayValue;
-	for (auto const& [location, kind]: semanticHighlight(sourceNode, sourceUnitName))
+	for (auto const& [location, kind]: semanticHighlight(sourceNode, sourceOffset, sourceUnitName))
 	{
 		Json::Value item = Json::objectValue;
 		item["range"] = toRange(location);
@@ -40,13 +40,13 @@ void SemanticHighlight::operator()(MessageID _id, Json::Value const& _args)
 	client().reply(_id, jsonReply);
 }
 
-vector<Reference> SemanticHighlight::semanticHighlight(ASTNode const* _sourceNode, string const& _sourceUnitName)
+vector<Reference> SemanticHighlight::semanticHighlight(ASTNode const* _sourceNode, int _sourceOffset, string const& _sourceUnitName)
 {
 	if (!_sourceNode)
 		return {};
 
 	SourceUnit const& sourceUnit = m_server.ast(_sourceUnitName);
 
-	return ReferenceCollector::collect(_sourceNode, sourceUnit);
+	return ReferenceCollector::collect(_sourceNode, _sourceOffset, sourceUnit);
 }
 

libsolidity/lsp/SemanticHighlight.h

@@ -23,15 +23,18 @@
 namespace solidity::lsp
 {
 
+/**
+ * Implements JSON RPC for `textDocument/documentHighlight`.
+ */
 class SemanticHighlight: public HandlerBase
 {
 public:
-	SemanticHighlight(LanguageServer& _server): HandlerBase(_server) {}
+	explicit SemanticHighlight(LanguageServer& _server): HandlerBase(_server) {}
 
 	void operator()(MessageID _id, Json::Value const& _args);
 
 private:
-	std::vector<Reference> semanticHighlight(frontend::ASTNode const* _sourceNode, std::string const& _sourceUnitName);
+	std::vector<Reference> semanticHighlight(frontend::ASTNode const* _sourceNode, int _sourceOffset, std::string const& _sourceUnitName);
 };
 
 }

test/libsolidity/lsp/references/import_aliases_2.sol

@@ -0,0 +1,30 @@
+// SPDX-License-Identifier: UNLICENSED
+pragma solidity >=0.8.0;
+
+import {RGBColor as A, RGBColor as B, RGBColor as C} from "../goto/lib.sol";
+//                  ^ @AliasA
+
+contract MyContract
+{
+    function f(A memory a, B memory b) public pure returns (C memory c)
+//             ^ @FuncParamTypeA
+    {
+        c.red = b.red + a.green + b.blue;
+    }
+}
+// ----
+// lib: @diagnostics 2072
+// -> textDocument/documentHighlight {
+//     "position": @FuncParamTypeA
+// }
+// <- [
+//     { "kind": 2, "range": @AliasA },
+//     { "kind": 2, "range": @FuncParamTypeA }
+// ]
+// -> textDocument/documentHighlight {
+//     "position": @FuncParamTypeA
+// }
+// <- [
+//     { "kind": 2, "range": @AliasA },
+//     { "kind": 2, "range": @FuncParamTypeA }
+// ]

test/libsolidity/lsp/references/read_write_access_kinds.sol

@@ -30,54 +30,24 @@ contract C
 //     "position": @OutputWrite
 // }
 // <- [
-//     {
+//     { "kind": 3, "range": @OutputDef },
-//         "kind": 3,
+//     { "kind": 3, "range": @OutputWrite1 },
-//         "range": @OutputDef
+//     { "kind": 3, "range": @OutputWrite2 },
-//     },
+//     { "kind": 2, "range": @OutputRead1 },
-//     {
+//     { "kind": 3, "range": @OutputWrite3 },
-//         "kind": 3,
+//     { "kind": 2, "range": @OutputRead2 }
-//         "range": @OutputWrite1
-//     },
-//     {
-//         "kind": 3,
-//         "range": @OutputWrite2
-//     },
-//     {
-//         "kind": 2,
-//         "range": @OutputRead1
-//     },
-//     {
-//         "kind": 3,
-//         "range": @OutputWrite3
-//     },
-//     {
-//         "kind": 2,
-//         "range": @OutputRead2
-//     }
 // ]
 // -> textDocument/documentHighlight {
 //     "position": @GreenWrite
 // }
 // <- [
-//     {
+//     { "kind": 3, "range": @OutputGreenWrite1 },
-//         "kind": 3,
+//     { "kind": 2, "range": @OutputGreenRead1 }
-//         "range": @OutputGreenWrite1
-//     },
-//     {
-//         "kind": 2,
-//         "range": @OutputGreenRead1
-//     }
 // ]
 // -> textDocument/documentHighlight {
 //     "position": @GreenUse
 // }
 // <- [
-//     {
+//     { "kind": 3, "range": @OutputGreenWrite1 },
-//         "kind": 3,
+//     { "kind": 2, "range": @OutputGreenRead1 }
-//         "range": @OutputGreenWrite1
-//     },
-//     {
-//         "kind": 2,
-//         "range": @OutputGreenRead1
-//     }
 // ]