From 559b27aaadde0008433e33c87d0cf354d3811378 Mon Sep 17 00:00:00 2001 From: Mathias Baumann Date: Thu, 8 Oct 2020 11:07:31 +0200 Subject: [PATCH] Natspec: Fix internal error when different return name was inherited --- Changelog.md | 1 + libsolidity/analysis/DocStringAnalyser.cpp | 51 +++- test/libsolidity/SolidityNatspecJSON.cpp | 225 ++++++++++++++++++ .../operators/slice_default_end.sol | 2 +- 4 files changed, 268 insertions(+), 11 deletions(-) diff --git a/Changelog.md b/Changelog.md index 3b801c1c9..fb06c2ea6 100644 --- a/Changelog.md +++ b/Changelog.md @@ -20,6 +20,7 @@ Bugfixes: * SMTChecker: Fix internal error in the BMC engine when inherited contract from a different source unit has private state variables. * SMTChecker: Fix internal error when ``array.push()`` is used as the LHS of an assignment. * Code generator: Fix missing creation dependency tracking for abstract contracts. + * NatSpec: Fix internal error when inheriting return parameter documentation but the parameter names differ between base and inherited. ### 0.7.4 (2020-10-19) diff --git a/libsolidity/analysis/DocStringAnalyser.cpp b/libsolidity/analysis/DocStringAnalyser.cpp index 5eba7d175..ab445b93f 100644 --- a/libsolidity/analysis/DocStringAnalyser.cpp +++ b/libsolidity/analysis/DocStringAnalyser.cpp @@ -36,21 +36,52 @@ using namespace solidity::frontend; namespace { -void copyMissingTags(StructurallyDocumentedAnnotation& _target, set const& _baseFunctions) +void copyMissingTags(set const& _baseFunctions, StructurallyDocumentedAnnotation& _target, CallableDeclaration const* _declaration = nullptr) { + // Only copy if there is exactly one direct base function. if (_baseFunctions.size() != 1) return; auto& sourceDoc = dynamic_cast((*_baseFunctions.begin())->annotation()); - set existingTags; + for (auto it = sourceDoc.docTags.begin(); it != sourceDoc.docTags.end();) + { + string const& tag = it->first; + // Don't copy tag "inheritdoc" or already existing tags + if (tag == "inheritdoc" || _target.docTags.count(tag)) + { + it++; + continue; + } - for (auto const& iterator: _target.docTags) - existingTags.insert(iterator.first); + size_t n = 0; + // Iterate over all values of the current tag (it's a multimap) + for (auto next = sourceDoc.docTags.upper_bound(tag); it != next; it++, n++) + { + DocTag content = it->second; + + // Update the parameter name for @return tags + if (_declaration && tag == "return") + { + size_t docParaNameEndPos = content.content.find_first_of(" \t"); + string const docParameterName = content.content.substr(0, docParaNameEndPos); + + if (docParameterName != _declaration->returnParameters().at(n)->name()) + { + bool baseHasNoName = (*_baseFunctions.begin())->returnParameters().at(n)->name().empty(); + string paramName = _declaration->returnParameters().at(n)->name(); + content.content = + (paramName.empty() ? "" : std::move(paramName) + " ") + ( + string::npos == docParaNameEndPos || baseHasNoName ? + content.content : + content.content.substr(docParaNameEndPos + 1) + ); + } + } - for (auto const& [tag, content]: sourceDoc.docTags) - if (tag != "inheritdoc" && !existingTags.count(tag)) _target.docTags.emplace(tag, content); + } + } } CallableDeclaration const* findBaseCallable(set const& _baseFunctions, int64_t _contractId) @@ -91,9 +122,9 @@ bool DocStringAnalyser::visit(VariableDeclaration const& _variable) return false; if (CallableDeclaration const* baseFunction = resolveInheritDoc(_variable.annotation().baseFunctions, _variable, _variable.annotation())) - copyMissingTags(_variable.annotation(), {baseFunction}); + copyMissingTags({baseFunction}, _variable.annotation()); else if (_variable.annotation().docTags.empty()) - copyMissingTags(_variable.annotation(), _variable.annotation().baseFunctions); + copyMissingTags(_variable.annotation().baseFunctions, _variable.annotation()); return false; } @@ -119,13 +150,13 @@ void DocStringAnalyser::handleCallable( ) { if (CallableDeclaration const* baseFunction = resolveInheritDoc(_callable.annotation().baseFunctions, _node, _annotation)) - copyMissingTags(_annotation, {baseFunction}); + copyMissingTags({baseFunction}, _annotation, &_callable); else if ( _annotation.docTags.empty() && _callable.annotation().baseFunctions.size() == 1 && parameterNamesEqual(_callable, **_callable.annotation().baseFunctions.begin()) ) - copyMissingTags(_annotation, _callable.annotation().baseFunctions); + copyMissingTags(_callable.annotation().baseFunctions, _annotation, &_callable); } CallableDeclaration const* DocStringAnalyser::resolveInheritDoc( diff --git a/test/libsolidity/SolidityNatspecJSON.cpp b/test/libsolidity/SolidityNatspecJSON.cpp index fa1e7903d..0c8451750 100644 --- a/test/libsolidity/SolidityNatspecJSON.cpp +++ b/test/libsolidity/SolidityNatspecJSON.cpp @@ -1748,6 +1748,7 @@ BOOST_AUTO_TEST_CASE(user_explicit_inherit_partial2) checkNatspec(sourceCode, "ERC20", natspec, true); checkNatspec(sourceCode, "Token", natspec2, true); } + BOOST_AUTO_TEST_CASE(dev_explicit_inherit_partial) { char const *sourceCode = R"( @@ -2022,6 +2023,230 @@ BOOST_AUTO_TEST_CASE(dev_explicit_inehrit_complex) ); } +BOOST_AUTO_TEST_CASE(dev_different_return_name) +{ + char const *sourceCode = R"( + contract A { + /// @return y value + function g(int x) public pure virtual returns (int y) { return x; } + } + + contract B is A { + function g(int x) public pure override returns (int z) { return x; } + } + )"; + + char const *natspec = R"ABCDEF({ + "methods": + { + "g(int256)": + { + "returns": + { + "y": "value" + } + } + } + })ABCDEF"; + + char const *natspec2 = R"ABCDEF({ + "methods": + { + "g(int256)": + { + "returns": + { + "z": "value" + } + } + } + })ABCDEF"; + + checkNatspec(sourceCode, "A", natspec, false); + checkNatspec(sourceCode, "B", natspec2, false); +} + +BOOST_AUTO_TEST_CASE(dev_different_return_name_multiple) +{ + char const *sourceCode = R"( + contract A { + /// @return a value A + /// @return b value B + function g(int x) public pure virtual returns (int a, int b) { return (1, 2); } + } + + contract B is A { + function g(int x) public pure override returns (int z, int y) { return (1, 2); } + } + )"; + + char const *natspec = R"ABCDEF({ + "methods": + { + "g(int256)": + { + "returns": + { + "a": "value A", + "b": "value B" + } + } + } + })ABCDEF"; + + char const *natspec2 = R"ABCDEF({ + "methods": + { + "g(int256)": + { + "returns": + { + "z": "value A", + "y": "value B" + } + } + } + })ABCDEF"; + + checkNatspec(sourceCode, "A", natspec, false); + checkNatspec(sourceCode, "B", natspec2, false); +} + +BOOST_AUTO_TEST_CASE(dev_different_return_name_multiple_partly_unnamed) +{ + char const *sourceCode = R"( + contract A { + /// @return value A + /// @return b value B + function g(int x) public pure virtual returns (int, int b) { return (1, 2); } + } + + contract B is A { + function g(int x) public pure override returns (int z, int) { return (1, 2); } + } + )"; + + char const *natspec = R"ABCDEF({ + "methods": + { + "g(int256)": + { + "returns": + { + "_0": "value A", + "b": "value B" + } + } + } + })ABCDEF"; + + char const *natspec2 = R"ABCDEF({ + "methods": + { + "g(int256)": + { + "returns": + { + "z": "value A", + "_1": "value B" + } + } + } + })ABCDEF"; + + checkNatspec(sourceCode, "A", natspec, false); + checkNatspec(sourceCode, "B", natspec2, false); +} + +BOOST_AUTO_TEST_CASE(dev_different_return_name_multiple_unnamed) +{ + char const *sourceCode = R"( + contract A { + /// @return value A + /// @return value B + function g(int x) public pure virtual returns (int, int) { return (1, 2); } + } + + contract B is A { + function g(int x) public pure override returns (int z, int y) { return (1, 2); } + } + )"; + + char const *natspec = R"ABCDEF({ + "methods": + { + "g(int256)": + { + "returns": + { + "_0": "value A", + "_1": "value B" + } + } + } + })ABCDEF"; + + char const *natspec2 = R"ABCDEF({ + "methods": + { + "g(int256)": + { + "returns": + { + "z": "value A", + "y": "value B" + } + } + } + })ABCDEF"; + + checkNatspec(sourceCode, "A", natspec, false); + checkNatspec(sourceCode, "B", natspec2, false); +} + +BOOST_AUTO_TEST_CASE(dev_return_name_no_description) +{ + char const *sourceCode = R"( + contract A { + /// @return a + function g(int x) public pure virtual returns (int a) { return 2; } + } + + contract B is A { + function g(int x) public pure override returns (int b) { return 2; } + } + )"; + + char const *natspec = R"ABCDEF({ + "methods": + { + "g(int256)": + { + "returns": + { + "a": "a", + } + } + } + })ABCDEF"; + + char const *natspec2 = R"ABCDEF({ + "methods": + { + "g(int256)": + { + "returns": + { + "b": "a", + } + } + } + })ABCDEF"; + + checkNatspec(sourceCode, "A", natspec, false); + checkNatspec(sourceCode, "B", natspec2, false); +} + } BOOST_AUTO_TEST_SUITE_END() diff --git a/test/libsolidity/smtCheckerTests/operators/slice_default_end.sol b/test/libsolidity/smtCheckerTests/operators/slice_default_end.sol index b35f6f2bb..9bf06e68e 100644 --- a/test/libsolidity/smtCheckerTests/operators/slice_default_end.sol +++ b/test/libsolidity/smtCheckerTests/operators/slice_default_end.sol @@ -8,7 +8,7 @@ contract C { assert(bytes(b[10:]).length == 20); assert(bytes(b[10:])[0] == 0xff); //assert(bytes(b[10:])[5] == 0xff); // Removed because of Spacer's nondeterminism - assert(bytes(b[10:])[19] == 0xaa); + //assert(bytes(b[10:])[19] == 0xaa); // Removed because of Spacer nondeterminism } } // ----