Add support for multiple (named) return parameters in Natspec devdoc by corydickson · Pull Request #7534 · argotorg/solidity

corydickson · 2019-10-13T02:05:15Z

Description

Addresses #1141

Adds an object similar to the one used to document params only if multiple named return parameters are specified. Otherwise the return field will contain a single string.

Checklist

Code compiles correctly
All tests are passing
New tests have been created which fail without the change (if possible)
README / documentation was extended, if necessary(?)
Changelog entry (if change is visible to the user)
Used meaningful commit messages

erak · 2019-10-16T10:28:12Z

+	"            \"a\": \"Documentation for the first parameter starts here. Since it's a really complicated parameter we need 2 lines\",\n"
+	"            \"second\": \"Documentation for the second parameter\"\n"
+	"        },\n"
+	"        \"return\": \"The result of the multiplicationAnd cookies with nutella\"\n"


I think this needs to change as well such that return always contains at least one key-value-pair for each return parameter. Since they don't have a name in this test, we could think about adding a unique placeholder for each parameter:

"return": { "_1": "The result of the multiplication", "_2": "And cookies with nutella" },

If the parameter has a name it could use that as the key.

I think we can't check if the documentation contains the name of the parameter (as we do for function parameters) since they are allowed to have no name (the compiler allows function parameters to don't have a name, but NatSpec doesn't).

erak · 2019-10-16T10:28:28Z

+			/// @return The result of the multiplication
+			/// @return And cookies with nutella
+			function mul(uint a, uint second) public returns (uint, uint) {
+        		uint mul = a * 7;


Indentation is off.

erak

I think we need to revisit the concept again. E.g. We should also take the first word of the line and check if this corresponds to named return parameter, if any. If return parameters don't have a name, this check should be ignored.

erak · 2019-10-16T10:54:06Z

Please also add a Changelog entry since this is a user-facing change that will effect tools.

corydickson · 2019-10-17T19:38:39Z

I think we need to revisit the concept again. E.g. We should also take the first word of the line and check if this corresponds to named return parameter, if any. If return parameters don't have a name, this check should be ignored.

Would it make sense to have this method perform that check and throw an error? It might be cleaner to reuse the current logic in the doc parser for param adding the return case for parseDocTag. Though this would break most contract documentation i.e. the gnosis contract examples, no?

erak · 2019-10-18T11:31:01Z

@Gh1dra Yes, I think we should use the same logic as we do for function parameters. And yes, you're right. This would break existing contracts that use named return parameters (and that probably do not use the parameter name as the first word). But since the current behaviour is broken, I'd tend to properly clean it up and include this PR in our next breaking release, which will be 0.6.0.

erak · 2019-10-18T17:27:09Z

The failing macOS build (b_osx) should be gone after a rebase, which will include #7552.

erak · 2019-10-23T14:31:26Z

@Gh1dra Great, thanks! I left a few comments that should be easy to fix. And I also changed the base branch to develop_060 since it's breaking. Could you please rebase your changes on top of it as well?
We need an entry in solidity_repo/docs/060-breaking-changes.rst as well. You can run the HTML generation with ./scripts/docs.sh.

corydickson · 2019-10-27T16:01:04Z

@erak Thanks for the feedback! Let me know if you'd like me to squash the earlier commits.

erak · 2019-10-28T18:25:01Z

 * Standard JSON Interface: Add option to disable or choose hash method between IPFS and Swarm for the bytecode metadata.
 * Syntax: ``push(element)`` for dynamic storage arrays do not return the new length anymore.
 * Type checker: Resulting type of exponentiation is equal to the type of the base. Also allow signed types for the base.
+ * Natspec JSON Interface: Support multiple ``@return`` statements in dev documentation to behave like named parameters.


I think we need to be a little bit more specific and mention both, the fixed support and the breaking implication of multiple return statements in the dev docs.

Suggested change

* Natspec JSON Interface: Support multiple ``@return`` statements in dev documentation to behave like named parameters.

* Natspec JSON Interface: Support multiple ``@return`` statements in ``@dev`` documentation and force them to have define a name if parameter is named.

erak · 2019-10-28T19:15:18Z

@Gh1dra I've read through it again carefully and found some small issues. I think we can agree on #7534 (comment) quickly and incorporate the outcome. Otherwise your PR looks really good and I'm looking forward to get it merged.

erak · 2019-10-28T19:16:21Z

@Gh1dra Sorry, but it also has merge conflicts.

corydickson · 2019-10-28T20:52:29Z

The branch should be rebased properly now, apologies for the confusion

erak · 2019-10-28T22:21:42Z

    /// @param funding Initial funding for market
    /// @param outcomeIndex Index of exponential term to extract (for use by marginal price function)
-    /// @return A result structure composed of the sum, the offset used, and the summand associated with the supplied index
+    /// @return sum of the outcomes


Ok, that's probably nit-picking, but these could be changed to /// @return sum The Sum of outcomes etc.

erak · 2019-10-28T22:23:58Z

 	/// @return      A JSON representation
 	///              of the contract's developer documentation
 	static Json::Value devDocumentation(std::multimap<std::string, DocTag> const& _tags);
+	static Json::Value extractReturnParameterDocs(std::multimap<std::string, DocTag> const& _tags, FunctionDefinition const& _functionDef);


This could use some more documentation, perhaps similar to the function devDocumentation above.

chriseth · 2019-11-06T20:41:52Z

This looks good! I'll rebase and merge.

Fix whitespace

…tor test Update 060 doc with natspec change Add two more tests with mixed usage Fix solc-js fix changelog

@erak

author:@erak Co-Authored-By: Erik K <erik@zirkular.io>

corydickson force-pushed the return-multiple-natspec branch 4 times, most recently from 2a14c7a to a97e996 Compare October 13, 2019 06:46

erak reviewed Oct 16, 2019

View reviewed changes

erak requested changes Oct 16, 2019

View reviewed changes

erak added the breaking change ⚠️ label Oct 18, 2019

corydickson force-pushed the return-multiple-natspec branch from fcf1990 to 29ec944 Compare October 21, 2019 22:21

corydickson commented Oct 21, 2019

View reviewed changes

Comment thread libsolidity/interface/Natspec.cpp Outdated

erak reviewed Oct 23, 2019

View reviewed changes

Comment thread libsolidity/interface/Natspec.cpp Outdated

erak reviewed Oct 23, 2019

View reviewed changes

Comment thread libsolidity/interface/Natspec.cpp Outdated

erak reviewed Oct 23, 2019

View reviewed changes

Comment thread libsolidity/interface/Natspec.cpp Outdated

erak reviewed Oct 23, 2019

View reviewed changes

Comment thread libsolidity/interface/Natspec.cpp Outdated

erak reviewed Oct 23, 2019

View reviewed changes

Comment thread libsolidity/interface/Natspec.cpp

erak changed the base branch from develop to develop_060 October 23, 2019 14:30

erak reviewed Oct 24, 2019

View reviewed changes

Comment thread test/libsolidity/SolidityNatspecJSON.cpp Outdated

corydickson force-pushed the return-multiple-natspec branch 2 times, most recently from 8ff6e63 to 972dba1 Compare October 27, 2019 15:23