Inheritance in natspec (documentation)

[axic]

Inheritance is an untouched subject on Natspec -- I haven't found any issue regarding it.

Take this example:

interface ERC20 {
  /// @notice Transfer ``amount`` from ``msg.sender`` to ``to``.
  /// @param to The destination address.
  /// @param amount The amount to be transferred.
  /// @return Success or failure.
  function transfer(address to, uint amount) external returns (bool);
}

contract Token is ERC20 {
  function transfer(address to, uint amount) override external returns (bool) {
    return false;
  }
}

This currently generates no user/devdoc on Token:

======= natspec.sol:ERC20 =======
Developer Documentation
{
  "methods":
  {
    "transfer(address,uint256)":
    {
      "params":
      {
        "amount": "The amount to be transferred.",
        "to": "The destination address."
      },
      "returns":
      {
        "_0": "Success or failure."
      }
    }
  }
}
User Documentation
{
  "methods":
  {
    "transfer(address,uint256)":
    {
      "notice": "Transfer ``amount`` from ``msg.sender`` to ``to``."
    }
  }
}

======= natspec.sol:Token =======
Developer Documentation
{
  "methods": {}
}
User Documentation
{
  "methods": {}
}

Q1. It would be nice to have interfaces documented and transfer that over to the function in the documentation. Is that a bad idea?

Q2. What happens if the implementing function has a different notice or param. Does it take precedence? Is it ignored? Does it only take precedence over implementing an interface or applies to inheriting from contracts?

[hrkrshnn]

Firstly, it's not mentioned in the docs how inherited (implemented) members would contain the NatSpec from base. Perhaps we should add this under the section "Inheritance Notes."

---

For Q2.

1. For @notice, the overriding function's notice should take precedence. I see no arguments against this.
2. For @param I think it's reasonable to assume that the overriding function will not have different description for @param. But for the sake of consistency, we should allow the NatSpec to be overridden.

With this rule, I don't see any problem with such a feature for interfaces. Also, why do we have to restrict this for only interfaces? Contracts can also have this.

[axic]

Oh, you mention https://solidity.readthedocs.io/en/v0.6.7/natspec-format.html#inheritance-notes with inheritance notes.

[axic]

@chriseth @aarlt @frangio @ethers any opinion here?

[chriseth]

We looked into that in the past, but I think doxygen and javadoc do not mention this subject, unfortunately.

I would prefer to have an all-or-nothing behaviour: As soon as you provide a docstring, the base docstring is ignored. The same should happen for a function overriding multiple base functions, even if only one of them specfies docstrings.

In addition to that, we could add @inheritdoc <ContractName>, which means that all doc tags from that contract are taken (has to be a direct base) but mentioning a doc tag again in the same comment will override it.

[frangio]

In the docs for OpenZeppelin Contracts we manually add a reference to the overriden function. ERC20.transfer, for example, looks like this:

We plan to eventually also embed along with the link a portion of the referenced docs so the user doesn't have to click through so many links to see what is conceptually a single piece of information. It would probably be the first sentence, or maybe a few sentences up to some other delimiter.

We would not expect any of this to be done automatically by the compiler, but we would be interested in some help from the compiler. For example, I don't think the AST currently has a reference to the function that is being overriden, so we don't have a way of automating the "See Parent.foo" text. That would be very valuable.

[frangio]

I don't think the AST currently has a reference to the function that is being overriden

I see now that the FunctionDefinition AST node contains a baseFunctions array, but I'm not sure if that's what I want.

[aarlt]

@axic I think this is a really good idea. I could imagine that, if the implementing functions have different natspec tags, that the defined tags will override the content defined by the base classes, but the rest will just be inherited. This may improve the reusability.

I also agree with @hrkrshnn that this may also be useful for contracts.

[axic]

On today's meeting we've agreed to the following:

1. Inherit from base class by default if there are no annotation.
2. Do not inherit if there are conflicting base definitions (two contracts/interfaces defining the same function).
3. Do not inherit implicitly if the are annotations (any parseable tag) present.
4. Can use @inheritdoc (or @copydoc?) without an argument to inherit from the base class or use an argument to specify a class to inherit from. In this case any additional annotation replaces the given inherited one, i.e. a @notice would replace the inherited notice.

[Marenz]

I wonder if the given rules might be too intransparent for the end user

[frangio]

One issue with @inheritdoc with an argument is that NatSpec in the AST is included as text without semantic information, and notably there would be no referencedDeclaration field for the identifier included along with @inheritdoc. This would make it difficult to parse NatSpec out of the AST. I understand that the compiler has an output selection for documentation, but being able to use only the AST is useful (and it's the approach I used in solidity-docgen).

[hrkrshnn]

@frangio This issue #8247 might be relevant.

[chriseth]

I think we should also not inherit the documentation if one of the parameters or return parameters are named differently than in the base function.

[Marenz]

Regarding point 2, conflicting definitions. Is there mere presence of multiple bases enough to trigger this, or do they also need to have documentation?
e.g. when we have

contract A
    function foo(); // no docu
contract B
    /// Natspec docu
    function foo();
contract C is A, B
    function foo();

Would we have a conflict already?

[axic]

Default inheritance of events does not seem to be working with #9267.

[Marenz]

natspec inheritance with events doesn't work because we never fill their "baseFunctions" variable in the callableannotation
and they seem to be generally a bit "outside" our inheritance logic/topic as you can specify events with the same name but different parameters in base/child