From 16fe59b7b4b6c0452cbc338cfaf113b514b52beb Mon Sep 17 00:00:00 2001 From: cd10012 Date: Sun, 27 Oct 2019 10:27:47 -0400 Subject: [PATCH] Implement @erak review notes by creating function and adding constructor test Update 060 doc with natspec change Add two more tests with mixed usage Fix solc-js fix changelog --- Changelog.md | 3 +- docs/060-breaking-changes.rst | 7 ++ libsolidity/interface/Natspec.cpp | 64 +++++++------- libsolidity/interface/Natspec.h | 2 + test/externalTests/solc-js/DAO/Token.sol | 10 +-- test/libsolidity/SolidityNatspecJSON.cpp | 108 ++++++++++++++++++++--- 6 files changed, 146 insertions(+), 48 deletions(-) diff --git a/Changelog.md b/Changelog.md index 746887d3c..51f716657 100644 --- a/Changelog.md +++ b/Changelog.md @@ -14,7 +14,7 @@ Breaking changes: * Syntax: Abstract contracts need to be marked explicitly as abstract by using the ``abstract`` keyword. * Inline Assembly: Only strict inline assembly is allowed. * 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. + * Natspec JSON Interface: Support multiple ``@return`` statements in dev documentation to behave like parameters where the first word in the entry must contain the name. * Source mappings: Add "modifier depth" as a fifth field in the source mappings. @@ -48,7 +48,6 @@ Bugfixes: * Type Checker: Treat magic variables as unknown identifiers in inline assembly - ### 0.5.12 (2019-10-01) Language Features: diff --git a/docs/060-breaking-changes.rst b/docs/060-breaking-changes.rst index cbe9178a6..ef0ad9e6e 100644 --- a/docs/060-breaking-changes.rst +++ b/docs/060-breaking-changes.rst @@ -66,10 +66,17 @@ This section gives detailed instructions on how to update prior code for every b * Change ``uint length = array.push(value)`` to ``array.push(value);``. The new length can be accessed via ``array.length``. +* For every named return parameter in a function's ``@dev`` documentation define a ``@return`` + entry which contains the parameter's name as the first word. E.g. if you have function ``f()`` defined + like ``function f() public returns (uint value)`` and a ``@dev`` annotating it, document its return + parameters like so: ``@return value The return value.``. You can mix named and un-named return parameters + documentation so long as the notices are in the order they appear in the tuple return type. + New Features ============ * The :ref:`try/catch statement ` allows you to react on failed external calls. + * Natspec supports multiple return parameters in dev documentation, enforcing the same naming check as ``@param``. * Yul and Inline Assembly have a new statement called ``leave`` that exits the current function. diff --git a/libsolidity/interface/Natspec.cpp b/libsolidity/interface/Natspec.cpp index 0da229a7f..2c8c68694 100644 --- a/libsolidity/interface/Natspec.cpp +++ b/libsolidity/interface/Natspec.cpp @@ -102,38 +102,10 @@ Json::Value Natspec::devDocumentation(ContractDefinition const& _contractDef) if (!method.empty()) { // add the function, only if we have any documentation to add - Json::Value ret(Json::objectValue); + Json::Value jsonReturn = extractReturnParameterDocs(fun->annotation().docTags, *fun); - auto returnParams = fun->returnParameters(); - auto returnDoc = fun->annotation().docTags.equal_range("return"); - - if (!returnParams.empty()) - { - unsigned int n = 0; - for (auto i = returnDoc.first; i != returnDoc.second; i++) - { - string paramName = returnParams.at(n)->name(); - string content = i->second.content; - - if (paramName.empty()) - { - paramName = "_" + std::to_string(n+1); - } - else - { - //check to make sure the first word of the doc str is the same as the return name - auto nameEndPos = content.find_first_of(" \t"); - solAssert(content.substr(0, nameEndPos) == paramName, "No return param name given: " + paramName); - content = content.substr(nameEndPos+1); - } - - ret[paramName] = Json::Value(content); - n++; - } - } - - if (!ret.empty()) - method["return"] = ret; + if (!jsonReturn.empty()) + method["returns"] = jsonReturn; methods[it.second->externalSignature()] = method; } @@ -145,6 +117,36 @@ Json::Value Natspec::devDocumentation(ContractDefinition const& _contractDef) return doc; } +Json::Value Natspec::extractReturnParameterDocs(std::multimap const& _tags, FunctionDefinition const& _functionDef) +{ + Json::Value jsonReturn{Json::objectValue}; + auto returnDocs = _tags.equal_range("return"); + + if (!_functionDef.returnParameters().empty()) + { + size_t n = 0; + for (auto i = returnDocs.first; i != returnDocs.second; i++) + { + string paramName = _functionDef.returnParameters().at(n)->name(); + string content = i->second.content; + + if (paramName.empty()) + paramName = "_" + std::to_string(n); + else + { + //check to make sure the first word of the doc str is the same as the return name + auto nameEndPos = content.find_first_of(" \t"); + solAssert(content.substr(0, nameEndPos) == paramName, "No return param name given: " + paramName); + content = content.substr(nameEndPos+1); + } + + jsonReturn[paramName] = Json::Value(content); + n++; + } + } + + return jsonReturn; +} string Natspec::extractDoc(multimap const& _tags, string const& _name) { diff --git a/libsolidity/interface/Natspec.h b/libsolidity/interface/Natspec.h index 7a0c40a1a..fee38c133 100644 --- a/libsolidity/interface/Natspec.h +++ b/libsolidity/interface/Natspec.h @@ -28,6 +28,7 @@ #include #include #include +#include namespace dev { @@ -60,6 +61,7 @@ private: /// @return A JSON representation /// of the contract's developer documentation static Json::Value devDocumentation(std::multimap const& _tags); + static Json::Value extractReturnParameterDocs(std::multimap const& _tags, FunctionDefinition const& _functionDef); }; } //solidity NS diff --git a/test/externalTests/solc-js/DAO/Token.sol b/test/externalTests/solc-js/DAO/Token.sol index 149e61c5e..0daa76ec9 100644 --- a/test/externalTests/solc-js/DAO/Token.sol +++ b/test/externalTests/solc-js/DAO/Token.sol @@ -44,13 +44,13 @@ abstract contract TokenInterface { uint256 public totalSupply; /// @param _owner The address from which the balance will be retrieved - /// @return The balance + /// @return balance The balance function balanceOf(address _owner) public view returns (uint256 balance); /// @notice Send `_amount` tokens to `_to` from `msg.sender` /// @param _to The address of the recipient /// @param _amount The amount of tokens to be transferred - /// @return Whether the transfer was successful or not + /// @return success Whether the transfer was successful or not function transfer(address _to, uint256 _amount) public returns (bool success); /// @notice Send `_amount` tokens to `_to` from `_from` on the condition it @@ -58,19 +58,19 @@ abstract contract TokenInterface { /// @param _from The address of the origin of the transfer /// @param _to The address of the recipient /// @param _amount The amount of tokens to be transferred - /// @return Whether the transfer was successful or not + /// @return success Whether the transfer was successful or not function transferFrom(address _from, address _to, uint256 _amount) public returns (bool success); /// @notice `msg.sender` approves `_spender` to spend `_amount` tokens on /// its behalf /// @param _spender The address of the account able to transfer the tokens /// @param _amount The amount of tokens to be approved for transfer - /// @return Whether the approval was successful or not + /// @return success Whether the approval was successful or not function approve(address _spender, uint256 _amount) public returns (bool success); /// @param _owner The address of the account owning tokens /// @param _spender The address of the account able to transfer the tokens - /// @return Amount of remaining tokens of _owner that _spender is allowed + /// @return remaining Amount of remaining tokens of _owner that _spender is allowed /// to spend function allowance( address _owner, diff --git a/test/libsolidity/SolidityNatspecJSON.cpp b/test/libsolidity/SolidityNatspecJSON.cpp index d553c313e..5aa15f1ed 100644 --- a/test/libsolidity/SolidityNatspecJSON.cpp +++ b/test/libsolidity/SolidityNatspecJSON.cpp @@ -378,7 +378,7 @@ BOOST_AUTO_TEST_CASE(dev_return) " \"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\": {\n" + " \"returns\": {\n" " \"d\": \"The result of the multiplication\"\n" " }\n" " }\n" @@ -411,7 +411,7 @@ BOOST_AUTO_TEST_CASE(dev_return_desc_after_nl) " \"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\": {\n" + " \"returns\": {\n" " \"d\": \"The result of the multiplication\"\n" " }\n" " }\n" @@ -420,6 +420,79 @@ BOOST_AUTO_TEST_CASE(dev_return_desc_after_nl) checkNatspec(sourceCode, "test", natspec, false); } +BOOST_AUTO_TEST_CASE(dev_return_desc_multiple_unamed_mixed) +{ + char const* sourceCode = R"( + contract test { + /// @dev Multiplies a number by 7 and adds second parameter + /// @param a Documentation for the first parameter starts here. + /// Since it's a really complicated parameter we need 2 lines + /// @param second Documentation for the second parameter + /// @return The result of the multiplication + /// @return _cookies And cookies with nutella + function mul(uint a, uint second) public returns (uint, uint _cookies) { + uint mul = a * 7; + return (mul, second); + } + } + )"; + + char const* natspec = "{" + "\"methods\":{" + " \"mul(uint256,uint256)\":{ \n" + " \"details\": \"Multiplies a number by 7 and adds second parameter\",\n" + " \"params\": {\n" + " \"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" + " \"returns\": {\n" + " \"_0\": \"The result of the multiplication\",\n" + " \"_cookies\": \"And cookies with nutella\"\n" + " }\n" + " }\n" + "}}"; + + checkNatspec(sourceCode, "test", natspec, false); +} + +BOOST_AUTO_TEST_CASE(dev_return_desc_multiple_unamed_mixed_2) +{ + char const* sourceCode = R"( + contract test { + /// @dev Multiplies a number by 7 and adds second parameter + /// @param a Documentation for the first parameter starts here. + /// Since it's a really complicated parameter we need 2 lines + /// @param second Documentation for the second parameter + /// @return _cookies And cookies with nutella + /// @return The result of the multiplication + /// @return _milk And milk with nutella + function mul(uint a, uint second) public returns (uint _cookies, uint, uint _milk) { + uint mul = a * 7; + uint milk = 4; + return (mul, second, milk); + } + } + )"; + + char const* natspec = "{" + "\"methods\":{" + " \"mul(uint256,uint256)\":{ \n" + " \"details\": \"Multiplies a number by 7 and adds second parameter\",\n" + " \"params\": {\n" + " \"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" + " \"returns\": {\n" + " \"_cookies\": \"And cookies with nutella\",\n" + " \"_1\": \"The result of the multiplication\",\n" + " \"_milk\": \"And milk with nutella\"\n" + " }\n" + " }\n" + "}}"; + + checkNatspec(sourceCode, "test", natspec, false); +} + BOOST_AUTO_TEST_CASE(dev_return_desc_multiple_unamed) { char const* sourceCode = R"( @@ -445,9 +518,9 @@ BOOST_AUTO_TEST_CASE(dev_return_desc_multiple_unamed) " \"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\": {\n" - " \"_1\": \"The result of the multiplication\",\n" - " \"_2\": \"And cookies with nutella\"\n" + " \"returns\": {\n" + " \"_0\": \"The result of the multiplication\",\n" + " \"_1\": \"And cookies with nutella\"\n" " }\n" " }\n" "}}"; @@ -466,7 +539,7 @@ BOOST_AUTO_TEST_CASE(dev_return_desc_multiple) /// @return d The result of the multiplication /// @return f And cookies with nutella function mul(uint a, uint second) public returns (uint d, uint f) { - uint mul = a * 7; + uint mul = a * 7; return (mul, second); } } @@ -480,7 +553,7 @@ BOOST_AUTO_TEST_CASE(dev_return_desc_multiple) " \"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\": {\n" + " \"returns\": {\n" " \"d\": \"The result of the multiplication\",\n" " \"f\": \"And cookies with nutella\"\n" " }\n" @@ -514,7 +587,7 @@ BOOST_AUTO_TEST_CASE(dev_multiline_return) " \"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\": {\n" + " \"returns\": {\n" " \"d\": \"The result of the multiplication and cookies with nutella\",\n" " }\n" " }\n" @@ -549,7 +622,7 @@ BOOST_AUTO_TEST_CASE(dev_multiline_comment) " \"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\": {\n" + " \"returns\": {\n" " \"d\": \"The result of the multiplication and cookies with nutella\",\n" " }\n" " }\n" @@ -558,6 +631,21 @@ BOOST_AUTO_TEST_CASE(dev_multiline_comment) checkNatspec(sourceCode, "test", natspec, false); } +BOOST_AUTO_TEST_CASE(dev_documenting_no_return_paramname) +{ + char const* sourceCode = R"( + contract test { + /// @dev Multiplies a number by 7 and adds second parameter + /// @param a Documentation for the first parameter + /// @param second Documentation for the second parameter + /// @return + function mul(uint a, uint second) public returns (uint d) { return a * 7 + second; } + } + )"; + + expectNatspecError(sourceCode); +} + BOOST_AUTO_TEST_CASE(dev_contract_no_doc) { char const* sourceCode = R"( @@ -871,7 +959,7 @@ BOOST_AUTO_TEST_CASE(dev_constructor_and_function) "a" : "Documentation for the first parameter starts here. Since it's a really complicated parameter we need 2 lines", "second" : "Documentation for the second parameter" }, - "return" : { + "returns" : { "d": "The result of the multiplication and cookies with nutella" } },