From aae9d347aa7468a75d52590313b4912bbfdb4a09 Mon Sep 17 00:00:00 2001 From: Mathias Baumann Date: Wed, 26 May 2021 17:37:49 +0200 Subject: [PATCH 1/2] Split DocStringParsing in two stages one requiring type info in the next step --- libsolidity/analysis/DocStringTagParser.cpp | 100 ++++++++++++-------- libsolidity/analysis/DocStringTagParser.h | 3 + libsolidity/interface/CompilerStack.cpp | 16 +++- test/libsolidity/SolidityNatspecJSON.cpp | 4 +- 4 files changed, 77 insertions(+), 46 deletions(-) diff --git a/libsolidity/analysis/DocStringTagParser.cpp b/libsolidity/analysis/DocStringTagParser.cpp index 75a8dcc6d..3f532f59d 100644 --- a/libsolidity/analysis/DocStringTagParser.cpp +++ b/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(&_node.annotation())) + { + auto const& documentationNode = dynamic_cast(_node); + + size_t returnTagsVisited = 0; + + for (auto const& [tagName, tagValue]: annotation->docTags) + if (tagName == "return") + { + returnTagsVisited++; + if (auto const* varDecl = dynamic_cast(&_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(&_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 const validTags = set{"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(&_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(&_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." - ); - } - } - } } } + diff --git a/libsolidity/analysis/DocStringTagParser.h b/libsolidity/analysis/DocStringTagParser.h index 548de1d42..84fde6c6c 100644 --- a/libsolidity/analysis/DocStringTagParser.h +++ b/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; diff --git a/libsolidity/interface/CompilerStack.cpp b/libsolidity/interface/CompilerStack.cpp index a267f0796..00919ddba 100644 --- a/libsolidity/interface/CompilerStack.cpp +++ b/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(); // 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 diff --git a/test/libsolidity/SolidityNatspecJSON.cpp b/test/libsolidity/SolidityNatspecJSON.cpp index 4416adc02..0027a5861 100644 --- a/test/libsolidity/SolidityNatspecJSON.cpp +++ b/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" : { From 354f9d101530d38faaeaaf12382bf95e053e21e3 Mon Sep 17 00:00:00 2001 From: Mathias Baumann Date: Wed, 26 May 2021 17:37:49 +0200 Subject: [PATCH 2/2] Fix: Allow multiple @return tags on public state variables --- Changelog.md | 1 + libsolidity/analysis/DocStringTagParser.cpp | 54 ++++++++------- test/libsolidity/SolidityNatspecJSON.cpp | 67 +++++++++++++++++++ ...ng_state_variable_too_many_return_tags.sol | 2 +- 4 files changed, 97 insertions(+), 27 deletions(-) diff --git a/Changelog.md b/Changelog.md index 6003fb444..354eb6414 100644 --- a/Changelog.md +++ b/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``. diff --git a/libsolidity/analysis/DocStringTagParser.cpp b/libsolidity/analysis/DocStringTagParser.cpp index 3f532f59d..52fb4aebe 100644 --- a/libsolidity/analysis/DocStringTagParser.cpp +++ b/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 returnParameterNames; + if (auto const* varDecl = dynamic_cast(&_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(&_node)) - { - string content = tagValue.content; - string firstWord = content.substr(0, content.find_first_of(" \t")); + returnParameterNames = FunctionType(*function).returnParameterNames(); + else + continue; - if (returnTagsVisited > function->returnParameters().size()) + 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 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." - ); - } } } } diff --git a/test/libsolidity/SolidityNatspecJSON.cpp b/test/libsolidity/SolidityNatspecJSON.cpp index 0027a5861..b295b8091 100644 --- a/test/libsolidity/SolidityNatspecJSON.cpp +++ b/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"( diff --git a/test/libsolidity/syntaxTests/natspec/docstring_state_variable_too_many_return_tags.sol b/test/libsolidity/syntaxTests/natspec/docstring_state_variable_too_many_return_tags.sol index 438bb7b80..bccedd5a4 100644 --- a/test/libsolidity/syntaxTests/natspec/docstring_state_variable_too_many_return_tags.sol +++ b/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.