diff --git a/Changelog.md b/Changelog.md index 92018a771..4723b1039 100644 --- a/Changelog.md +++ b/Changelog.md @@ -12,6 +12,7 @@ Compiler Features: Bugfixes: * Control Flow Graph: Perform proper virtual lookup for modifiers for uninitialized variable and unreachable code analysis. * Immutables: Fix wrong error when the constructor of a base contract uses ``return`` and the parent contract contains immutable variables. + * Natspec: Fix ICE when overriding a struct getter with a Natspec-documented return value and the name in the struct is different. * TypeChecker: Fix ICE when a constant variable declaration forward references a struct. diff --git a/libsolidity/analysis/DocStringAnalyser.cpp b/libsolidity/analysis/DocStringAnalyser.cpp index 018c10f58..5cff34f2a 100644 --- a/libsolidity/analysis/DocStringAnalyser.cpp +++ b/libsolidity/analysis/DocStringAnalyser.cpp @@ -25,6 +25,7 @@ #include #include +#include #include #include @@ -37,7 +38,7 @@ using namespace solidity::frontend; namespace { -void copyMissingTags(set const& _baseFunctions, StructurallyDocumentedAnnotation& _target, CallableDeclaration const* _declaration = nullptr) +void copyMissingTags(set const& _baseFunctions, StructurallyDocumentedAnnotation& _target, FunctionType const* _functionType = nullptr) { // Only copy if there is exactly one direct base function. if (_baseFunctions.size() != 1) @@ -45,12 +46,6 @@ void copyMissingTags(set const& _baseFunctions, Stru CallableDeclaration const& baseFunction = **_baseFunctions.begin(); - auto hasReturnParameter = [](CallableDeclaration const& declaration, size_t _n) - { - return declaration.returnParameterList() && - declaration.returnParameters().size() > _n; - }; - auto& sourceDoc = dynamic_cast(baseFunction.annotation()); for (auto it = sourceDoc.docTags.begin(); it != sourceDoc.docTags.end();) @@ -70,21 +65,22 @@ void copyMissingTags(set const& _baseFunctions, Stru DocTag content = it->second; // Update the parameter name for @return tags - if (_declaration && tag == "return") + if (_functionType && tag == "return") { size_t docParaNameEndPos = content.content.find_first_of(" \t"); string const docParameterName = content.content.substr(0, docParaNameEndPos); if ( - hasReturnParameter(*_declaration, n) && - docParameterName != _declaration->returnParameters().at(n)->name() + _functionType->returnParameterNames().size() > n && + docParameterName != _functionType->returnParameterNames().at(n) ) { bool baseHasNoName = - hasReturnParameter(baseFunction, n) && + baseFunction.returnParameterList() && + baseFunction.returnParameters().size() > n && baseFunction.returnParameters().at(n)->name().empty(); - string paramName = _declaration->returnParameters().at(n)->name(); + string paramName = _functionType->returnParameterNames().at(n); content.content = (paramName.empty() ? "" : std::move(paramName) + " ") + ( string::npos == docParaNameEndPos || baseHasNoName ? @@ -127,7 +123,7 @@ bool DocStringAnalyser::analyseDocStrings(SourceUnit const& _sourceUnit) bool DocStringAnalyser::visit(FunctionDefinition const& _function) { if (!_function.isConstructor()) - handleCallable(_function, _function, _function.annotation()); + handleCallable(_function, _function, _function.annotation(), TypeProvider::function(_function)); return true; } @@ -136,10 +132,12 @@ bool DocStringAnalyser::visit(VariableDeclaration const& _variable) if (!_variable.isStateVariable() && !_variable.isFileLevelVariable()) return false; + auto const* getterType = TypeProvider::function(_variable); + if (CallableDeclaration const* baseFunction = resolveInheritDoc(_variable.annotation().baseFunctions, _variable, _variable.annotation())) - copyMissingTags({baseFunction}, _variable.annotation()); + copyMissingTags({baseFunction}, _variable.annotation(), getterType); else if (_variable.annotation().docTags.empty()) - copyMissingTags(_variable.annotation().baseFunctions, _variable.annotation()); + copyMissingTags(_variable.annotation().baseFunctions, _variable.annotation(), getterType); return false; } @@ -168,17 +166,18 @@ bool DocStringAnalyser::visit(ErrorDefinition const& _error) void DocStringAnalyser::handleCallable( CallableDeclaration const& _callable, StructurallyDocumented const& _node, - StructurallyDocumentedAnnotation& _annotation + StructurallyDocumentedAnnotation& _annotation, + FunctionType const* _functionType ) { if (CallableDeclaration const* baseFunction = resolveInheritDoc(_callable.annotation().baseFunctions, _node, _annotation)) - copyMissingTags({baseFunction}, _annotation, &_callable); + copyMissingTags({baseFunction}, _annotation, _functionType); else if ( _annotation.docTags.empty() && _callable.annotation().baseFunctions.size() == 1 && parameterNamesEqual(_callable, **_callable.annotation().baseFunctions.begin()) ) - copyMissingTags(_callable.annotation().baseFunctions, _annotation, &_callable); + copyMissingTags(_callable.annotation().baseFunctions, _annotation, _functionType); } CallableDeclaration const* DocStringAnalyser::resolveInheritDoc( diff --git a/libsolidity/analysis/DocStringAnalyser.h b/libsolidity/analysis/DocStringAnalyser.h index 1fa076359..d9cd6f09d 100644 --- a/libsolidity/analysis/DocStringAnalyser.h +++ b/libsolidity/analysis/DocStringAnalyser.h @@ -54,7 +54,8 @@ private: void handleCallable( CallableDeclaration const& _callable, StructurallyDocumented const& _node, - StructurallyDocumentedAnnotation& _annotation + StructurallyDocumentedAnnotation& _annotation, + FunctionType const* _functionType = nullptr ); langutil::ErrorReporter& m_errorReporter; diff --git a/test/libsolidity/SolidityNatspecJSON.cpp b/test/libsolidity/SolidityNatspecJSON.cpp index 6ee55d586..af04d0795 100644 --- a/test/libsolidity/SolidityNatspecJSON.cpp +++ b/test/libsolidity/SolidityNatspecJSON.cpp @@ -2482,7 +2482,7 @@ BOOST_AUTO_TEST_CASE(custom_inheritance) checkNatspec(sourceCode, "B", natspecB, false); } -BOOST_AUTO_TEST_CASE(dev_different_amount_return_parameters) +BOOST_AUTO_TEST_CASE(dev_struct_getter_override) { char const *sourceCode = R"( interface IThing { @@ -2534,6 +2534,58 @@ BOOST_AUTO_TEST_CASE(dev_different_amount_return_parameters) checkNatspec(sourceCode, "Thing", natspec2, false); } +BOOST_AUTO_TEST_CASE(dev_struct_getter_override_different_return_parameter_names) +{ + 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 a; + uint128 b; + } + + 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": + { + "a": "a number", + "b": "another number" + } + } + } + })ABCDEF"; + + checkNatspec(sourceCode, "IThing", natspec, false); + checkNatspec(sourceCode, "Thing", natspec2, false); +} + } BOOST_AUTO_TEST_SUITE_END()