solidity-parser-antlr icon indicating copy to clipboard operation
solidity-parser-antlr copied to clipboard

Natspec feature throwing errors in natspec not attached to functions

Open Janther opened this issue 4 years ago • 9 comments

when running the latest code of master against openzepellin contracts there are many errors because of the natspec feature.

for example when running it against contracts/mocks/ERC165/ERC165InterfacesSupported.sol I get these errors.

ParserError: no viable alternative at input '/**\n     * @dev A mapping of interface id to whether or not it's supported.\n     */mapping' (24:4)
    at Object.parse (/Users/klaushottvidal/DAPPS/solidity-parser-antlr/dist/index.js:79:11) {
  message: "no viable alternative at input '/**\\n     * @dev A mapping of interface id to whether or not it's supported.\\n     */mapping' (24:4)",
  errors: [
    {
      message: "no viable alternative at input '/**\\n     * @dev A mapping of interface id to whether or not it's supported.\\n     */mapping'",
      line: 24,
      column: 4
    },
    {
      message: "no viable alternative at input '/**\\n     * @dev A contract implementing SupportsInterfaceWithLookup\\n     * implement ERC165 itself.\\n     */constructor'",
      line: 30,
      column: 4
    }
  ]
}

my guess is that probably the current rules only consider natspec for functions, contracts, libraries, and interfaces (notice that the constructor is a valid place for natspec and also throws an error in the example shown).

While the solc only considers these cases, people use natspec for variable declaration and modifier among other nodes so perhaps ignore these cases without throwing errors or also parse them (I like the latter better but it's up to you).

Janther avatar Aug 04 '19 07:08 Janther

Cool, this is exactly the kind of feedback I wanted to get before publishing 0.5.0. Thank you @Janther!

I'll try to look into this in the next days, but I don't have much time. Pinging @andremedeiros if he wants to check this out too.

federicobond avatar Aug 04 '19 15:08 federicobond

on the top of my mind, the nodes that are direct child of ContractDefinition, and therefor very likely to get an @dev or an @notice (or a cheeky @author if a dev want to take full credit of a feature but it's unlikely), are:

  • EnumDefinition
  • EventDefinition
  • FunctionDefinition (constructors also fall here)
  • ModifierDefinition (can also get @param)
  • StateVariableDeclaration
  • StructDefinition
  • UsingForDeclaration

I hope this helps.

Janther avatar Aug 04 '19 16:08 Janther

Hi Fede 👋🏻

Was this released in the latest version?

I've been contacted by one of Aragon's engineers to tell me that their CI broke because solidity-parser-antlr fails to parse this file. The error message in their CI looks like an instance of this issue.

btw, thanks for maintaining this project! ❤️

alcuadrado avatar Aug 29 '19 20:08 alcuadrado

Yes, I messed up the latest release, sorry! I created the stable branch here to continue the 0.4.x series without having to worry about NatSpec problems, but I did not choose the right solidity-antlr tip. There should be a stable branch there too without NatSpec changes.

I don't have much time today, but if you can help me out we should be able to get this solved by rebuilding the stable branch here with the fixed stable branch tip from solidity-antlr.

federicobond avatar Aug 29 '19 20:08 federicobond

Well, I think I got it fixed in 0.4.11. Can you check with the Aragon folks?

federicobond avatar Aug 29 '19 20:08 federicobond

It works now :) Thanks @federicobond !

alcuadrado avatar Aug 29 '19 21:08 alcuadrado

Hey, Can you guys extract the natspec? It seems like it's not working. I took an example from a test and does not work

const ast = parser.parse(
    `/**
        * @dev This is the Sum contract.
        * @title Sum Contract
        * @author username
        */
    contract Sum { }`
);
console.log(ast);

I'm using 0.4.11

obernardovieira avatar Oct 02 '19 19:10 obernardovieira

As far as I know, the Natspec feature was put on hold until this issue is solved. The comments about this getting fixed in 0.4.11 were because the feature was published by mistake and removed with 0.4.11.

Janther avatar Oct 04 '19 09:10 Janther

hmm, Well, I coded most of it when it was first merged. But I think, the problem is because the wrong ANTLR code was merged. The code merged wasn't mine, so it didn't work together. @federicobond As a reference:

  • https://github.com/federicobond/solidity-parser-antlr/commit/2273972a655e10cf53c8ac8a9ded5c1f9e87fca6
  • https://github.com/federicobond/solidity-parser-antlr/pull/71
  • https://github.com/solidityj/solidity-antlr4/pull/19

When I did this, I didn't parse variables natspec, and now I was planning to do so.

obernardovieira avatar Oct 04 '19 14:10 obernardovieira