Merge pull request #12544 from ethereum/natspec-ice-12528

Natspec: Fix ICE when overriding a struct getter with a Natspec-documented return value and the name in the struct is different.

Author: ekpyron

Changelog.md

@@ -13,6 +13,7 @@ Bugfixes:
  * Antlr Grammar: Allow builtin names in ``yulPath`` to support ``.address`` in function pointers.
  * Control Flow Graph: Perform proper virtual lookup for modifiers for uninitialized variable and unreachable code analysis.
  * Immutables: Fix wrong error when the constructor of a base contract uses ``return`` and the parent contract contains immutable variables.
+ * Natspec: Fix ICE when overriding a struct getter with a Natspec-documented return value and the name in the struct is different.
  * TypeChecker: Fix ICE when a constant variable declaration forward references a struct.
 
 

libsolidity/analysis/DocStringAnalyser.cpp

@@ -25,6 +25,7 @@
 #include <libsolidity/analysis/DocStringAnalyser.h>
 
 #include <libsolidity/ast/AST.h>
+#include <libsolidity/ast/TypeProvider.h>
 #include <liblangutil/ErrorReporter.h>
 
 #include <boost/algorithm/string.hpp>
@@ -37,20 +38,14 @@ using namespace solidity::frontend;
 namespace
 {
 
-void copyMissingTags(set<CallableDeclaration const*> const& _baseFunctions, StructurallyDocumentedAnnotation& _target, CallableDeclaration const* _declaration = nullptr)
+void copyMissingTags(set<CallableDeclaration const*> const& _baseFunctions, StructurallyDocumentedAnnotation& _target, FunctionType const* _functionType = nullptr)
 {
 	// Only copy if there is exactly one direct base function.
 	if (_baseFunctions.size() != 1)
 		return;
 
 	CallableDeclaration const& baseFunction = **_baseFunctions.begin();
 
-	auto hasReturnParameter = [](CallableDeclaration const& declaration, size_t _n)
-	{
-		return declaration.returnParameterList() &&
-			declaration.returnParameters().size() > _n;
-	};
-
 	auto& sourceDoc = dynamic_cast<StructurallyDocumentedAnnotation const&>(baseFunction.annotation());
 
 	for (auto it = sourceDoc.docTags.begin(); it != sourceDoc.docTags.end();)
@@ -70,21 +65,22 @@ void copyMissingTags(set<CallableDeclaration const*> const& _baseFunctions, Stru
 			DocTag content = it->second;
 
 			// Update the parameter name for @return tags
-			if (_declaration && tag == "return")
+			if (_functionType && tag == "return")
 			{
 				size_t docParaNameEndPos = content.content.find_first_of(" \t");
 				string const docParameterName = content.content.substr(0, docParaNameEndPos);
 
 				if (
-					hasReturnParameter(*_declaration, n) &&
-					docParameterName != _declaration->returnParameters().at(n)->name()
+					_functionType->returnParameterNames().size() > n &&
+					docParameterName != _functionType->returnParameterNames().at(n)
 				)
 				{
 					bool baseHasNoName =
-						hasReturnParameter(baseFunction, n) &&
+						baseFunction.returnParameterList() &&
+						baseFunction.returnParameters().size() > n &&
 						baseFunction.returnParameters().at(n)->name().empty();
 
-					string paramName = _declaration->returnParameters().at(n)->name();
+					string paramName = _functionType->returnParameterNames().at(n);
 					content.content =
 						(paramName.empty() ? "" : std::move(paramName) + " ") + (
 							string::npos == docParaNameEndPos || baseHasNoName ?
@@ -127,7 +123,7 @@ bool DocStringAnalyser::analyseDocStrings(SourceUnit const& _sourceUnit)
 bool DocStringAnalyser::visit(FunctionDefinition const& _function)
 {
 	if (!_function.isConstructor())
-		handleCallable(_function, _function, _function.annotation());
+		handleCallable(_function, _function, _function.annotation(), TypeProvider::function(_function));
 	return true;
 }
 
@@ -136,10 +132,12 @@ bool DocStringAnalyser::visit(VariableDeclaration const& _variable)
 	if (!_variable.isStateVariable() && !_variable.isFileLevelVariable())
 		return false;
 
+	auto const* getterType = TypeProvider::function(_variable);
+
 	if (CallableDeclaration const* baseFunction = resolveInheritDoc(_variable.annotation().baseFunctions, _variable, _variable.annotation()))
-		copyMissingTags({baseFunction}, _variable.annotation());
+		copyMissingTags({baseFunction}, _variable.annotation(), getterType);
 	else if (_variable.annotation().docTags.empty())
-		copyMissingTags(_variable.annotation().baseFunctions, _variable.annotation());
+		copyMissingTags(_variable.annotation().baseFunctions, _variable.annotation(), getterType);
 
 	return false;
 }
@@ -168,17 +166,18 @@ bool DocStringAnalyser::visit(ErrorDefinition const& _error)
 void DocStringAnalyser::handleCallable(
 	CallableDeclaration const& _callable,
 	StructurallyDocumented const& _node,
-	StructurallyDocumentedAnnotation& _annotation
+	StructurallyDocumentedAnnotation& _annotation,
+	FunctionType const* _functionType
 )
 {
 	if (CallableDeclaration const* baseFunction = resolveInheritDoc(_callable.annotation().baseFunctions, _node, _annotation))
-		copyMissingTags({baseFunction}, _annotation, &_callable);
+		copyMissingTags({baseFunction}, _annotation, _functionType);
 	else if (
 		_annotation.docTags.empty() &&
 		_callable.annotation().baseFunctions.size() == 1 &&
 		parameterNamesEqual(_callable, **_callable.annotation().baseFunctions.begin())
 	)
-		copyMissingTags(_callable.annotation().baseFunctions, _annotation, &_callable);
+		copyMissingTags(_callable.annotation().baseFunctions, _annotation, _functionType);
 }
 
 CallableDeclaration const* DocStringAnalyser::resolveInheritDoc(

libsolidity/analysis/DocStringAnalyser.h

@@ -54,7 +54,8 @@ class DocStringAnalyser: private ASTConstVisitor
 	void handleCallable(
 		CallableDeclaration const& _callable,
 		StructurallyDocumented const& _node,
-		StructurallyDocumentedAnnotation& _annotation
+		StructurallyDocumentedAnnotation& _annotation,
+		FunctionType const* _functionType = nullptr
 	);
 
 	langutil::ErrorReporter& m_errorReporter;

test/libsolidity/SolidityNatspecJSON.cpp

@@ -2482,7 +2482,7 @@ BOOST_AUTO_TEST_CASE(custom_inheritance)
 	checkNatspec(sourceCode, "B", natspecB, false);
 }
 
-BOOST_AUTO_TEST_CASE(dev_different_amount_return_parameters)
+BOOST_AUTO_TEST_CASE(dev_struct_getter_override)
 {
 	char const *sourceCode = R"(
 		interface IThing {
@@ -2534,6 +2534,58 @@ BOOST_AUTO_TEST_CASE(dev_different_amount_return_parameters)
 	checkNatspec(sourceCode, "Thing", natspec2, false);
 }
 
+BOOST_AUTO_TEST_CASE(dev_struct_getter_override_different_return_parameter_names)
+{
+	char const *sourceCode = R"(
+		interface IThing {
+			/// @return x a number
+			/// @return y another number
+			function value() external view returns (uint128 x, uint128 y);
+		}
+
+		contract Thing is IThing {
+			struct Value {
+				uint128 a;
+				uint128 b;
+			}
+
+			Value public override value;
+		}
+	)";
+
+	char const *natspec = R"ABCDEF({
+		"methods":
+		{
+			"value()":
+			{
+			  "returns":
+			  {
+				"x": "a number",
+				"y": "another number"
+			  }
+			}
+		}
+	})ABCDEF";
+
+	char const *natspec2 = R"ABCDEF({
+		"methods": {},
+		"stateVariables":
+		{
+			"value":
+			{
+				"returns":
+				{
+					"a": "a number",
+					"b": "another number"
+				}
+			}
+		}
+	})ABCDEF";
+
+	checkNatspec(sourceCode, "IThing", natspec, false);
+	checkNatspec(sourceCode, "Thing", natspec2, false);
+}
+
 }
 
 BOOST_AUTO_TEST_SUITE_END()