cerc-io/solidity
16
0
Fork 0
mirror of https://github.com/ethereum/solidity synced 2023-10-03 13:03:40 +00:00
Code Issues Actions Packages Projects Releases Wiki Activity

Merge pull request #11455 from ethereum/issue-11381

Browse Source 
Fix: Allow multiple @return tags on public state variables
This commit is contained in:
chriseth 2021-06-07 16:51:37 +02:00 committed by GitHub
commit ad3bc71f27
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
6 changed files with 148 additions and 47 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

1
Changelog.md
View File

@ -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``.

102
libsolidity/analysis/DocStringTagParser.cpp
View File

@ -47,6 +47,68 @@ bool DocStringTagParser::parseDocStrings(SourceUnit const& _sourceUnit)
return errorWatcher.ok();
}
bool DocStringTagParser::validateDocStringsUsingTypes(SourceUnit const& _sourceUnit)
{
ErrorReporter::ErrorWatcher 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++;
vector<string> returnParameterNames;
if (auto const* varDecl = dynamic_cast<VariableDeclaration const*>(&_node))
{
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 const& parameter = returnParameterNames.at(returnTagsVisited - 1);
if (!parameter.empty() && parameter != firstWord)
m_errorReporter.docstringParsingError(
5856_error,
documentationNode.documentation()->location(),
"Documentation tag \"@" + tagName + " " + 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 +231,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 +257,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."
);
}
}
}
}
}

3
libsolidity/analysis/DocStringTagParser.h
View File

@ -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;

16
libsolidity/interface/CompilerStack.cpp
View File

@ -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

71
test/libsolidity/SolidityNatspecJSON.cpp
View File

@ -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"(
@ -1274,7 +1341,7 @@ BOOST_AUTO_TEST_CASE(dev_default_inherit_variable)
char const *natspec1 = R"ABCDEF({
"methods" : {},
"stateVariables" :
"stateVariables" :
{
"x" :
{
@ -1340,7 +1407,7 @@ BOOST_AUTO_TEST_CASE(dev_explicit_inherit_variable)
char const *natspec1 = R"ABCDEF({
"methods" : {},
"stateVariables" :
"stateVariables" :
{
"x" :
{

2
test/libsolidity/syntaxTests/natspec/docstring_state_variable_too_many_return_tags.sol
View File

@ -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.