Merge pull request #9989 from ethereum/issue-9947

Natspec: Fix internal error when different return name was inherited

Changelog.md

@@ -31,6 +31,7 @@ Bugfixes:
  * Command Line Interface: Fix write error when the directory passed to ``--output-dir`` ends with a slash.
  * SMTChecker: Fix CHC false positives when branches are used inside modifiers.
  * 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)

libsolidity/analysis/DocStringAnalyser.cpp

@@ -36,21 +36,52 @@ using namespace solidity::frontend;
 namespace
 {
 
-void copyMissingTags(StructurallyDocumentedAnnotation& _target, set<CallableDeclaration const*> const& _baseFunctions)
+void copyMissingTags(set<CallableDeclaration const*> 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<StructurallyDocumentedAnnotation const&>((*_baseFunctions.begin())->annotation());
 
-	set<string> 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)
+		size_t n = 0;
-		existingTags.insert(iterator.first);
+		// 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<CallableDeclaration const*> 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(

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()

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
 	}
 }
 // ----