Merge pull request #9267 from ethereum/issue-8911-split

NatSpec: Implement default inheritance.

Changelog.md

@@ -6,6 +6,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

@@ -42,10 +42,6 @@ The following example shows a contract and a function using all available tags.
 
 .. note::
 
-  NatSpec currently does NOT apply to public state variables (see
-  `solidity#3418 <https://github.com/ethereum/solidity/issues/3418>`__),
-  even if they are declared public and therefore do affect the ABI.
-
   The Solidity compiler only interprets tags if they are external or
   public. You are welcome to use similar comments for your internal and
   private functions, but those will not be parsed.
@@ -60,7 +56,6 @@ The following example shows a contract and a function using all available tags.
     /// @notice You can use this contract for only the most basic simulation
     /// @dev All function calls are currently implemented without side effects
     contract Tree {
-        /// @author Mary A. Botanist
         /// @notice Calculate tree age in years, rounded up, for live trees
         /// @dev The Alexandr N. Tetearing algorithm could increase precision
         /// @param rings The number of rings from dendrochronological sample
@@ -84,7 +79,7 @@ in the same way as if it were tagged with ``@notice``.
 Tag                                                                                         Context
 =========== =============================================================================== =============================
 ``@title``  A title that should describe the contract/interface                             contract, interface
-``@author`` The name of the author                                                          contract, interface, function
+``@author`` The name of the author                                                          contract, interface
 ``@notice`` Explain to an end user what this does                                           contract, interface, function, public state variable, event
 ``@dev``    Explain to a developer any extra details                                        contract, interface, function, state variable, event
 ``@param``  Documents a parameter just like in doxygen (must be followed by parameter name) function, event
@@ -127,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:
 
@@ -193,7 +190,6 @@ file should also be produced and should look like this:
       {
         "age(uint256)" :
         {
-          "author" : "Mary A. Botanist",
           "details" : "The Alexandr N. Tetearing algorithm could increase precision",
           "params" :
           {

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;
 }
@@ -142,6 +172,20 @@ void DocStringAnalyser::handleCallable(
 	static set<string> const validTags = set<string>{"author", "dev", "notice", "return", "param"};
 	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(),
+			"Documentation tag @author is only allowed on contract definitions. "
+			"It will be disallowed in 0.7.0."
+		);
 }
 
 void DocStringAnalyser::parseDocStrings(

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

test/libsolidity/syntaxTests/natspec/docstring_author_function.sol

@@ -0,0 +1,6 @@
+contract C {
+    /// @author author
+    function iHaveAuthor() public pure {}
+}
+// ----
+// Warning 9843: (17-35): Documentation tag @author is only allowed on contract definitions. It will be disallowed in 0.7.0.