Allow NatSpec comments for state variables by aarlt · Pull Request #8532 · argotorg/solidity

aarlt · 2020-03-24T22:47:22Z

Fixes #3418.
replaces #8399

chriseth · 2020-03-25T16:53:37Z

Please squash.

libsolidity/ast/ASTJsonConverter.cpp

libsolidity/interface/Natspec.cpp

libsolidity/parsing/Parser.cpp

test/libsolidity/SolidityNatspecJSON.cpp

libsolidity/analysis/DocStringAnalyser.cpp

aarlt · 2020-03-27T16:20:40Z

@chriseth i'm not sure whether it is good to forbid notice on non-public state variables. it is used a lot by contract developers. Some examples:

colonyNetwork @ contracts/ReputationMiningCycle.sol

contract ReputationMiningCycle is ReputationMiningCycleStorage, PatriciaTreeProofs, DSMath {
  /// @notice Minimum reputation mining stake in CLNY
  uint256 constant MIN_STAKE = 2000 * WAD;

  /// @notice Size of mining window in seconds
  uint256 constant MINING_WINDOW_SIZE = 60 * 60 * 24; // 24 hours
  ...
}

OpenZeppelin @ contracts/drafts/ERC20Migrator.sol:

contract ERC20Migrator {
    using SafeERC20 for IERC20;

    /// Address of the old token contract
    IERC20 private _legacyToken;

    /// Address of the new token contract
    ERC20Mintable private _newToken;
    ...
}

Solidity Documentation

    contract Fund {
        /// Mapping of ether shares of the contract.
        mapping(address => uint) shares;
        /// Withdraw your share.
        function withdraw() public {
            (bool success,) = msg.sender.call{value: shares[msg.sender]}("");
            if (success)
                shares[msg.sender] = 0;
        }
    }

chriseth · 2020-04-01T09:24:20Z

@aarlt @notice is useless on non-public functions. We can add a warning now and disallow it in 0.7.0.

chriseth · 2020-05-12T16:01:54Z

libsolidity/analysis/DocStringAnalyser.cpp

+{
+	if (_variable.isStateVariable())
+	{
+		static set<string> const validPublicTags = set<string>{"dev", "notice", "title", "author"};


Is author not only for contract?

Also title?

Yes, they are normally only allowed on contract. The thing is that some contract authors in the past put this "accidentally" in front of a state variable. It was ok in the past, because there was no check on state variables implemented. Looks like that they never noticed that title and author was never set. However, I added the checks here to inform the authors that this will not be allowed anymore.

chriseth · 2020-05-12T16:02:47Z

libsolidity/analysis/DocStringAnalyser.cpp

+		else
+		{
+			parseDocStrings(_variable, _variable.annotation(), validPublicTags, "non-public state variables");
+			if (_variable.annotation().docTags.find("notice") != _variable.annotation().docTags.end())


Please use contains or count

chriseth · 2020-05-12T16:03:03Z

libsolidity/analysis/DocStringAnalyser.cpp

+			if (_variable.annotation().docTags.find("notice") != _variable.annotation().docTags.end())
+				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.find("title") != _variable.annotation().docTags.end() ||


chriseth · 2020-05-12T16:03:46Z

libsolidity/parsing/Parser.cpp

 	}

+	if (!_options.isStateVariable && documentation != nullptr)
+		parserWarning(2837_error, "Only state variables can retrieve a docstring. This will be disallowed in 0.7.0.");


Suggested change

parserWarning(2837_error, "Only state variables can retrieve a docstring. This will be disallowed in 0.7.0.");

parserWarning(2837_error, "Only state variables can have a docstring. This will be disallowed in 0.7.0.");

chriseth · 2020-05-12T16:04:42Z

test/libsolidity/SolidityNatspecJSON.cpp

+	char const* sourceCode = R"(
+		contract test {
+			/// @notice example of notice
+			/// @dev example of dev


Can you check if @returns works here?

Right now only @dev and @notice is allowed, but it could also make sense to allow @return here too.

For contract

contract test { /// @notice example of notice /// @dev example of dev /// @return returns something uint public state; }

devdoc will now look like this:

{ "methods": {}, "stateVariables": { "state": { "details": "example of dev", "return": "returns something" } } }

multiple return tags on a state-variable will result in a parsing error.

chriseth · 2020-05-14T13:34:43Z

Needs rebase.

leonardoalt · 2020-05-18T09:39:23Z

Needs rebase again. solc-js tests might be fixed then

chriseth · 2020-05-19T15:02:26Z

Needs rebase.

chriseth · 2020-05-19T15:03:08Z

docs/natspec-format.rst

 ``@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


Suggested change

``@notice`` Explain to an end user what this does contract, interface, function, state variable

``@notice`` Explain to an end user what this does contract, interface, function, public state variable

chriseth · 2020-05-19T15:03:21Z

docs/natspec-format.rst

+``@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


Suggested change

``@return`` Documents the return variables of a contract's function function, state variable

``@return`` Documents the return variables of a contract's function function, public state variable

Thats true, @return should only be allowed on public state-variables.

I added now a check: @return is now only allowed on public state-variables. I added also a test for this.

chriseth · 2020-05-19T15:05:07Z

libsolidity/analysis/DocStringAnalyser.cpp

+					if (returnTagsVisited > 1)
+						appendError(
+							_node.documentation()->location(),
+							"Documentation tag \"@" + docTag.first + "\" is only allowed once on state-variables.");


Suggested change

"Documentation tag \"@" + docTag.first + "\" is only allowed once on state-variables.");

"Documentation tag \"@" + docTag.first + "\" is only allowed once on state-variables."

);

chriseth · 2020-05-19T15:05:24Z

libsolidity/analysis/DocStringAnalyser.cpp

+		{
+			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.");


Please wrap this line.

chriseth · 2020-05-19T15:05:31Z

libsolidity/analysis/DocStringAnalyser.cpp

+				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.");


Please wrap this line.

- adds natspec generation for state variables. - exports structured docs for state variables to JSON.

aarlt force-pushed the structured-docs-variables-aarlt branch 4 times, most recently from 9e6d5c4 to 84f27c4 Compare March 25, 2020 00:04

chriseth mentioned this pull request Mar 25, 2020

Allow NatSpec comments for state variables #8399

Closed

6 tasks