Split DocStringParsing in two stages

one requiring type info in the next step

libsolidity/analysis/DocStringTagParser.cpp

@@ -47,6 +47,66 @@ bool DocStringTagParser::parseDocStrings(SourceUnit const& _sourceUnit)
 	return errorWatcher.ok();
 }
 
+bool DocStringTagParser::validateDocStringsUsingTypes(SourceUnit const& _sourceUnit)
+{
+	auto errorWatcher = m_errorReporter.errorWatcher();
+
+	SimpleASTVisitor visitReturns(
+		[](ASTNode const&) { return true; },
+		[&](ASTNode const& _node)
+		{
+			if (auto const* annotation = dynamic_cast<StructurallyDocumentedAnnotation const*>(&_node.annotation()))
+			{
+				auto const& documentationNode = dynamic_cast<StructurallyDocumented const&>(_node);
+
+				size_t returnTagsVisited = 0;
+
+				for (auto const& [tagName, tagValue]: annotation->docTags)
+					if (tagName == "return")
+					{
+						returnTagsVisited++;
+						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."
+								);
+						}
+						else if (auto const* function = dynamic_cast<FunctionDefinition const*>(&_node))
+						{
+							string content = tagValue.content;
+							string firstWord = content.substr(0, content.find_first_of(" \t"));
+
+							if (returnTagsVisited > function->returnParameters().size())
+								m_errorReporter.docstringParsingError(
+									2604_error,
+									documentationNode.documentation()->location(),
+									"Documentation tag \"@" + tagName + " " + tagValue.content + "\"" +
+									" exceeds the number of return parameters."
+								);
+							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."
+									);
+							}
+						}
+					}
+			}
+	});
+
+	_sourceUnit.accept(visitReturns);
+	return errorWatcher.ok();
+}
+
 bool DocStringTagParser::visit(ContractDefinition const& _contract)
 {
 	static set<string> const validTags = set<string>{"author", "title", "dev", "notice"};
@@ -169,7 +229,6 @@ void DocStringTagParser::parseDocStrings(
 
 	_annotation.docTags = DocStringParser{*_node.documentation(), m_errorReporter}.parse();
 
-	size_t returnTagsVisited = 0;
 	for (auto const& [tagName, tagValue]: _annotation.docTags)
 	{
 		string static const customPrefix("custom:");
@@ -196,43 +255,6 @@ void DocStringTagParser::parseDocStrings(
 				_node.documentation()->location(),
 				"Documentation tag @" + tagName + " not valid for " + _nodeName + "."
 			);
-		else if (tagName == "return")
-		{
-			returnTagsVisited++;
-			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,
-						_node.documentation()->location(),
-						"Documentation tag \"@" + tagName + "\" is only allowed once on state-variables."
-					);
-			}
-			else if (auto const* function = dynamic_cast<FunctionDefinition const*>(&_node))
-			{
-				string content = tagValue.content;
-				string firstWord = content.substr(0, content.find_first_of(" \t"));
-
-				if (returnTagsVisited > function->returnParameters().size())
-					m_errorReporter.docstringParsingError(
-						2604_error,
-						_node.documentation()->location(),
-						"Documentation tag \"@" + tagName + " " + tagValue.content + "\"" +
-						" exceeds the number of return parameters."
-					);
-				else
-				{
-					auto parameter = function->returnParameters().at(returnTagsVisited - 1);
-					if (!parameter->name().empty() && parameter->name() != firstWord)
-						m_errorReporter.docstringParsingError(
-							5856_error,
-							_node.documentation()->location(),
-							"Documentation tag \"@" + tagName + " " + tagValue.content + "\"" +
-							" does not contain the name of its return parameter."
-						);
-				}
-			}
-		}
 	}
 }
+

libsolidity/analysis/DocStringTagParser.h

@@ -37,6 +37,9 @@ class DocStringTagParser: private ASTConstVisitor
 public:
 	explicit DocStringTagParser(langutil::ErrorReporter& _errorReporter): m_errorReporter(_errorReporter) {}
 	bool parseDocStrings(SourceUnit const& _sourceUnit);
+	/// Validate the parsed doc strings, requires parseDocStrings() and the
+	/// DeclarationTypeChecker to have run.
+	bool validateDocStringsUsingTypes(SourceUnit const& _sourceUnit);
 
 private:
 	bool visit(ContractDefinition const& _contract) override;

libsolidity/interface/CompilerStack.cpp

@@ -416,11 +416,6 @@ bool CompilerStack::analyze()
 			if (source->ast && !syntaxChecker.checkSyntax(*source->ast))
 				noErrors = false;
 
-		DocStringTagParser docStringTagParser(m_errorReporter);
-		for (Source const* source: m_sourceOrder)
-			if (source->ast && !docStringTagParser.parseDocStrings(*source->ast))
-				noErrors = false;
-
 		m_globalContext = make_shared<GlobalContext>();
 		// We need to keep the same resolver during the whole process.
 		NameAndTypeResolver resolver(*m_globalContext, m_evmVersion, m_errorReporter);
@@ -437,6 +432,12 @@ bool CompilerStack::analyze()
 
 		resolver.warnHomonymDeclarations();
 
+		DocStringTagParser docStringTagParser(m_errorReporter);
+		for (Source const* source: m_sourceOrder)
+			if (source->ast && !docStringTagParser.parseDocStrings(*source->ast))
+				noErrors = false;
+
+		// Requires DocStringTagParser
 		for (Source const* source: m_sourceOrder)
 			if (source->ast && !resolver.resolveNamesAndTypes(*source->ast))
 				return false;
@@ -446,6 +447,11 @@ bool CompilerStack::analyze()
 			if (source->ast && !declarationTypeChecker.check(*source->ast))
 				return false;
 
+		// Requires DeclarationTypeChecker to have run
+		for (Source const* source: m_sourceOrder)
+			if (source->ast && !docStringTagParser.validateDocStringsUsingTypes(*source->ast))
+				noErrors = false;
+
 		// Next, we check inheritance, overrides, function collisions and other things at
 		// contract or function level.
 		// This also calculates whether a contract is abstract, which is needed by the

test/libsolidity/SolidityNatspecJSON.cpp

@@ -1274,7 +1274,7 @@ BOOST_AUTO_TEST_CASE(dev_default_inherit_variable)
 
 	char const *natspec1 = R"ABCDEF({
 		"methods" : {},
-			"stateVariables" :
+		"stateVariables" :
 		{
 			"x" :
 			{
@@ -1340,7 +1340,7 @@ BOOST_AUTO_TEST_CASE(dev_explicit_inherit_variable)
 
 	char const *natspec1 = R"ABCDEF({
 		"methods" : {},
-			"stateVariables" :
+		"stateVariables" :
 		{
 			"x" :
 			{