Natspec: Don't copy from base function if return parameters differ

Changelog.md

@@ -24,6 +24,7 @@ Compiler Features:
 
 
 Bugfixes:
+ * Natspec: Fix internal error related to the `@returns` documentation for a public state variable overriding a function.
  * Antlr Grammar: Fix parsing of import paths involving properly distinguishing between empty and non-empty string literals in general.
  * AST Output: Fix ``kind`` field of ``ModifierInvocation`` for base constructor calls.
  * SMTChecker: Fix false positive and false negative on ``push`` as LHS of a compound assignment.

libsolidity/interface/Natspec.cpp

@@ -134,7 +134,10 @@ Json::Value Natspec::devDocumentation(ContractDefinition const& _contractDef)
 		{
 			Json::Value method(devDocumentation(fun->annotation().docTags));
 			// add the function, only if we have any documentation to add
-			Json::Value jsonReturn = extractReturnParameterDocs(fun->annotation().docTags, *fun);
+			Json::Value jsonReturn = extractReturnParameterDocs(
+				fun->annotation().docTags,
+				fun->functionType(false)->returnParameterNames()
+			);
 
 			if (!jsonReturn.empty())
 				method["returns"] = move(jsonReturn);
@@ -149,9 +152,20 @@ Json::Value Natspec::devDocumentation(ContractDefinition const& _contractDef)
 		if (auto devDoc = devDocumentation(varDecl->annotation().docTags); !devDoc.empty())
 			doc["stateVariables"][varDecl->name()] = devDoc;
 
-		solAssert(varDecl->annotation().docTags.count("return") <= 1, "");
+		auto const assignIfNotEmpty = [&](string const& _name, Json::Value const& _content)
+		{
+			if (!_content.empty())
+				doc["stateVariables"][varDecl->name()][_name] = _content;
+		};
+
 		if (varDecl->annotation().docTags.count("return") == 1)
-			doc["stateVariables"][varDecl->name()]["return"] = extractDoc(varDecl->annotation().docTags, "return");
+			assignIfNotEmpty("return", extractDoc(varDecl->annotation().docTags, "return"));
+
+		if (FunctionTypePointer functionType = varDecl->functionType(false))
+			assignIfNotEmpty("returns", extractReturnParameterDocs(
+				varDecl->annotation().docTags,
+				functionType->returnParameterNames()
+			));
 	}
 
 	for (auto const& event: _contractDef.events())
@@ -165,17 +179,17 @@ Json::Value Natspec::devDocumentation(ContractDefinition const& _contractDef)
 	return doc;
 }
 
-Json::Value Natspec::extractReturnParameterDocs(std::multimap<std::string, DocTag> const& _tags, FunctionDefinition const& _functionDef)
+Json::Value Natspec::extractReturnParameterDocs(std::multimap<std::string, DocTag> const& _tags, vector<string> const& _returnParameterNames)
 {
 	Json::Value jsonReturn{Json::objectValue};
 	auto returnDocs = _tags.equal_range("return");
 
-	if (!_functionDef.returnParameters().empty())
+	if (!_returnParameterNames.empty())
 	{
 		size_t n = 0;
 		for (auto i = returnDocs.first; i != returnDocs.second; i++)
 		{
-			string paramName = _functionDef.returnParameters().at(n)->name();
+			string paramName = _returnParameterNames.at(n);
 			string content = i->second.content;
 
 			if (paramName.empty())

libsolidity/interface/Natspec.h

@@ -68,10 +68,10 @@ private:
 
 	/// Helper-function that will create a json object for the "returns" field for a given function definition.
 	/// @param _tags docTags that are used.
-	/// @param _functionDef functionDefinition that is used to determine which return parameters are named.
+	/// @param _returnParameterNames names of the return parameters
 	/// @return      A JSON representation
 	///              of a method's return notice documentation
-	static Json::Value extractReturnParameterDocs(std::multimap<std::string, DocTag> const& _tags, FunctionDefinition const& _functionDef);
+	static Json::Value extractReturnParameterDocs(std::multimap<std::string, DocTag> const& _tags, std::vector<std::string> const& _returnParameterNames);
 };
 
 }

test/libsolidity/SolidityNatspecJSON.cpp

@@ -303,7 +303,11 @@ BOOST_AUTO_TEST_CASE(public_state_variable)
 			"state" :
 			{
 				"details" : "example of dev",
-				"return" : "returns state"
+				"return" : "returns state",
+				"returns" :
+				{
+					"_0" : "returns state"
+				}
 			}
 		}
 	}
@@ -2411,6 +2415,58 @@ BOOST_AUTO_TEST_CASE(custom_inheritance)
 	checkNatspec(sourceCode, "B", natspecB, false);
 }
 
+BOOST_AUTO_TEST_CASE(dev_different_amount_return_parameters)
+{
+	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 x;
+				uint128 y;
+			}
+
+			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":
+				{
+					"x": "a number",
+					"y": "another number"
+				}
+			}
+		}
+	})ABCDEF";
+
+	checkNatspec(sourceCode, "IThing", natspec, false);
+	checkNatspec(sourceCode, "Thing", natspec2, false);
+}
+
 }
 
 BOOST_AUTO_TEST_SUITE_END()

test/libsolidity/syntaxTests/natspec/invalid/return_param_amount_differs.sol

@@ -0,0 +1,14 @@
+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 x;
+        uint128 y;
+    }
+
+    Value public override value;
+}

test/libsolidity/syntaxTests/natspec/invalid/return_param_amount_differs2.sol

@@ -0,0 +1,15 @@
+interface IThing {
+    /// @param v value to search for
+    /// @return x a number
+    /// @return y another number
+    function value(uint256 v) external view returns (uint128 x, uint128 y);
+}
+
+contract Thing is IThing {
+    struct Value {
+        uint128 x;
+        uint128 y;
+    }
+
+    mapping(uint256=>Value) public override value;
+}