Allow natspec comments on state variables.

Changelog.md

@@ -6,7 +6,7 @@ Language Features:
 Compiler Features:
  * Build system: Update the soljson.js build to emscripten 1.39.15 and boost 1.73.0 and include Z3 for integrated SMTChecker support without the callback mechanism.
  * SMTChecker: Support array ``length``.
-
+ * Add support for natspec comments on state variables.
 
 Bugfixes:
  * Optimizer: Fixed a bug in BlockDeDuplicator.

docs/control-structures.rst

@@ -255,9 +255,9 @@ which only need to be created if there is a dispute.
 
     contract C {
         function createDSalted(bytes32 salt, uint arg) public {
-            /// This complicated expression just tells you how the address
-            /// can be pre-computed. It is just there for illustration.
-            /// You actually only need ``new D{salt: salt}(arg)``.
+            // This complicated expression just tells you how the address
+            // can be pre-computed. It is just there for illustration.
+            // You actually only need ``new D{salt: salt}(arg)``.
             address predictedAddress = address(uint(keccak256(abi.encodePacked(
                 byte(0xff),
                 address(this),

docs/natspec-format.rst

@@ -85,10 +85,10 @@ Tag
 =========== =============================================================================== =============================
 ``@title``  A title that should describe the contract/interface                             contract, interface
 ``@author`` The name of the author                                                          contract, interface, function
-``@notice`` Explain to an end user what this does                                           contract, interface, function
-``@dev``    Explain to a developer any extra details                                        contract, interface, function
+``@notice`` Explain to an end user what this does                                           contract, interface, function, state variable
+``@dev``    Explain to a developer any extra details                                        contract, interface, function, state variable
 ``@param``  Documents a parameter just like in doxygen (must be followed by parameter name) function
-``@return`` Documents the return variables of a contract's function                         function
+``@return`` Documents the return variables of a contract's function                         function, state variable
 =========== =============================================================================== =============================
 
 If your function returns multiple values, like ``(int quotient, int remainder)``

docs/security-considerations.rst

@@ -63,7 +63,7 @@ complete contract):
 
     // THIS CONTRACT CONTAINS A BUG - DO NOT USE
     contract Fund {
-        /// Mapping of ether shares of the contract.
+        /// @dev Mapping of ether shares of the contract.
         mapping(address => uint) shares;
         /// Withdraw your share.
         function withdraw() public {
@@ -87,7 +87,7 @@ as it uses ``call`` which forwards all remaining gas by default:
 
     // THIS CONTRACT CONTAINS A BUG - DO NOT USE
     contract Fund {
-        /// Mapping of ether shares of the contract.
+        /// @dev Mapping of ether shares of the contract.
         mapping(address => uint) shares;
         /// Withdraw your share.
         function withdraw() public {
@@ -106,7 +106,7 @@ outlined further below:
     pragma solidity >=0.4.11 <0.7.0;
 
     contract Fund {
-        /// Mapping of ether shares of the contract.
+        /// @dev Mapping of ether shares of the contract.
         mapping(address => uint) shares;
         /// Withdraw your share.
         function withdraw() public {

docs/types/reference-types.rst

@@ -415,7 +415,7 @@ Array slices are useful to ABI-decode secondary data passed in function paramete
     pragma solidity >=0.6.0 <0.7.0;
 
     contract Proxy {
-        /// Address of the client contract managed by proxy i.e., this contract
+        /// @dev Address of the client contract managed by proxy i.e., this contract
         address client;
 
         constructor(address _client) public {

libsolidity/analysis/DocStringAnalyser.cpp

@@ -59,9 +59,21 @@ bool DocStringAnalyser::visit(FunctionDefinition const& _function)
 
 bool DocStringAnalyser::visit(VariableDeclaration const& _variable)
 {
-	if (_variable.isStateVariable() && _variable.isPartOfExternalInterface())
-		handleDeclaration(_variable, _variable, _variable.annotation());
-	return true;
+	if (_variable.isStateVariable())
+	{
+		static set<string> const validPublicTags = set<string>{"dev", "notice", "return", "title", "author"};
+		if (_variable.isPublic())
+			parseDocStrings(_variable, _variable.annotation(), validPublicTags, "public state variables");
+		else
+		{
+			parseDocStrings(_variable, _variable.annotation(), validPublicTags, "non-public state variables");
+			if (_variable.annotation().docTags.count("notice") > 0)
+				m_errorReporter.warning(9098_error, _variable.documentation()->location(), "Documentation tag on non-public state variables will be disallowed in 0.7.0. You will need to use the @dev tag explicitly.");
+		}
+		if (_variable.annotation().docTags.count("title") > 0 || _variable.annotation().docTags.count("author") > 0)
+			m_errorReporter.warning(4822_error, _variable.documentation()->location(), "Documentation tag @title and @author is only allowed on contract definitions. It will be disallowed in 0.7.0.");
+	}
+	return false;
 }
 
 bool DocStringAnalyser::visit(ModifierDefinition const& _modifier)
@@ -124,16 +136,6 @@ void DocStringAnalyser::handleCallable(
 	checkParameters(_callable, _node, _annotation);
 }
 
-void DocStringAnalyser::handleDeclaration(
-	Declaration const&,
-	StructurallyDocumented const& _node,
-	StructurallyDocumentedAnnotation& _annotation
-)
-{
-	static set<string> const validTags = set<string>{"author", "dev", "notice", "return", "param"};
-	parseDocStrings(_node, _annotation, validTags, "state variables");
-}
-
 void DocStringAnalyser::parseDocStrings(
 	StructurallyDocumented const& _node,
 	StructurallyDocumentedAnnotation& _annotation,
@@ -161,7 +163,14 @@ void DocStringAnalyser::parseDocStrings(
 			if (docTag.first == "return")
 			{
 				returnTagsVisited++;
-				if (auto* function = dynamic_cast<FunctionDefinition const*>(&_node))
+				if (dynamic_cast<VariableDeclaration const*>(&_node))
+				{
+					if (returnTagsVisited > 1)
+						appendError(
+							_node.documentation()->location(),
+							"Documentation tag \"@" + docTag.first + "\" is only allowed once on state-variables.");
+				}
+				else if (auto* function = dynamic_cast<FunctionDefinition const*>(&_node))
 				{
 					string content = docTag.second.content;
 					string firstWord = content.substr(0, content.find_first_of(" \t"));

libsolidity/ast/AST.h

@@ -882,17 +882,17 @@ class VariableDeclaration: public Declaration, public StructurallyDocumented
 		ASTPointer<ASTString> const& _name,
 		ASTPointer<Expression> _value,
 		Visibility _visibility,
-        ASTPointer<StructuredDocumentation> const& _documentation,
+		ASTPointer<StructuredDocumentation> const _documentation = nullptr,
 		bool _isStateVar = false,
 		bool _isIndexed = false,
 		Mutability _mutability = Mutability::Mutable,
 		ASTPointer<OverrideSpecifier> _overrides = nullptr,
 		Location _referenceLocation = Location::Unspecified
 	):
 		Declaration(_id, _location, _name, _visibility),
-        StructurallyDocumented(_documentation),
-		m_typeName(_type),
-		m_value(_value),
+		StructurallyDocumented(std::move(_documentation)),
+		m_typeName(std::move(_type)),
+		m_value(std::move(_value)),
 		m_isStateVariable(_isStateVar),
 		m_isIndexed(_isIndexed),
 		m_mutability(_mutability),

libsolidity/ast/ASTJsonConverter.cpp

@@ -390,10 +390,9 @@ bool ASTJsonConverter::visit(VariableDeclaration const& _node)
 		make_pair("typeDescriptions", typePointerToJson(_node.annotation().type, true))
 	};
 	if (_node.isStateVariable() && _node.isPublic())
-	{
 		attributes.emplace_back("functionSelector", _node.externalIdentifierHex());
-		attributes.emplace_back("documentation", _node.documentation() ? toJson(*_node.documentation()) : Json::nullValue);
-	}
+	if (_node.isStateVariable() && _node.documentation())
+		attributes.emplace_back("documentation", toJson(*_node.documentation()));
 	if (m_inEvent)
 		attributes.emplace_back("indexed", _node.isIndexed());
 	if (!_node.annotation().baseFunctions.empty())

libsolidity/ast/ASTJsonImporter.cpp

@@ -441,7 +441,7 @@ ASTPointer<VariableDeclaration> ASTJsonImporter::createVariableDeclaration(Json:
 		make_shared<ASTString>(member(_node, "name").asString()),
 		nullOrCast<Expression>(member(_node, "value")),
 		visibility(_node),
-        _node["documentation"].isNull() ? nullptr : createDocumentation(member(_node, "documentation")),
+		_node["documentation"].isNull() ? nullptr : createDocumentation(member(_node, "documentation")),
 		memberAsBool(_node, "stateVariable"),
 		_node.isMember("indexed") ? memberAsBool(_node, "indexed") : false,
 		mutability,

libsolidity/interface/Natspec.cpp

@@ -40,7 +40,7 @@ Json::Value Natspec::userDocumentation(ContractDefinition const& _contractDef)
 	auto constructorDefinition(_contractDef.constructor());
 	if (constructorDefinition)
 	{
-		string value = extractDoc(constructorDefinition->annotation().docTags, "notice");
+		string const value = extractDoc(constructorDefinition->annotation().docTags, "notice");
 		if (!value.empty())
 			// add the constructor, only if we have any documentation to add
 			methods["constructor"] = Json::Value(value);
@@ -64,8 +64,9 @@ Json::Value Natspec::userDocumentation(ContractDefinition const& _contractDef)
 					methods[it.second->externalSignature()] = user;
 				}
 			}
-			if (auto var = dynamic_cast<VariableDeclaration const*>(&it.second->declaration()))
+			else if (auto var = dynamic_cast<VariableDeclaration const*>(&it.second->declaration()))
 			{
+				solAssert(var->isStateVariable() && var->isPublic(), "");
 				string value = extractDoc(var->annotation().docTags, "notice");
 				if (!value.empty())
 				{
@@ -121,22 +122,22 @@ Json::Value Natspec::devDocumentation(ContractDefinition const& _contractDef)
 			if (!method.empty())
 				methods[it.second->externalSignature()] = method;
 		}
-		if (auto var = dynamic_cast<VariableDeclaration const*>(&it.second->declaration()))
-		{
-			Json::Value method(devDocumentation(var->annotation().docTags));
-			if (!method.empty())
-			{
-//				Json::Value jsonReturn = extractReturnParameterDocs(var->annotation().docTags, *var);
+	}
 
-//				if (!jsonReturn.empty())
-//					method["returns"] = jsonReturn;
+	Json::Value stateVariables(Json::objectValue);
+	for (VariableDeclaration const* varDecl: _contractDef.stateVariables())
+	{
+		if (auto devDoc = devDocumentation(varDecl->annotation().docTags); !devDoc.empty())
+			stateVariables[varDecl->name()] = devDoc;
 
-				methods[it.second->externalSignature()] = method;
-			}
-		}
+		solAssert(varDecl->annotation().docTags.count("return") <= 1, "");
+		if (varDecl->annotation().docTags.count("return") == 1)
+			stateVariables[varDecl->name()]["return"] = extractDoc(varDecl->annotation().docTags, "return");
 	}
 
 	doc["methods"] = methods;
+	if (!stateVariables.empty())
+		doc["stateVariables"] = stateVariables;
 
 	return doc;
 }

libsolidity/parsing/Parser.cpp

@@ -686,7 +686,7 @@ ASTPointer<VariableDeclaration> Parser::parseVariableDeclaration(
 	ASTNodeFactory nodeFactory = _lookAheadArrayType ?
 		ASTNodeFactory(*this, _lookAheadArrayType) : ASTNodeFactory(*this);
 	ASTPointer<TypeName> type;
-    ASTPointer<StructuredDocumentation> documentation = parseStructuredDocumentation();
+	ASTPointer<StructuredDocumentation> const documentation = parseStructuredDocumentation();
 	if (_lookAheadArrayType)
 		type = _lookAheadArrayType;
 	else
@@ -696,8 +696,8 @@ ASTPointer<VariableDeclaration> Parser::parseVariableDeclaration(
 			nodeFactory.setEndPositionFromNode(type);
 	}
 
-//	if (!_options.isStateVariable && documentation != nullptr)
-//		fatalParserError("Only state variables can retrieve a docstring.");
+	if (!_options.isStateVariable && documentation != nullptr)
+		parserWarning(2837_error, "Only state variables can have a docstring. This will be disallowed in 0.7.0.");
 
 	if (dynamic_cast<FunctionTypeName*>(type.get()) && _options.isStateVariable && m_scanner->currentToken() == Token::LBrace)
 		fatalParserError(
@@ -813,7 +813,7 @@ ASTPointer<VariableDeclaration> Parser::parseVariableDeclaration(
 		identifier,
 		value,
 		visibility,
-        documentation,
+		documentation,
 		_options.isStateVariable,
 		isIndexed,
 		mutability,
@@ -1579,8 +1579,7 @@ ASTPointer<VariableDeclarationStatement> Parser::parseVariableDeclarationStateme
 						ASTPointer<TypeName>(),
 						name,
 						ASTPointer<Expression>(),
-                        Visibility::Default,
-                        nullptr
+						Visibility::Default
 					);
 				}
 				variables.push_back(var);