Natspec: Implement inheritance and @inheritdoc #9218

Marenz · 2020-06-17T12:16:24Z

fixes #8911

libsolidity/analysis/DocStringAnalyser.cpp

Changelog.md

libsolidity/analysis/DocStringAnalyser.cpp

test/libsolidity/SolidityNatspecJSON.cpp

hrkrshnn · 2020-06-18T10:32:48Z

We are missing an entry in the documentation for @inheritdoc. Other than that, this looks good.

chriseth · 2020-06-18T11:34:23Z

Does this both implement the explicit inheritance via @inheritdoc and the implicit inheritance or just the first?

Marenz · 2020-06-18T11:34:41Z

@chriseth this implements both

libsolidity/analysis/DocStringAnalyser.cpp

hrkrshnn · 2020-06-22T07:06:40Z

There is a minor issue. According to documentation, @author is not applicable to public state variables, but applicable to functions. What should happen if a public state variable overrides a function that has an @author tag?

This could be an issue with the documentation, because when running tests, I was able to use @author on public state variables.

christianparpart

Mostly constness suggestions and one boost algorithm proposal.

libsolidity/analysis/DocStringAnalyser.cpp

Marenz · 2020-06-22T11:39:44Z

@hrkrshnn according to the code author is fine for public vars. I'll fix the docu

Marenz · 2020-06-22T11:41:17Z

Ah well.. no.

		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."
			);

chriseth · 2020-07-09T14:43:52Z

libsolidity/analysis/DocStringAnalyser.cpp

@@ -52,6 +52,17 @@ void copyMissingTags(StructurallyDocumentedAnnotation& _target, set<CallableDecl
 _target.docTags.emplace(tag, content);
 }

+CallableDeclaration const* findBaseCallable(std::set<CallableDeclaration const*>& _baseFunctions, int64_t _contractId)


Suggested change

CallableDeclaration const* findBaseCallable(std::set<CallableDeclaration const*>& _baseFunctions, int64_t _contractId)

CallableDeclaration const* findBaseCallable(set<CallableDeclaration const*> const& _baseFunctions, int64_t _contractId)

chriseth · 2020-07-09T14:46:39Z

libsolidity/analysis/DocStringAnalyser.cpp

+ _node.documentation()->location(),
+ "Documentation tag @inheritdoc references contract \"" +
+ _annotation.inheritdocReference->name() +
+ "\", but the contract does not override this function."


shouldn't it be
"...contract does not contain a function that is overridden by this function"?

chriseth · 2020-07-09T14:48:17Z

libsolidity/analysis/DocStringTagParser.h

+{
+
+/**
+ * Parses the doc tags and does basic vadility checks.


Suggested change

* Parses the doc tags and does basic vadility checks.

* Parses the doc tags and does basic validity checks.

chriseth · 2020-07-09T14:49:38Z

libsolidity/analysis/ReferencesResolver.cpp

+ 5142_error,
+ _documentation.location(),
+ "Documentation tag @inheritdoc can only be given once."
+ );


indentation

chriseth · 2020-07-09T14:53:16Z

libsolidity/analysis/ReferencesResolver.cpp

+ case 1:
+ {
+ string const& name = _annotation.docTags.find("inheritdoc")->second.content;
+ vector<Declaration const*> results = m_resolver.nameFromCurrentScope(name);


I think we should use pathFromCurrentScope to allow more complex use-cases.

chriseth · 2020-07-09T14:54:24Z

Good apart from the comments!

chriseth · 2020-07-13T16:51:34Z

Changelog.md

@@ -26,6 +26,7 @@ Language Features:
 Compiler Features:
 * NatSpec: Add fields ``kind`` and ``version`` to the JSON output.
 * NatSpec: Inherit tags from unique base functions if derived function does not provide any.
+ * NatSpec: Implement tag ``@inheritdoc`` to copy documentation from a specific contract.


Wrong version

not yet fixed.

I only rebased, fixes are about to come

chriseth · 2020-07-13T16:54:35Z

libsolidity/analysis/ReferencesResolver.cpp

+ string const& name = _annotation.docTags.find("inheritdoc")->second.content;
+ vector<string> path;
+ boost::split(path, name, boost::is_any_of("."));
+ Declaration const* result = m_resolver.pathFromCurrentScope(path);


Do you have a test case for that? You will need to use a multi-file test that uses import.

This test does a resolving of a path: https:/ethereum/solidity/pull/9218/files#diff-5dcd49ff3e08e5ff224ee08fbb7ac514

It's not a contract and thus fails later but it does successfully resolve the path.

Can you please still add a multi-file test?

leonardoalt · 2020-07-16T12:52:55Z

libsolidity/analysis/DocStringAnalyser.cpp

@@ -52,6 +52,17 @@ void copyMissingTags(StructurallyDocumentedAnnotation& _target, set<CallableDecl
 _target.docTags.emplace(tag, content);
 }

+CallableDeclaration const* findBaseCallable(set<CallableDeclaration const*> const& _baseFunctions, int64_t _contractId)


Because that's the type that ContractDefinition->id() returns

Indeed. Now I'm confused why id is int

leonardoalt · 2020-07-16T12:56:56Z

libsolidity/analysis/DocStringAnalyser.cpp

 }

-void DocStringAnalyser::parseDocStrings(
+CallableDeclaration const* DocStringAnalyser::resolveInheritDoc(
+ std::set<CallableDeclaration const*>& _baseFuncs,


Suggested change

std::set<CallableDeclaration const*>& _baseFuncs,

std::set<CallableDeclaration const*> const& _baseFuncs,

?

leonardoalt · 2020-07-16T13:00:37Z

libsolidity/analysis/ReferencesResolver.cpp

+ m_errorReporter.docstringParsingError(
+ 9397_error,
+ _documentation.location(),
+ "Documentation tag @inheritdoc references unexisting contract \"" +


Suggested change

"Documentation tag @inheritdoc references unexisting contract \"" +

"Documentation tag @inheritdoc references inexistent contract \"" +

leonardoalt · 2020-07-16T13:02:07Z

test/libsolidity/SolidityNatspecJSON.cpp

+ checkNatspec(sourceCode, "D", natspec, true);
+}
+
+BOOST_AUTO_TEST_CASE(dev_explicit_inherit_variable)


Did these tests have to be added via cpp tests? Couldn't they have been added as command line tests? (or something that is not cpp tests)

I suppose anything could be a cmdline test, but this file is specifically for natspecs so it does seem to be the right place..
Whether we want/should turn the whole thing into not-cpp tests is a different question (of which I'd be in favour)

Yea, I just wondered if some similar tests had already been "converted" into cmd line tests

I am not sure cmd line tests should be the favored target, maybe a more specific extension in (i)soltest would be better.

leonardoalt · 2020-07-16T13:03:14Z

libsolidity/interface/Natspec.cpp

- Json::Value user;
- // since @notice is the only user tag if missing function should not appear
- user["notice"] = Json::Value(value);
- methods[it.second->externalSignature()] = user;


What happened to these 2?

I simplified the code a bit, they just moved to line 74/75

leonardoalt · 2020-07-16T13:09:27Z

libsolidity/analysis/ReferencesResolver.cpp

+ );
+ }
+ break;
+}


Indentation looks off

leonardoalt · 2020-07-16T13:12:01Z

libsolidity/analysis/DocStringTagParser.cpp

+ "Documentation tag @" + docTag.first + " not valid for " + _nodeName + "."
+ );
+ else
+ if (docTag.first == "return")


Suggested change

if (docTag.first == "return")

else if (docTag.first == "return")

leonardoalt · 2020-07-16T13:12:23Z

libsolidity/analysis/DocStringTagParser.cpp

+ if (docTag.first == "return")
+ {
+ returnTagsVisited++;
+ if (auto* varDecl = dynamic_cast<VariableDeclaration const*>(&_node))


Suggested change

if (auto* varDecl = dynamic_cast<VariableDeclaration const*>(&_node))

if (auto const* varDecl = dynamic_cast<VariableDeclaration const*>(&_node))

leonardoalt · 2020-07-16T13:12:42Z

libsolidity/analysis/DocStringTagParser.cpp

+ "Documentation tag \"@" + docTag.first + "\" is only allowed once on state-variables."
+ );
+ }
+ else if (auto* function = dynamic_cast<FunctionDefinition const*>(&_node))


Suggested change

else if (auto* function = dynamic_cast<FunctionDefinition const*>(&_node))

else if (auto const* function = dynamic_cast<FunctionDefinition const*>(&_node))

Marenz force-pushed the issue-8911 branch 5 times, most recently from a96752e to 0039eb1 Compare June 17, 2020 12:32

hrkrshnn reviewed Jun 17, 2020

View reviewed changes

libsolidity/analysis/DocStringAnalyser.cpp Outdated Show resolved Hide resolved

hrkrshnn reviewed Jun 17, 2020

View reviewed changes

Changelog.md Outdated Show resolved Hide resolved

hrkrshnn reviewed Jun 17, 2020

View reviewed changes

libsolidity/analysis/DocStringAnalyser.cpp Outdated Show resolved Hide resolved

hrkrshnn reviewed Jun 18, 2020

View reviewed changes

libsolidity/analysis/DocStringAnalyser.cpp Outdated Show resolved Hide resolved

hrkrshnn reviewed Jun 18, 2020

View reviewed changes

libsolidity/analysis/DocStringAnalyser.cpp Show resolved Hide resolved

hrkrshnn reviewed Jun 18, 2020

View reviewed changes

libsolidity/analysis/DocStringAnalyser.cpp Outdated Show resolved Hide resolved

hrkrshnn reviewed Jun 18, 2020

View reviewed changes

libsolidity/analysis/DocStringAnalyser.cpp Outdated Show resolved Hide resolved

hrkrshnn reviewed Jun 18, 2020

View reviewed changes

test/libsolidity/SolidityNatspecJSON.cpp Show resolved Hide resolved

Marenz force-pushed the issue-8911 branch 3 times, most recently from 685f833 to 5450612 Compare June 18, 2020 10:24

Marenz force-pushed the issue-8911 branch 4 times, most recently from 796e12b to e314541 Compare June 18, 2020 15:21

hrkrshnn reviewed Jun 19, 2020

View reviewed changes

libsolidity/analysis/DocStringAnalyser.cpp Outdated Show resolved Hide resolved

christianparpart suggested changes Jun 22, 2020

View reviewed changes

Marenz force-pushed the issue-8911 branch 2 times, most recently from a955f59 to 07e4ccf Compare June 22, 2020 09:31

chriseth reviewed Jul 9, 2020

View reviewed changes

Marenz force-pushed the issue-8911 branch from 2e777e9 to d164c71 Compare July 13, 2020 13:59

chriseth reviewed Jul 13, 2020

View reviewed changes

Marenz force-pushed the issue-8911 branch 2 times, most recently from 6dfa28e to 636d259 Compare July 14, 2020 13:55

leonardoalt reviewed Jul 16, 2020

View reviewed changes

libsolidity/analysis/ReferencesResolver.cpp Outdated

);

}

break;

}

Copy link

Member

leonardoalt Jul 16, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indentation looks off

Marenz reacted with thumbs up emoji

leonardoalt reviewed Jul 16, 2020

View reviewed changes

Marenz force-pushed the issue-8911 branch from 636d259 to 5f298dc Compare July 16, 2020 13:56

leonardoalt previously approved these changes Jul 20, 2020

View reviewed changes

chriseth previously approved these changes Jul 20, 2020

View reviewed changes

NatSpec: Implement @inheritdoc

ba0a4de

Marenz dismissed stale reviews from chriseth and leonardoalt via ba0a4de July 20, 2020 16:32

Marenz force-pushed the issue-8911 branch from 5f298dc to ba0a4de Compare July 20, 2020 16:32

chriseth approved these changes Jul 20, 2020

View reviewed changes

chriseth merged commit e7f26c2 into develop Jul 20, 2020

chriseth deleted the issue-8911 branch July 20, 2020 16:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Natspec: Implement inheritance and @inheritdoc #9218

Natspec: Implement inheritance and @inheritdoc #9218

Marenz commented Jun 17, 2020

hrkrshnn commented Jun 18, 2020

chriseth commented Jun 18, 2020

Marenz commented Jun 18, 2020

hrkrshnn commented Jun 22, 2020

christianparpart left a comment

Marenz commented Jun 22, 2020

Marenz commented Jun 22, 2020

chriseth Jul 9, 2020

chriseth Jul 9, 2020

chriseth Jul 9, 2020

chriseth Jul 9, 2020

chriseth Jul 9, 2020

chriseth commented Jul 9, 2020

chriseth Jul 13, 2020

chriseth Jul 14, 2020

Marenz Jul 14, 2020

chriseth Jul 13, 2020

Marenz Jul 13, 2020

chriseth Jul 14, 2020

leonardoalt Jul 16, 2020

Marenz Jul 16, 2020

leonardoalt Jul 16, 2020

leonardoalt Jul 16, 2020

leonardoalt Jul 16, 2020

leonardoalt Jul 16, 2020

Marenz Jul 16, 2020

leonardoalt Jul 16, 2020

Marenz Jul 16, 2020

leonardoalt Jul 16, 2020

Marenz Jul 16, 2020

leonardoalt Jul 16, 2020

leonardoalt Jul 16, 2020

leonardoalt Jul 16, 2020

leonardoalt Jul 16, 2020

	CallableDeclaration const* findBaseCallable(std::set<CallableDeclaration const*>& _baseFunctions, int64_t _contractId)
	CallableDeclaration const* findBaseCallable(set<CallableDeclaration const*> const& _baseFunctions, int64_t _contractId)

	* Parses the doc tags and does basic vadility checks.
	* Parses the doc tags and does basic validity checks.

	std::set<CallableDeclaration const*>& _baseFuncs,
	std::set<CallableDeclaration const*> const& _baseFuncs,

	"Documentation tag @inheritdoc references unexisting contract \"" +
	"Documentation tag @inheritdoc references inexistent contract \"" +

	if (docTag.first == "return")
	else if (docTag.first == "return")

	if (auto* varDecl = dynamic_cast<VariableDeclaration const*>(&_node))
	if (auto const* varDecl = dynamic_cast<VariableDeclaration const*>(&_node))

	else if (auto* function = dynamic_cast<FunctionDefinition const*>(&_node))
	else if (auto const* function = dynamic_cast<FunctionDefinition const*>(&_node))

Natspec: Implement inheritance and @inheritdoc #9218

Natspec: Implement inheritance and @inheritdoc #9218

Conversation

Marenz commented Jun 17, 2020

hrkrshnn commented Jun 18, 2020

chriseth commented Jun 18, 2020

Marenz commented Jun 18, 2020

hrkrshnn commented Jun 22, 2020

christianparpart left a comment

Choose a reason for hiding this comment

Marenz commented Jun 22, 2020

Marenz commented Jun 22, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chriseth commented Jul 9, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment