Support the documentation of global variables

[barakman]

Following this feature-request, extend the solidity-docgen package to support the documentation of global variables, which are implicitly added by the compiler as getter functions in the contract.

This should allow users to get the natspec-documentation of their global variables added into the solidity-docgen output by declaring it appropriately in the input template (hbs) file.

For example:

{{#if ownVariables}}
# Variables:
{{#ownVariables}}
- [`{{type}} {{name}}`](#{{anchor}})
{{/ownVariables}}
{{/if}}

{{#ownVariables}}
# Variable `{{type}} {{name}}` {#{{anchor~}} }
{{#if natspec.devdoc}}{{natspec.devdoc}}{{else}}No description{{/if}}
{{/ownVariables}}

Note that this PR is incomplete, since the devdoc of a global variable is always undefined.

For this reason, I also had to override function SolidityContractItem.natspec() in class SolidityVariable with a slightly different implementation (since this.astNode.documentation === undefined).

Thanks

[barakman]

From the official documentation:

NatSpec currently does NOT apply to public state variables (see solidity#3418), even if they are declared public and therefore do affect the ABI.

[barakman]

It occurred to me that you might actually want to merge this PR, with function SolidityVariable.natspec doing:

throw new Error("NatSpec currently does NOT apply to public state variables (see https://github.com/ethereum/solidity/issues/3418)");

So I'm reopening it and leaving for you to decide.

Thanks

[frangio]

Sounds like a good idea!

I'm thinking whether the behavior should be to throw an exception or to return empty natspec and only print a warning.

[frangio]

SolidityContractItem needs astNode to be the definition of either a function, event, or modifier. Although technically public variables define functions, the AST node doesn't have the same information that a function definition does. In particular, it doesn't have the function arguments.

For now please implement the Linkable interface directly, instead of extending SolidityContractItem (whose purpose is to implement Linkable based on the common structure of some of the AST nodes.

[barakman]

@frangio:

First of all, "doesn't have the function arguments" isn't entirely true.
Consider the following example:

contract Contract1 {
    uint[42]                          public x1; // function x1(uint)
    mapping (address => uint)         public x2; // function x2(address)
    mapping (address => uint[42])     public x3; // function x3(address, uint)
    mapping (address => uint)[42]     public x4; // function x4(uint, address)
    mapping (address => uint[42])[84] public x5; // function x5(uint, address, uint)
}

contract Contract2 {
    uint public x1;
    uint public x2;
    uint public x3;
    uint public x4;
    uint public x5;

    constructor() public {
        uint u = 1;
        address a = address(1);
        Contract1 contract1 = new Contract1();
        x1 = contract1.x1(u);
        x2 = contract1.x2(a);
        x3 = contract1.x3(a, u);
        x4 = contract1.x4(u, a);
        x5 = contract1.x5(u, a, u);
    }
}

I initially had an idea, which was indeed based on the notion that compiler-generated getter functions take no arguments:

• Your tool will provide just the ability to list the variables (which is pretty much what I had already completed in this PR)
• In the input template file, one could simply do something like:

{{#ownVariables}}
# Function `{{name}}() → {{type}}`
{{/ownVariables}}

But then I realized that it wouldn't do the job right for all types of variables (in particularly for arrays and mappings).

Second, I don't quite understand the gain in not extending SolidityContractItem, since functions name, fullName and anchor (which relies on signature which we override) should actually be implemented exactly the same.
It is true that the args function requires AST information which is not available for variables, but we can simply override it in the same way that we override the natspec function (though the latter will (hopefully) be overridden temporarily, while the former will be overridden permanently).
This also addresses the suggestion that you've made about the signature function.
I believe that this function is actually very much relevant for variables.
First, because on the philosophical aspect, a variable-declaration can be considered a signature just as much as a function-declaration can.
Second, because on the technical aspect, it is used by the anchor function, which I think we should support for variables.

Please let me know what you think.

Thanks

[frangio]

First of all, "doesn't have the function arguments" isn't entirely true.

What I mean is that the AST doesn't have the information about the function arguments.

I completely agree that it is relevant information. But since the AST doesn't make it available, we would have to generate it ourselves, reproducing the same logic that the Solidity compiler uses. I would prefer not to do that right now, and for the moment only expose the list of variables and their type and visibility.

Note that this PR as it is now doesn't implement the logic for figuring out what are the arguments for a generated getter. There's some tricky logic there, so let's leave that for later on, and for now focus on make state variable information available.

[frangio]

Second, because on the technical aspect, it is used by the anchor function, which I think we should support for variables.

We should support the anchor function, that's why I suggested to implement the Linkable interface directly, without relying on SolidityContractItem that requires all of the getter information we don't yet have.

[barakman]

@frangio:
OK, done, but I'm a little confused about whether or not we want to expose function signature...?
And if not, then (for the exact same reason) why should we expose function natspec?
Thanks

[frangio]

If you don't mind I will commit on top of this branch to show what I mean regarding signature and anchor.

[barakman]

@frangio:
Sure. Please let me know if anything is required on my side.
Thanks.

[frangio]

Please remove this function, for now we will not expose the signature of the functions generated by variables. (See the comment above.)

[barakman]

I believe that this function is actually very much relevant for variables.
First, because on the philosophical aspect, a variable-declaration can be considered a signature just as much as a function-declaration can.
Second, because on the technical aspect, it is used by the anchor function, which I think we should support for variables.

[frangio]

I've finished implementing this. Thank you!

[barakman]

@frangio:
Thanks!
Since that comment kinda "got lost" here somewhere along the way, you might want to emphasize (here or elsewhere) that the NatSpec documentation of global variables is not supported by the compiler itself (ethereum/solidity#3418), hence this fix does not really extend this tool to support this feature.

[frangio]

Yeah, I will keep this in mind when I write the documentation. (Soon!)

[frangio]

Released this in 0.3.11!

[barakman]

@frangio: Thanks.

I've upgraded to v0.3.11 and even tested this feature (by adding an {{#ownVariables}} clause in my template file).

It works, but... I receive this warning in stderr... I'm assuming that the console.warn(msg) in your oneTimeLogger function is responsible for that.

I'm not really familiar with the convention, but it seems a little strange that a warning is propagated via strerr and not via stdout.

My script for using solidity-docgen throws an error if anything is returned in stderr, and technically speaking - this is really just a warning of an unsupported feature (even the message says that).

Of course, I'm fine with it in either case, since I'm currently not using ownVariables anyway.
So just FYI.

Thanks again :)

[frangio]

It's standard for warnings to print on stderr. The idea is that stdout is "useful" output produced by the program, and stderr is any kind of logging.

You should check the process exit status to see if a program exited due to an error.

[barakman]

Yes, I think we might have discussed that issue before...
Thanks!