Generate proper parser errors for NatSpec documentation

[erak]

Abstract

With #7534, we introduced a (partially) breaking change: the developer documentation of a function must contain a @return tag for every named return parameter and define the parameter name as the first word after the tag. Compiling without --devdoc is still fine, but requesting it, leads to an Internal Compiler Error.

We should generate proper parser errors in the analyzation phase of a NatSpec string.

This will require some changes to the AST, since documentation needs to become an ASTNode such that it can provide source locations and, in a subsequent step, even every tag as an individual ASTNode which we can pinpoint to in case of an error.

Motivation

Consider the following contract in a file called Test.sol

pragma solidity ^0.6.0;

abstract contract C {
    /// @dev Some nice documentation
    /// @return A value
    function test() public returns (uint value);
}

Compiling it with solc Test.sol will succeed. Requesting the developer documentation using solc --devdoc will result in an Internal Compiler Error because /// @return A value does not contain the name of the return parameter as the first word after the tag.

Also, our source upgrade tool (solidity-upgrade), that is currently in development, will profit from source locations, because it would be also able to upgrade documentation then.

Specification

• A minimal solution for the 0.6.0 release could be the generation of a DocStringError without source location. That would at least make the change a consistently breaking one --> Report DocString error on named return paramater mismatch #7837

• The next step could be the introduction of a DocString node to the AST, and the Documented class having a pointer to it (instead of ASTString) --> [natspec] Introduce AST node for structured documentation #7834

• After the AST node was introduced, report source locations at least for every single or multiline comment, spanning over the whole documentation string -> [parser] Source locations for structured documentation errors #8221

• In order to report errors and their source location correctly, an additional AST node for every tag is needed.

[Marenz]

I see a PR from you @erak so it seems you're working on this. Assigning you

[erak]

I think we need to discuss the last point: "In order to report errors and their source location correctly, an additional AST node for every tag is needed."

I did some hacking on that and realized a few things that might need more work than initially thought:

• The DocStringParser would need to create nodes, but doesn't have the full documentation string.
  Comment literals ///, /* */ etc. are filtered by the scanner before and thus the DocStringParser cannot properly compute the offset from the actual tag to the beginning of the documentation string.
• The above would probably require proper tokenization of documentation strings in order to report the correct, "inner" source location of tags.
• Both would need a solution which needs a good chunk of code to be rewritten (enhance scanner and parser and throw away DocStringParser)

So I would propose to post-pone the last step and at least report the location of the full documentation string in which the error occured. This will be done with: #8221.

Does anyone have further thoughts or opinions?

[chriseth]

You are right, this sounds rather difficult with the current architecture.