Introduction of @external/@ext natspec tags

[aarlt]

This PR introduces new @external and @ext natspec tags. This will give third-party applications the possibility to parse contracts and/or contract-methods annotations in a generic way.

The tag is named @external because it's semantic is not defined by solidity, the semantic is defined by the third-party application that uses specific @external tags.

The main problem is that the class dev::solidity::CompilerStack currently only provide access to two types of natspec comments related to User and Developer documentation. The natspec comments are strictly defined and not extensible. So contracts only allow @author, @title, @dev and @notice annotations, where contract-methods can be annotated with @author, @dev, @notice, @return and @param. Furthermore all new-lines will be discarded.

Third-party applications will benefit from the possibility to parse application-specific contract/contract-method annotations. The @external, short @ext tag can be extracted easily with dev::solidity::CompilerStack::natspecExternal(...) methods. In general, @external:<name> defines an external natspec comment for the module name, this will ensure extensibility, and will prevent potential conflicts of different @external annotations. In contrast to other natspec tags, new-lines will be preserved, where multiple definitions of the same @external tags will result in appending. Multi-line annotations are supported.

/// @external:ModuleA fancy moduleA annotation
/// multiline annotation for moduleA
contract Contract {
	uint256 stateVar;
	/// @external:ModuleB awesome moduleB annotation
	/// fancy multiline annotation for moduleB
	/// @external:ModuleC moduleC
	function functionName1(bytes32 input) returns (bytes32 out) {}
	/// @ext:ModuleD fancy annotation for moduleD
	function functionName2(bytes32 input) returns (bytes32 out) {}
}

[chriseth]

I think this is a very good proposal. Could you explain your reasons for introducing a third category? Why not just allow arbitrary tag names?

[aarlt]

@chriseth The introduction of that category will somehow open a new namespace for natspec tags that are not supported by solidity itself. Contract code that uses @external tags will clearly state: "an external tool is needed". If anyone can just define new tags arbitrarily, it may become a big source of confusion. It will not be clear anymore, what tags are really supported by solidity.

[aarlt]

Forgot that some tools may use solc directly. Added handling in solc with --extdoc.

[chriseth]

While it is probably a good idea to add a "user defined namespace", we will also have clashes inside that namespace. Apart from that, I'm not sure we should pull out data from that namespace into its own section. extdoc should be part of the developer docs, shouldn't they?

[aarlt]

Yeah sure, inside that "user defined namespace" clashes will be still possible, but they will not effect the solidity natspec tag integrity. Sure, extdoc could be seen as a part of the developer docs. I guess it's somehow just a matter of taste. If you prefer it that way, we can put everything into a special external node within the developers documentation. I would be totally ok with that.

[aarlt]

Probably there is no need to have an designated external node. We can put the stuff directly under the root or methods nodes of the developers documentation.

Furthermore, I realised that adding line number information could be from importance: so a mapping of each external line to the original line in the solidity file is possible.

[aarlt]

Btw, it looks like that natspecs doesn't work for constructors.

[aarlt]

I removed dev::solidity::CompilerStack::natspecExternal(...), dev::solidity::CompilerStack::natspecDev(...) can now be used to retrieve the @external or @ext annotations. So I reverted all the solc specific changes. Also natspecs annotations for constructors are supported now. I putted the constructor annotations also inside the method node, because they can be distinguished from normal methods by the method signature.

I didn't implement the line information yet. It seem to be more complex to do correctly, the only thing I currently see is to extract the line of the contract/constructor/method definition - that is not accurate enough. It would be nice to create a mapping of each @external line to the line number of the solidity source.

[chriseth]

Can you please extract your fix about natspec concerning constructors into a separate pull request?

[aarlt]

Created a separate PR for the constructor natspec issue, see PR #3666.

[aarlt]

@chriseth after doing a rebase, the test SolidityNameAndTypeResolution/no_warning_for_using_members_that_look_like_address_members fails.

It's also failing on develop:

➜  build git:(develop) ✗ test/soltest -- --no-smt --no-ipc
Running 1200 test cases...
/tmp/solidity/test/libsolidity/AnalysisFramework.cpp:108: fatal error: in "SolidityNameAndTypeResolution/no_warning_for_using_members_that_look_like_address_members": critical check !sourceAndErrors.second.empty() has failed

*** 1 failure is detected in the test module "SolidityTests"

[chriseth]

This was a failing test that made it into the develop branch. It should be fixed now, so please just rebase.

[aarlt]

External natspec tags will also provide the line information now. I also grouped the external tags into a node "external", where for all its members the "external:" prefix is already removed, this may simplify the usage of external tags.

So basically from the example contract

/// @external:ModuleA fancy moduleA annotation
/// multiline annotation for moduleA
contract Contract {
	uint256 stateVar;
	/// @external:ModuleB awesome moduleB annotation
	/// fancy multiline annotation for moduleB
	/// @external:ModuleC moduleC
	function functionName1(bytes32 input) returns (bytes32 out) {}
	/// @ext:ModuleD fancy annotation for moduleD
	function functionName2(bytes32 input) returns (bytes32 out) {}
}

the resulting devdoc json looks like that:

{
  "external" :
  {
    "ModuleA" :
    {
      "content" : " fancy moduleA annotation\n multiline annotation for moduleA\n",
      "line" : 0
    }
  },
  "methods" :
  {
    "functionName1(bytes32)" :
    {
      "external" :
      {
        "ModuleB" :
        {
          "content" : " awesome moduleB annotation\n fancy multiline annotation for moduleB\n",
          "line" : 4
        },
        "ModuleC" :
        {
          "content" : " moduleC\n",
          "line" : 6
        }
      }
    },
    "functionName2(bytes32)" :
    {
      "external" :
      {
        "ModuleD" :
        {
          "content" : " fancy annotation for moduleD\n",
          "line" : 8
        }
      }
    }
  }
}

[aarlt]

@chriseth Any news on this?

[chriseth]

This needs to be discussed in the next weekly meeting, I think.

[aarlt]

@chriseth ok, i think i could make it next week.

[aarlt]

I think we can close this PR. If we allow arbitrary tags during the redesign of natspecs, we should discuss whether it makes sense to allow the user to specify somehow that newlines should be preserved. We should also think about adding positioning information to the natspec.

It might also be useful if we add comments to the AST - newline preserving & with positioning information.

However, this is for sure a different discussion.

[axic]

@aarlt in that case can you create an issue describing the proposal?

[aarlt]

Will close that PR now.