-
Notifications
You must be signed in to change notification settings - Fork 5.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Natspec: Implement inheritance and @inheritdoc #9218
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -53,6 +53,17 @@ void copyMissingTags(StructurallyDocumentedAnnotation& _target, set<CallableDecl | |
_target.docTags.emplace(tag, content); | ||
} | ||
|
||
CallableDeclaration const* findBaseCallable(set<CallableDeclaration const*> const& _baseFunctions, int64_t _contractId) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Because that's the type that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Indeed. Now I'm confused why |
||
{ | ||
for (CallableDeclaration const* baseFuncCandidate: _baseFunctions) | ||
if (baseFuncCandidate->annotation().contract->id() == _contractId) | ||
return baseFuncCandidate; | ||
else if (auto callable = findBaseCallable(baseFuncCandidate->annotation().baseFunctions, _contractId)) | ||
return callable; | ||
|
||
return nullptr; | ||
} | ||
|
||
bool parameterNamesEqual(CallableDeclaration const& _a, CallableDeclaration const& _b) | ||
{ | ||
return boost::range::equal(_a.parameters(), _b.parameters(), [](auto const& pa, auto const& pb) { return pa->name() == pb->name(); }); | ||
|
@@ -67,50 +78,23 @@ bool DocStringAnalyser::analyseDocStrings(SourceUnit const& _sourceUnit) | |
return errorWatcher.ok(); | ||
} | ||
|
||
bool DocStringAnalyser::visit(ContractDefinition const& _contract) | ||
{ | ||
static set<string> const validTags = set<string>{"author", "title", "dev", "notice"}; | ||
parseDocStrings(_contract, _contract.annotation(), validTags, "contracts"); | ||
|
||
return true; | ||
} | ||
|
||
bool DocStringAnalyser::visit(FunctionDefinition const& _function) | ||
{ | ||
if (_function.isConstructor()) | ||
handleConstructor(_function, _function, _function.annotation()); | ||
else | ||
if (!_function.isConstructor()) | ||
handleCallable(_function, _function, _function.annotation()); | ||
return true; | ||
} | ||
|
||
bool DocStringAnalyser::visit(VariableDeclaration const& _variable) | ||
{ | ||
if (_variable.isStateVariable()) | ||
{ | ||
static set<string> const validPublicTags = set<string>{"dev", "notice", "return", "title", "author"}; | ||
if (_variable.isPublic()) | ||
parseDocStrings(_variable, _variable.annotation(), validPublicTags, "public state variables"); | ||
else | ||
{ | ||
parseDocStrings(_variable, _variable.annotation(), validPublicTags, "non-public state variables"); | ||
if (_variable.annotation().docTags.count("notice") > 0) | ||
m_errorReporter.warning( | ||
7816_error, _variable.documentation()->location(), | ||
"Documentation tag on non-public state variables will be disallowed in 0.7.0. " | ||
"You will need to use the @dev tag explicitly." | ||
); | ||
} | ||
if (_variable.annotation().docTags.count("title") > 0 || _variable.annotation().docTags.count("author") > 0) | ||
m_errorReporter.warning( | ||
8532_error, _variable.documentation()->location(), | ||
"Documentation tag @title and @author is only allowed on contract definitions. " | ||
"It will be disallowed in 0.7.0." | ||
); | ||
if (!_variable.isStateVariable()) | ||
return false; | ||
|
||
if (CallableDeclaration const* baseFunction = resolveInheritDoc(_variable.annotation().baseFunctions, _variable, _variable.annotation())) | ||
copyMissingTags(_variable.annotation(), {baseFunction}); | ||
else if (_variable.annotation().docTags.empty()) | ||
copyMissingTags(_variable.annotation(), _variable.annotation().baseFunctions); | ||
|
||
if (_variable.annotation().docTags.empty()) | ||
copyMissingTags(_variable.annotation(), _variable.annotation().baseFunctions); | ||
} | ||
return false; | ||
} | ||
|
||
|
@@ -128,133 +112,41 @@ bool DocStringAnalyser::visit(EventDefinition const& _event) | |
return true; | ||
} | ||
|
||
void DocStringAnalyser::checkParameters( | ||
CallableDeclaration const& _callable, | ||
StructurallyDocumented const& _node, | ||
StructurallyDocumentedAnnotation& _annotation | ||
) | ||
{ | ||
set<string> validParams; | ||
for (auto const& p: _callable.parameters()) | ||
validParams.insert(p->name()); | ||
if (_callable.returnParameterList()) | ||
for (auto const& p: _callable.returnParameterList()->parameters()) | ||
validParams.insert(p->name()); | ||
auto paramRange = _annotation.docTags.equal_range("param"); | ||
for (auto i = paramRange.first; i != paramRange.second; ++i) | ||
if (!validParams.count(i->second.paramName)) | ||
m_errorReporter.docstringParsingError( | ||
3881_error, | ||
_node.documentation()->location(), | ||
"Documented parameter \"" + | ||
i->second.paramName + | ||
"\" not found in the parameter list of the function." | ||
); | ||
|
||
} | ||
|
||
void DocStringAnalyser::handleConstructor( | ||
CallableDeclaration const& _callable, | ||
StructurallyDocumented const& _node, | ||
StructurallyDocumentedAnnotation& _annotation | ||
) | ||
{ | ||
static set<string> const validTags = set<string>{"author", "dev", "notice", "param"}; | ||
parseDocStrings(_node, _annotation, validTags, "constructor"); | ||
checkParameters(_callable, _node, _annotation); | ||
} | ||
|
||
void DocStringAnalyser::handleCallable( | ||
CallableDeclaration const& _callable, | ||
StructurallyDocumented const& _node, | ||
StructurallyDocumentedAnnotation& _annotation | ||
) | ||
{ | ||
static set<string> const validTags = set<string>{"author", "dev", "notice", "return", "param"}; | ||
parseDocStrings(_node, _annotation, validTags, "functions"); | ||
checkParameters(_callable, _node, _annotation); | ||
|
||
if ( | ||
if (CallableDeclaration const* baseFunction = resolveInheritDoc(_callable.annotation().baseFunctions, _node, _annotation)) | ||
copyMissingTags(_annotation, {baseFunction}); | ||
else if ( | ||
_annotation.docTags.empty() && | ||
_callable.annotation().baseFunctions.size() == 1 && | ||
parameterNamesEqual(_callable, **_callable.annotation().baseFunctions.begin()) | ||
hrkrshnn marked this conversation as resolved.
Show resolved
Hide resolved
|
||
) | ||
copyMissingTags(_annotation, _callable.annotation().baseFunctions); | ||
|
||
if (_node.documentation() && _annotation.docTags.count("author") > 0) | ||
m_errorReporter.warning( | ||
9843_error, _node.documentation()->location(), | ||
"Documentation tag @author is only allowed on contract definitions. " | ||
"It will be disallowed in 0.7.0." | ||
); | ||
} | ||
|
||
void DocStringAnalyser::parseDocStrings( | ||
CallableDeclaration const* DocStringAnalyser::resolveInheritDoc( | ||
set<CallableDeclaration const*> const& _baseFuncs, | ||
StructurallyDocumented const& _node, | ||
StructurallyDocumentedAnnotation& _annotation, | ||
set<string> const& _validTags, | ||
string const& _nodeName | ||
StructurallyDocumentedAnnotation& _annotation | ||
) | ||
{ | ||
DocStringParser parser; | ||
if (_node.documentation() && !_node.documentation()->text()->empty()) | ||
{ | ||
parser.parse(*_node.documentation()->text(), m_errorReporter); | ||
_annotation.docTags = parser.tags(); | ||
} | ||
if (_annotation.inheritdocReference == nullptr) | ||
return nullptr; | ||
|
||
if (auto const callable = findBaseCallable(_baseFuncs, _annotation.inheritdocReference->id())) | ||
return callable; | ||
|
||
size_t returnTagsVisited = 0; | ||
for (auto const& docTag: _annotation.docTags) | ||
{ | ||
if (!_validTags.count(docTag.first)) | ||
m_errorReporter.docstringParsingError( | ||
6546_error, | ||
_node.documentation()->location(), | ||
"Documentation tag @" + docTag.first + " not valid for " + _nodeName + "." | ||
); | ||
else | ||
if (docTag.first == "return") | ||
{ | ||
returnTagsVisited++; | ||
if (auto* varDecl = dynamic_cast<VariableDeclaration const*>(&_node)) | ||
{ | ||
if (!varDecl->isPublic()) | ||
m_errorReporter.docstringParsingError( | ||
9440_error, | ||
_node.documentation()->location(), | ||
"Documentation tag \"@" + docTag.first + "\" is only allowed on public state-variables." | ||
); | ||
if (returnTagsVisited > 1) | ||
m_errorReporter.docstringParsingError( | ||
5256_error, | ||
_node.documentation()->location(), | ||
"Documentation tag \"@" + docTag.first + "\" is only allowed once on state-variables." | ||
); | ||
} | ||
else if (auto* function = dynamic_cast<FunctionDefinition const*>(&_node)) | ||
{ | ||
string content = docTag.second.content; | ||
string firstWord = content.substr(0, content.find_first_of(" \t")); | ||
m_errorReporter.docstringParsingError( | ||
4682_error, | ||
_node.documentation()->location(), | ||
"Documentation tag @inheritdoc references contract \"" + | ||
_annotation.inheritdocReference->name() + | ||
"\", but the contract does not contain a function that is overridden by this function." | ||
); | ||
|
||
if (returnTagsVisited > function->returnParameters().size()) | ||
m_errorReporter.docstringParsingError( | ||
2604_error, | ||
_node.documentation()->location(), | ||
"Documentation tag \"@" + docTag.first + " " + docTag.second.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 \"@" + docTag.first + " " + docTag.second.content + "\"" + | ||
" does not contain the name of its return parameter." | ||
); | ||
} | ||
} | ||
} | ||
} | ||
return nullptr; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -15,12 +15,6 @@ | |
along with solidity. If not, see <http://www.gnu.org/licenses/>. | ||
*/ | ||
// SPDX-License-Identifier: GPL-3.0 | ||
/** | ||
* @author Christian <[email protected]> | ||
* @date 2015 | ||
* Parses and analyses the doc strings. | ||
* Stores the parsing results in the AST annotations and reports errors. | ||
*/ | ||
|
||
#pragma once | ||
|
||
|
@@ -35,7 +29,7 @@ namespace solidity::frontend | |
{ | ||
|
||
/** | ||
* Parses and analyses the doc strings. | ||
* Analyses and validates the doc strings. | ||
* Stores the parsing results in the AST annotations and reports errors. | ||
*/ | ||
class DocStringAnalyser: private ASTConstVisitor | ||
|
@@ -45,20 +39,13 @@ class DocStringAnalyser: private ASTConstVisitor | |
bool analyseDocStrings(SourceUnit const& _sourceUnit); | ||
|
||
private: | ||
bool visit(ContractDefinition const& _contract) override; | ||
bool visit(FunctionDefinition const& _function) override; | ||
bool visit(VariableDeclaration const& _variable) override; | ||
bool visit(ModifierDefinition const& _modifier) override; | ||
bool visit(EventDefinition const& _event) override; | ||
|
||
void checkParameters( | ||
CallableDeclaration const& _callable, | ||
StructurallyDocumented const& _node, | ||
StructurallyDocumentedAnnotation& _annotation | ||
); | ||
|
||
void handleConstructor( | ||
CallableDeclaration const& _callable, | ||
CallableDeclaration const* resolveInheritDoc( | ||
std::set<CallableDeclaration const*> const& _baseFunctions, | ||
StructurallyDocumented const& _node, | ||
StructurallyDocumentedAnnotation& _annotation | ||
); | ||
|
@@ -69,19 +56,6 @@ class DocStringAnalyser: private ASTConstVisitor | |
StructurallyDocumentedAnnotation& _annotation | ||
); | ||
|
||
void handleDeclaration( | ||
Declaration const& _declaration, | ||
StructurallyDocumented const& _node, | ||
StructurallyDocumentedAnnotation& _annotation | ||
); | ||
|
||
void parseDocStrings( | ||
StructurallyDocumented const& _node, | ||
StructurallyDocumentedAnnotation& _annotation, | ||
std::set<std::string> const& _validTags, | ||
std::string const& _nodeName | ||
); | ||
|
||
langutil::ErrorReporter& m_errorReporter; | ||
}; | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the statements in the previous paragraph are wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A short test confirmed that it is correct. Probably because we use
interfaceFunctions()
to iterate over the things that will eventually be output as natspec (see Natspec.cpp)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We do parse tags on internal functions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah. I guess we check them but we never output anything for it