NatSpec: Implement default inheritance.

Changelog.md

@@ -5,6 +5,7 @@ Language Features:
 
 Compiler Features:
  * NatSpec: Add fields "kind" and "version" to the JSON output.
+ * NatSpec: Inherit tags from unique base if derived function does not provide any.
  * Commandline Interface: Prevent some incompatible commandline options from being used together.
  * NatSpec: Support NatSpec comments on events.
 

docs/natspec-format.rst

@@ -122,9 +122,11 @@ documentation and you may read more at
 Inheritance Notes
 -----------------
 
-Currently it is undefined whether a contract with a function having no
-NatSpec will inherit the NatSpec of a parent contract/interface for that
-same function.
+Functions without NatSpec will automatically inherit the documentation of their
+base function. Exceptions to this are:
+
+ * When the parameter names are different.
+ * When there is more than one base function.
 
 .. _header-output:
 

libsolidity/analysis/DocStringAnalyser.cpp

@@ -32,6 +32,33 @@ using namespace solidity;
 using namespace solidity::langutil;
 using namespace solidity::frontend;
 
+namespace
+{
+
+void copyMissingTags(StructurallyDocumentedAnnotation& _target, set<CallableDeclaration const*> const& _baseFunctions)
+{
+	if (_baseFunctions.size() != 1)
+		return;
+
+	auto& sourceDoc = dynamic_cast<StructurallyDocumentedAnnotation const&>((*_baseFunctions.begin())->annotation());
+
+	set<string> existingTags;
+
+	for (auto const& iterator: _target.docTags)
+		existingTags.insert(iterator.first);
+
+	for (auto const& [tag, content]: sourceDoc.docTags)
+		if (tag != "inheritdoc" && !existingTags.count(tag))
+			_target.docTags.emplace(tag, content);
+}
+
+bool parameterNamesEqual(CallableDeclaration const& _a, CallableDeclaration const& _b)
+{
+	return boost::range::equal(_a.parameters(), _b.parameters(), [](auto const& pa, auto const& pb) { return pa->name() == pb->name(); });
+}
+
+}
+
 bool DocStringAnalyser::analyseDocStrings(SourceUnit const& _sourceUnit)
 {
 	auto errorWatcher = m_errorReporter.errorWatcher();
@@ -79,6 +106,9 @@ bool DocStringAnalyser::visit(VariableDeclaration const& _variable)
 				"Documentation tag @title and @author is only allowed on contract definitions. "
 				"It will be disallowed in 0.7.0."
 			);
+
+		if (_variable.annotation().docTags.empty())
+			copyMissingTags(_variable.annotation(), _variable.annotation().baseFunctions);
 	}
 	return false;
 }
@@ -143,6 +173,13 @@ void DocStringAnalyser::handleCallable(
 	parseDocStrings(_node, _annotation, validTags, "functions");
 	checkParameters(_callable, _node, _annotation);
 
+	if (
+		_annotation.docTags.empty() &&
+		_callable.annotation().baseFunctions.size() == 1 &&
+		parameterNamesEqual(_callable, **_callable.annotation().baseFunctions.begin())
+	)
+		copyMissingTags(_annotation, _callable.annotation().baseFunctions);
+
 	if (_node.documentation() && _annotation.docTags.count("author") > 0)
 		m_errorReporter.warning(
 			9843_error, _node.documentation()->location(),

libsolidity/interface/CompilerStack.cpp

@@ -307,11 +307,6 @@ bool CompilerStack::analyze()
 			if (source->ast && !syntaxChecker.checkSyntax(*source->ast))
 				noErrors = false;
 
-		DocStringAnalyser docStringAnalyser(m_errorReporter);
-		for (Source const* source: m_sourceOrder)
-			if (source->ast && !docStringAnalyser.analyseDocStrings(*source->ast))
-				noErrors = false;
-
 		m_globalContext = make_shared<GlobalContext>();
 		// We need to keep the same resolver during the whole process.
 		NameAndTypeResolver resolver(*m_globalContext, m_evmVersion, m_errorReporter);
@@ -367,6 +362,11 @@ bool CompilerStack::analyze()
 						if (!contractLevelChecker.check(*contract))
 							noErrors = false;
 
+		DocStringAnalyser docStringAnalyser(m_errorReporter);
+		for (Source const* source: m_sourceOrder)
+			if (source->ast && !docStringAnalyser.analyseDocStrings(*source->ast))
+				noErrors = false;
+
 		// New we run full type checks that go down to the expression level. This
 		// cannot be done earlier, because we need cross-contract types and information
 		// about whether a contract is abstract for the `new` expression.

test/libsolidity/SolidityNatspecJSON.cpp

@@ -1260,6 +1260,263 @@ BOOST_AUTO_TEST_CASE(slash3_slash4)
 	checkNatspec(sourceCode, "test", natspec, true);
 }
 
-BOOST_AUTO_TEST_SUITE_END()
+BOOST_AUTO_TEST_CASE(dev_default_inherit_variable)
+{
+	char const *sourceCode = R"(
+		contract C {
+			/// @notice Hello world
+			/// @dev test
+			function x() virtual external returns (uint) {
+				return 1;
+			}
+		}
+
+		contract D is C {
+			uint public override x;
+		}
+	)";
+
+	char const *natspec = R"ABCDEF({
+		"methods":
+		{
+			"x()":
+			{
+				"details": "test"
+			}
+		}
+	})ABCDEF";
+
+	char const *natspec1 = R"ABCDEF({
+		"methods" : {},
+			"stateVariables" :
+		{
+			"x" :
+			{
+				"details" : "test"
+			}
+		}
+	})ABCDEF";
+
+	checkNatspec(sourceCode, "C", natspec, false);
+	checkNatspec(sourceCode, "D", natspec1, false);
+}
+
+BOOST_AUTO_TEST_CASE(user_default_inherit_variable)
+{
+	char const *sourceCode = R"(
+		contract C {
+			/// @notice Hello world
+			/// @dev test
+			function x() virtual external returns (uint) {
+				return 1;
+			}
+		}
+
+		contract D is C {
+			uint public override x;
+		}
+	)";
+
+	char const *natspec = R"ABCDEF({
+		"methods":
+		{
+			"x()":
+			{
+				"notice": "Hello world"
+			}
+		}
+	})ABCDEF";
+
+	checkNatspec(sourceCode, "C", natspec, true);
+	checkNatspec(sourceCode, "D", natspec, true);
+}
+
+BOOST_AUTO_TEST_CASE(dev_default_inherit)
+{
+	char const *sourceCode = R"(
+		interface ERC20 {
+			/// Transfer ``amount`` from ``msg.sender`` to ``to``.
+			/// Second line.
+			/// @author Programmer
+			/// @dev test
+			/// @param to address to transfer to
+			/// @param amount amount to transfer
+			function transfer(address to, uint amount) external returns (bool);
+		}
+
+		contract Middle is ERC20 {
+			function transfer(address to, uint amount) virtual override external returns (bool)
+			{
+			return false;
+		  }
+		}
+
+		contract Token is Middle {
+			function transfer(address to, uint amount) override external returns (bool)
+			{
+				return false;
+			}
+		}
+	)";
+
+	char const *natspec = R"ABCDEF({
+		"methods":
+		{
+			"transfer(address,uint256)":
+			{
+				"author": "Programmer",
+				"details": "test",
+				"params":
+				{
+					"amount": "amount to transfer",
+					"to": "address to transfer to"
+				}
+			}
+		}
+	})ABCDEF";
+
+	checkNatspec(sourceCode, "ERC20", natspec, false);
+	checkNatspec(sourceCode, "Middle", natspec, false);
+	checkNatspec(sourceCode, "Token", natspec, false);
+}
+
+BOOST_AUTO_TEST_CASE(user_default_inherit)
+{
+	char const *sourceCode = R"(
+		interface ERC20 {
+			/// Transfer ``amount`` from ``msg.sender`` to ``to``.
+			/// Second line.
+			/// @author Programmer
+			/// @dev test
+			/// @param to address to transfer to
+			/// @param amount amount to transfer
+			function transfer(address to, uint amount) external returns (bool);
+		}
+
+		contract Middle is ERC20 {
+			function transfer(address to, uint amount) virtual override external returns (bool)
+			{
+			return false;
+		  }
+		}
+
+		contract Token is Middle {
+			function transfer(address to, uint amount) override external returns (bool)
+			{
+				return false;
+			}
+		}
+	)";
+
+	char const *natspec = R"ABCDEF({
+		"methods":
+		{
+			"transfer(address,uint256)":
+			{
+				"notice": "Transfer ``amount`` from ``msg.sender`` to ``to``. Second line."
+			}
+		}
+	})ABCDEF";
+
+	checkNatspec(sourceCode, "ERC20", natspec, true);
+	checkNatspec(sourceCode, "Middle", natspec, true);
+	checkNatspec(sourceCode, "Token", natspec, true);
+}
+
+BOOST_AUTO_TEST_CASE(dev_inherit_parameter_mismatch)
+{
+	char const *sourceCode = R"(
+		interface ERC20 {
+			/// Transfer ``amount`` from ``msg.sender`` to ``to``.
+			/// @author Programmer
+			/// @dev test
+			/// @param to address to transfer to
+			/// @param amount amount to transfer
+			function transfer(address to, uint amount) external returns (bool);
+		}
+
+		contract Middle is ERC20 {
+			function transfer(address to, uint amount) override virtual external returns (bool) {
+				return false;
+			}
+		}
+
+		contract Token is Middle {
+			function transfer(address too, uint amount) override external returns (bool) {
+				return false;
+			}
+		}
+	)";
+
+	char const *natspec = R"ABCDEF({
+		"methods":
+		{
+			"transfer(address,uint256)":
+			{
+				"author": "Programmer",
+				"details": "test",
+				"params":
+				{
+					"amount": "amount to transfer",
+					"to": "address to transfer to"
+				}
+			}
+		}
+	})ABCDEF";
+
+	char const *natspec2 = R"ABCDEF({
+		"methods": { }
+	})ABCDEF";
+
+	checkNatspec(sourceCode, "ERC20", natspec, false);
+	checkNatspec(sourceCode, "Middle", natspec, false);
+	checkNatspec(sourceCode, "Token", natspec2, false);
+}
+
+BOOST_AUTO_TEST_CASE(user_inherit_parameter_mismatch)
+{
+	char const *sourceCode = R"(
+		interface ERC20 {
+			/// Transfer ``amount`` from ``msg.sender`` to ``to``.
+			/// @author Programmer
+			/// @dev test
+			/// @param to address to transfer to
+			/// @param amount amount to transfer
+			function transfer(address to, uint amount) external returns (bool);
+		}
+
+		contract Middle is ERC20 {
+			function transfer(address to, uint amount) override virtual external returns (bool) {
+				return false;
+			}
+		}
+
+		contract Token is Middle {
+			function transfer(address too, uint amount) override external returns (bool) {
+				return false;
+			}
+		}
+	)";
+
+	char const *natspec = R"ABCDEF({
+		"methods":
+		{
+			"transfer(address,uint256)":
+			{
+				"notice": "Transfer ``amount`` from ``msg.sender`` to ``to``."
+			}
+		}
+	})ABCDEF";
+
+	char const *natspec2 = R"ABCDEF({
+		"methods": { }
+	})ABCDEF";
+
+	checkNatspec(sourceCode, "ERC20", natspec, true);
+	checkNatspec(sourceCode, "Middle", natspec, true);
+	checkNatspec(sourceCode, "Token", natspec2, true);
+}
 
 }
+
+BOOST_AUTO_TEST_SUITE_END()