From 1737bd7ded0e32934e0b2bb1973307f2517efeb7 Mon Sep 17 00:00:00 2001 From: Mathias Baumann Date: Tue, 9 Feb 2021 13:04:10 +0100 Subject: [PATCH] Natspec: Don't copy from base function if return parameters differ --- Changelog.md | 1 + libsolidity/interface/Natspec.cpp | 26 +++++++-- libsolidity/interface/Natspec.h | 4 +- test/libsolidity/SolidityNatspecJSON.cpp | 58 ++++++++++++++++++- .../invalid/return_param_amount_differs.sol | 14 +++++ .../invalid/return_param_amount_differs2.sol | 15 +++++ 6 files changed, 109 insertions(+), 9 deletions(-) create mode 100644 test/libsolidity/syntaxTests/natspec/invalid/return_param_amount_differs.sol create mode 100644 test/libsolidity/syntaxTests/natspec/invalid/return_param_amount_differs2.sol diff --git a/Changelog.md b/Changelog.md index 3a99d786f..32968f11a 100644 --- a/Changelog.md +++ b/Changelog.md @@ -24,6 +24,7 @@ Compiler Features: Bugfixes: + * Natspec: Fix internal error related to the `@returns` documentation for a public state variable overriding a function. * Antlr Grammar: Fix parsing of import paths involving properly distinguishing between empty and non-empty string literals in general. * AST Output: Fix ``kind`` field of ``ModifierInvocation`` for base constructor calls. * SMTChecker: Fix false positive and false negative on ``push`` as LHS of a compound assignment. diff --git a/libsolidity/interface/Natspec.cpp b/libsolidity/interface/Natspec.cpp index 1251fcb8d..402d8e15c 100644 --- a/libsolidity/interface/Natspec.cpp +++ b/libsolidity/interface/Natspec.cpp @@ -134,7 +134,10 @@ Json::Value Natspec::devDocumentation(ContractDefinition const& _contractDef) { Json::Value method(devDocumentation(fun->annotation().docTags)); // add the function, only if we have any documentation to add - Json::Value jsonReturn = extractReturnParameterDocs(fun->annotation().docTags, *fun); + Json::Value jsonReturn = extractReturnParameterDocs( + fun->annotation().docTags, + fun->functionType(false)->returnParameterNames() + ); if (!jsonReturn.empty()) method["returns"] = move(jsonReturn); @@ -149,9 +152,20 @@ Json::Value Natspec::devDocumentation(ContractDefinition const& _contractDef) if (auto devDoc = devDocumentation(varDecl->annotation().docTags); !devDoc.empty()) doc["stateVariables"][varDecl->name()] = devDoc; - solAssert(varDecl->annotation().docTags.count("return") <= 1, ""); + auto const assignIfNotEmpty = [&](string const& _name, Json::Value const& _content) + { + if (!_content.empty()) + doc["stateVariables"][varDecl->name()][_name] = _content; + }; + if (varDecl->annotation().docTags.count("return") == 1) - doc["stateVariables"][varDecl->name()]["return"] = extractDoc(varDecl->annotation().docTags, "return"); + assignIfNotEmpty("return", extractDoc(varDecl->annotation().docTags, "return")); + + if (FunctionTypePointer functionType = varDecl->functionType(false)) + assignIfNotEmpty("returns", extractReturnParameterDocs( + varDecl->annotation().docTags, + functionType->returnParameterNames() + )); } for (auto const& event: _contractDef.events()) @@ -165,17 +179,17 @@ Json::Value Natspec::devDocumentation(ContractDefinition const& _contractDef) return doc; } -Json::Value Natspec::extractReturnParameterDocs(std::multimap const& _tags, FunctionDefinition const& _functionDef) +Json::Value Natspec::extractReturnParameterDocs(std::multimap const& _tags, vector const& _returnParameterNames) { Json::Value jsonReturn{Json::objectValue}; auto returnDocs = _tags.equal_range("return"); - if (!_functionDef.returnParameters().empty()) + if (!_returnParameterNames.empty()) { size_t n = 0; for (auto i = returnDocs.first; i != returnDocs.second; i++) { - string paramName = _functionDef.returnParameters().at(n)->name(); + string paramName = _returnParameterNames.at(n); string content = i->second.content; if (paramName.empty()) diff --git a/libsolidity/interface/Natspec.h b/libsolidity/interface/Natspec.h index 2739eeb35..bbf624384 100644 --- a/libsolidity/interface/Natspec.h +++ b/libsolidity/interface/Natspec.h @@ -68,10 +68,10 @@ private: /// Helper-function that will create a json object for the "returns" field for a given function definition. /// @param _tags docTags that are used. - /// @param _functionDef functionDefinition that is used to determine which return parameters are named. + /// @param _returnParameterNames names of the return parameters /// @return A JSON representation /// of a method's return notice documentation - static Json::Value extractReturnParameterDocs(std::multimap const& _tags, FunctionDefinition const& _functionDef); + static Json::Value extractReturnParameterDocs(std::multimap const& _tags, std::vector const& _returnParameterNames); }; } diff --git a/test/libsolidity/SolidityNatspecJSON.cpp b/test/libsolidity/SolidityNatspecJSON.cpp index 53b377988..4416adc02 100644 --- a/test/libsolidity/SolidityNatspecJSON.cpp +++ b/test/libsolidity/SolidityNatspecJSON.cpp @@ -303,7 +303,11 @@ BOOST_AUTO_TEST_CASE(public_state_variable) "state" : { "details" : "example of dev", - "return" : "returns state" + "return" : "returns state", + "returns" : + { + "_0" : "returns state" + } } } } @@ -2411,6 +2415,58 @@ BOOST_AUTO_TEST_CASE(custom_inheritance) checkNatspec(sourceCode, "B", natspecB, false); } +BOOST_AUTO_TEST_CASE(dev_different_amount_return_parameters) +{ + char const *sourceCode = R"( + interface IThing { + /// @return x a number + /// @return y another number + function value() external view returns (uint128 x, uint128 y); + } + + contract Thing is IThing { + struct Value { + uint128 x; + uint128 y; + } + + Value public override value; + } + )"; + + char const *natspec = R"ABCDEF({ + "methods": + { + "value()": + { + "returns": + { + "x": "a number", + "y": "another number" + } + } + } + })ABCDEF"; + + char const *natspec2 = R"ABCDEF({ + "methods": {}, + "stateVariables": + { + "value": + { + "returns": + { + "x": "a number", + "y": "another number" + } + } + } + })ABCDEF"; + + checkNatspec(sourceCode, "IThing", natspec, false); + checkNatspec(sourceCode, "Thing", natspec2, false); +} + } BOOST_AUTO_TEST_SUITE_END() diff --git a/test/libsolidity/syntaxTests/natspec/invalid/return_param_amount_differs.sol b/test/libsolidity/syntaxTests/natspec/invalid/return_param_amount_differs.sol new file mode 100644 index 000000000..7e40b10b9 --- /dev/null +++ b/test/libsolidity/syntaxTests/natspec/invalid/return_param_amount_differs.sol @@ -0,0 +1,14 @@ +interface IThing { + /// @return x a number + /// @return y another number + function value() external view returns (uint128 x, uint128 y); +} + +contract Thing is IThing { + struct Value { + uint128 x; + uint128 y; + } + + Value public override value; +} diff --git a/test/libsolidity/syntaxTests/natspec/invalid/return_param_amount_differs2.sol b/test/libsolidity/syntaxTests/natspec/invalid/return_param_amount_differs2.sol new file mode 100644 index 000000000..929075be1 --- /dev/null +++ b/test/libsolidity/syntaxTests/natspec/invalid/return_param_amount_differs2.sol @@ -0,0 +1,15 @@ +interface IThing { + /// @param v value to search for + /// @return x a number + /// @return y another number + function value(uint256 v) external view returns (uint128 x, uint128 y); +} + +contract Thing is IThing { + struct Value { + uint128 x; + uint128 y; + } + + mapping(uint256=>Value) public override value; +}