Fix: Allow multiple @return tags on public state variables

Author: Marenz

Changelog.md

@@ -20,6 +20,7 @@ Bugfixes:
  * Code Generator: Fix internal error when super would have to skip an unimplemented function in the virtual resolution order.
  * Control Flow Graph: Take internal calls to functions that always revert into account for reporting unused or unassigned variables.
  * Control Flow Graph: Assume unimplemented modifiers use a placeholder.
+ * Natspec: Allow multiple ``@return`` tags on public state variable documentation.
  * SMTChecker: Fix internal error on struct constructor with fixed bytes member initialized with string literal.
  * SMTChecker: Fix internal error on external calls from the constructor.
  * SMTChecker: Fix internal error on conversion from ``bytes`` to ``fixed bytes``.

libsolidity/analysis/DocStringTagParser.cpp

@@ -49,7 +49,7 @@ bool DocStringTagParser::parseDocStrings(SourceUnit const& _sourceUnit)
 
 bool DocStringTagParser::validateDocStringsUsingTypes(SourceUnit const& _sourceUnit)
 {
-	auto errorWatcher = m_errorReporter.errorWatcher();
+	ErrorReporter::ErrorWatcher errorWatcher = m_errorReporter.errorWatcher();
 
 	SimpleASTVisitor visitReturns(
 		[](ASTNode const&) { return true; },
@@ -65,39 +65,41 @@ bool DocStringTagParser::validateDocStringsUsingTypes(SourceUnit const& _sourceU
 					if (tagName == "return")
 					{
 						returnTagsVisited++;
+						vector<string> returnParameterNames;
+
 						if (auto const* varDecl = dynamic_cast<VariableDeclaration const*>(&_node))
 						{
-							solAssert(varDecl->isPublic(), "@return is only allowed on public state-variables.");
-							if (returnTagsVisited > 1)
-								m_errorReporter.docstringParsingError(
-									5256_error,
-									documentationNode.documentation()->location(),
-									"Documentation tag \"@" + tagName + "\" is only allowed once on state-variables."
-								);
+							if (!varDecl->isPublic())
+								continue;
+
+							// FunctionType() requires the DeclarationTypeChecker to have run.
+							returnParameterNames = FunctionType(*varDecl).returnParameterNames();
 						}
 						else if (auto const* function = dynamic_cast<FunctionDefinition const*>(&_node))
+							returnParameterNames = FunctionType(*function).returnParameterNames();
+						else
+							continue;
+
+						string content = tagValue.content;
+						string firstWord = content.substr(0, content.find_first_of(" \t"));
+
+						if (returnTagsVisited > returnParameterNames.size())
+							m_errorReporter.docstringParsingError(
+								2604_error,
+								documentationNode.documentation()->location(),
+								"Documentation tag \"@" + tagName + " " + content + "\"" +
+								" exceeds the number of return parameters."
+							);
+						else
 						{
-							string content = tagValue.content;
-							string firstWord = content.substr(0, content.find_first_of(" \t"));
-
-							if (returnTagsVisited > function->returnParameters().size())
+							string const& parameter = returnParameterNames.at(returnTagsVisited - 1);
+							if (!parameter.empty() && parameter != firstWord)
 								m_errorReporter.docstringParsingError(
-									2604_error,
+									5856_error,
 									documentationNode.documentation()->location(),
-									"Documentation tag \"@" + tagName + " " + tagValue.content + "\"" +
-									" exceeds the number of return parameters."
+									"Documentation tag \"@" + tagName + " " + content + "\"" +
+									" does not contain the name of its return parameter."
 								);
-							else
-							{
-								auto parameter = function->returnParameters().at(returnTagsVisited - 1);
-								if (!parameter->name().empty() && parameter->name() != firstWord)
-									m_errorReporter.docstringParsingError(
-										5856_error,
-										documentationNode.documentation()->location(),
-										"Documentation tag \"@" + tagName + " " + tagValue.content + "\"" +
-										" does not contain the name of its return parameter."
-									);
-							}
 						}
 					}
 			}

test/libsolidity/SolidityNatspecJSON.cpp

@@ -328,6 +328,73 @@ BOOST_AUTO_TEST_CASE(public_state_variable)
 	checkNatspec(sourceCode, "test", userDoc, true);
 }
 
+BOOST_AUTO_TEST_CASE(public_state_variable_struct)
+{
+	char const* sourceCode = R"(
+		contract Bank {
+			struct Coin {
+				string observeGraphicURL;
+				string reverseGraphicURL;
+			}
+
+			/// @notice Get the n-th coin I own
+			/// @return observeGraphicURL Front pic
+			/// @return reverseGraphicURL Back pic
+			Coin[] public coinStack;
+		}
+	)";
+
+	char const* devDoc = R"R(
+	{
+		"methods" : {},
+		"stateVariables" :
+		{
+			"coinStack" :
+			{
+				"returns" :
+				{
+					"observeGraphicURL" : "Front pic",
+					"reverseGraphicURL" : "Back pic"
+				}
+			}
+		}
+	}
+	)R";
+	checkNatspec(sourceCode, "Bank", devDoc, false);
+
+	char const* userDoc = R"R(
+	{
+		"methods" :
+		{
+			"coinStack(uint256)" :
+			{
+				"notice": "Get the n-th coin I own"
+			}
+		}
+	}
+	)R";
+	checkNatspec(sourceCode, "Bank", userDoc, true);
+}
+
+BOOST_AUTO_TEST_CASE(public_state_variable_struct_repeated)
+{
+	char const* sourceCode = R"(
+		contract Bank {
+			struct Coin {
+				string obverseGraphicURL;
+				string reverseGraphicURL;
+			}
+
+			/// @notice Get the n-th coin I own
+			/// @return obverseGraphicURL Front pic
+			/// @return obverseGraphicURL Front pic
+			Coin[] public coinStack;
+		}
+	)";
+
+	expectNatspecError(sourceCode);
+}
+
 BOOST_AUTO_TEST_CASE(private_state_variable)
 {
 	char const* sourceCode = R"(

test/libsolidity/syntaxTests/natspec/docstring_state_variable_too_many_return_tags.sol

@@ -6,4 +6,4 @@ contract test {
   uint public state;
 }
 // ----
-// DocstringParsingError 5256: (18-137): Documentation tag "@return" is only allowed once on state-variables.
+// DocstringParsingError 2604: (18-137): Documentation tag "@return returns something" exceeds the number of return parameters.