natspec validation fails to parse example vyper code with `@external` modifier

[pcaversaccio]

Version Information

• vyper Version (output of vyper --version): 0.3.7
• OS: linux
• Python Version (output of python --version): 3.10.10

What's your issue about?

Many Vyper toolings use vyper-json instead of vyper to compile contracts. If you want to, e.g., compile my snekmate contracts using ape compile, it will throw multiple errors. I debugged and identified what is exactly raising these errors. To be clear, vyper compiles without any issues and this is also my expected behaviour for vyper-json.

Issue 1: It does not allow for comments like @external as part of the NatSpec description. Example:

# @version ^0.3.7
"""
@title Multi-Role-Based Access Control Functions
@license GNU Affero General Public License v3.0
@author pcaversaccio
@notice These functions can be used to implement role-based access
        control mechanisms. Roles are referred to by their `bytes32`
        identifier. These should be exposed in the external API and
        be unique. The best way to achieve this is by using `public
        constant` hash digests:
        ```vy
        MY_ROLE: public(constant(bytes32)) = keccak256("MY_ROLE");

    Roles can be used to represent a set of permissions. To restrict
    access to a function call, use the `external` function `hasRole`
    or the `internal` function `_check_role`:
    ```vy
    @external
    def foo():
        assert self.hasRole[MY_ROLE][msg.sender], "AccessControl: account is missing role"
        ...

    OR

    @external
    def foo():
        self._check_role(MY_ROLE, msg.sender)
        ...
    ```

The warning thrown is:

```bash
ERROR: (VyperCompileError) auth/AccessControl.vy
NatSpecSyntaxException:Unknown NatSpec field '@external'

Issue 2: It does not allow for a custom tag as part of the NatSpec comments. I use custom:coauthor and custom:security. The reasoning behind my choice here is consistency with the Solidity NatSpec tags that support custom for exactly such cases & to make an important emphasise on a certain topic (in my case security or co-authors) if this is really needed.

The warning thrown is:

ERROR: (VyperCompileError) extensions/ERC4626.vy
NatSpecSyntaxException:Unknown NatSpec field '@custom:security'

or

ERROR: (VyperCompileError) tokens/ERC1155.vy
NatSpecSyntaxException:Unknown NatSpec field '@custom:coauthor'

Issue 3: Path resolution of certain interface imports fail. I use the following import (which compiles using vyper) here:

import src.tokens.interfaces.IERC20Permit as IERC20Permit
implements: IERC20Permit

It will throw the following error (the same happens if I put it into a dedicated contracts folder):

ERROR: (VyperCompileError) extensions/ERC4626.vy
FileNotFoundError: Cannot locate interface 'src/tokens/interfaces/IERC20Permit{.vy,.json}'

After fixing these 3 issues locally (removing the comments and using an interface from the same folder for ERC4626) vyper-json compiles as intended.

How can it be fixed?

vyper-json should compile successfully as vyper does now. I hope someone finds the bandwidth to fix this for the Vyper 0.3.8 release.

[charles-cooper]

i think this is not exactly a difference between vyper and vyper-json tools. i can reproduce the exception as follows:

$ vyper -f devdoc src/auth/AccessControl.vy 
Error compiling: src/auth/AccessControl.vy
vyper.exceptions.NatSpecSyntaxException: Unknown NatSpec field '@external'
  line 19:9 
       18         ```vy
  ---> 19         @external
  -----------------^
       20         def foo():

so the issue here is presumably that ape is asking for the devdoc and userdoc outputs.

[pcaversaccio]

@fubuloubu so seems like it's an ape "issue". Why does ape compile need devdoc and/or userdoc exactly? Is there maybe an easy way to circumvent it, like ape compile --skip-docgen which would allow to compile snekmate's current code?

[fubuloubu]

@fubuloubu so seems like it's an ape "issue". Why does ape compile need devdoc and/or userdoc exactly? Is there maybe an easy way to circumvent it, like ape compile --skip-docgen which would allow to compile snekmate's current code?

The issue is definitely with Vyper, it raises a Vyper exception. If Vyper wants to add support for these types of fields, it can (per this issue). If it wants to relax validation of these fields, it can.

I don't think it's tenable to have it not compile with -f devdoc flag if the intent is to support devdoc natspec. At best it's a workaround to skip this in Ape, since it enables some other features. I think either Vyper adds support or relaxes validation, or you remove the offending tags from your docstrings since they won't compile with Vyper.

Probably a bigger issue is a lack of standardization of these fields, but that's outside of this discussion.

[pcaversaccio]

@charles-cooper what's your opinion on this? Can we relax the validation and allow for the custom tags (i.e. custom:coauthor and custom:security), as well as fix the parsing of NatSpec comments that use @ labels? In any case, I think having the NatSpec tag @custom:... is a good idea since maybe there will be other use cases such as @custom:audit or @custom:contact etc. that can be beneficial.

One small thing, in this PR here I fixed a spelling error for the license tag: https://github.com/vyperlang/vyper/commit/50ee0787de72e854443275175c6cc57fae6c2ea7. So it could be that people have used the wrong license tag (i.e. @licence) which would also throw if compiled with devdoc and userdoc.

[fubuloubu]

I think there's two issues:

1. Vyper is a little aggressive with detecting natspec fields from within other natspec fields (e.g. it needs to track indentation in the docstring to ensure that it isn't catching something like in your example where you have an example inside of the docstring that contains something that looks like a natspec field)
2. Vyper should add support for, or ignore, unsupported/experimental natspec fields

I would separate those two issues. 1. is a bug that should be fixed, 2. is a feature vyper should support with it's natspec parser

[charles-cooper]

i vote for just relaxing the validation. i am not a huge fan of having to support natspec (which is an entirely separate language!) inside the compiler in the first place.

[charles-cooper]

for the @custom: tags, see https://github.com/vyperlang/vyper/pull/3403

[fubuloubu]

for the @custom: tags, see https://github.com/vyperlang/vyper/pull/3403

This is perfect, works for me

Still think the other issue is the bug with aggressive checking for natspec tags. It should basically detect whatever indentation the first tag is at, and all tags must be at that level. If you indent further it gets treated as whitespace to be normalized into a space or newline char (see his example). Could add additional handling for triple back ticks although maybe overkill

[pcaversaccio]

for the @custom: tags, see #3403

awesome, I like! I quickly made a PR to cover the docs: https://github.com/vyperlang/vyper/pull/3404.

Still think the other issue is the bug with aggressive checking for natspec tags. It should basically detect whatever indentation the first tag is at, and all tags must be at that level. If you indent further it gets treated as whitespace to be normalized into a space or newline char (see his example). Could add additional handling for triple back ticks although maybe overkill

I like this, since it's consistent with the Vyper docs that state the following:

[pcaversaccio]

Is there a smart quick fix that doesn't introduce any technical debt which would allow me to compile my example with @external and could be shipped in 0.3.8? Later on, we can revisit this issue for a more robust solution for 0.4.0. I don't consider relaxing the validation btw as a security risk.

[fubuloubu]

Is there a smart quick fix that doesn't introduce any technical debt which would allow me to compile my example with @external and could be shipped in 0.3.8? Later on, we can revisit this issue for a more robust solution for 0.4.0. I don't consider relaxing the validation btw as a security risk.

I don't think so, you'll have to fix the parser as per our previous conversation. I don't think it'd be a lot of work though

[charles-cooper]

Relaxing the validator to ignore unknown tags would be "low debt". We can also change the parser, but we would essentially be creating "natspec vyper", and correct me if I'm wrong, but I don't think anybody really wants to have multiple natspec dialects floating around.

[fubuloubu]

Relaxing the validator to ignore unknown tags would be "low debt". We can also change the parser, but we would essentially be creating "natspec vyper", and correct me if I'm wrong, but I don't think anybody really wants to have multiple natspec dialects floating around.

the problem isn't really that @external is an unknown natspec tag, the problem is it's interpreting a code example he gave in the dev comment as the start of a new field, when really it's not

and yes I agree, we did sort of commit to maintaining a natspec parser in vyper

[charles-cooper]

Well right - the natspec spec does not (IIRC) have any carveouts for code examples inside of dev comments

[fubuloubu]

Well right - the natspec spec does not (IIRC) have any carveouts for code examples inside of dev comments

Also true

@pcaversaccio triggered undefined behavior

[pcaversaccio]

the problem isn't really that @external is an unknown natspec tag, the problem is it's interpreting a code example he gave in the dev comment as the start of a new field, when really it's not

Exactly - so how about we protect markdown code inside NatSpec comments that starts with ``` (and maybe inline backticks as well)?

Well right - the natspec spec does not (IIRC) have any carveouts for code examples inside of dev comments

correct :)

[fubuloubu]

the problem isn't really that @external is an unknown natspec tag, the problem is it's interpreting a code example he gave in the dev comment as the start of a new field, when really it's not

Exactly - so how about we protect markdown code inside NatSpec comments that starts with ``` (and maybe inline backticks as well)?

Probably solidity doesn't have this problem because it doesn't use @ when defining modifiers :rofl:

[pcaversaccio]

Probably solidity doesn't have this problem because it doesn't use @ when defining modifiers 🤣

Well, you can have comments like

@notice Please follow the instructions:

          npm install @openzeppelin/contracts
          ```
          ...

[fubuloubu]

Probably solidity doesn't have this problem because it doesn't use @ when defining modifiers rofl

Well, you can have comments like

@notice Please follow the instructions:

          npm install @openzeppelin/contracts
          ```
          ...

But that's not at the beginning of the sentence

[pcaversaccio]

right, that's fair 🤣

[charles-cooper]

yea i mean i want to point out that the fact that solidity currently allows @external in some places (it, by the way, does not handle markdown consistently) is already breaking from their own natspec specification. so the natspec supported by solc is actually already "natspec solc" and not strict natspec. so by adding our own special rules we are really just compounding the problem.

which if we are going to make our own dialect, we might as well go all the way and make our own full-on documentation language :).

(mostly joking)

i also want to point out that another other option is to drop natspec entirely and just have people use doxygen's python dialect, or sphinx, or really any number of purpose-built inline documentation tools.

btw just to make sure my position is clear - i'm not against adding the markdown rules. i think that is probably the most pragmatic path forward here. but i just want to make sure everybody understands that introducing a new natspec dialect is not a trivial choice to make, and will almost definitely create issues in the future ("why does my natspec work with vyper and not solc" or vice versa - the flip of which we are currently already seeing due to solidity creating their own undocumented natspec dialect).

[pcaversaccio]

per offline discussion with @charles-cooper, I will quickly fix the @external comments, for now, (maybe will use an emoji for the @ symbol lol) to make it compile using Vyper 0.3.8. We must revisit this discussion after the release. Generally, my position is pretty clear: support the Solidity NatSpec rules in order to ensure interoperability, as well as have a few exceptions due to Python/Vyper syntax (e.g. relaxing validation for markdowns or inline code comments) and document this properly. If properly documented, folks will understand very quickly why something might not compile.

@fubuloubu could you quickly check the "issue 3" I raised above when using ape? Also, can you confirm that by using the latest Vyper version and removing the @external tag in AccessControl.vy, you can successfully compile all snekmate contracts?

[fubuloubu]

per offline discussion with @charles-cooper, I will quickly fix the @external comments, for now, (maybe will use an emoji for the @ symbol lol) to make it compile using Vyper 0.3.8. We must revisit this discussion after the release. Generally, my position is pretty clear: support the Solidity NatSpec rules in order to ensure interoperability, as well as have a few exceptions due to Python/Vyper syntax (e.g. relaxing validation for markdowns or inline code comments) and document this properly. If properly documented, folks will understand very quickly why something might not compile.

@fubuloubu could you quickly check the "issue 3" I raised above when using ape? Also, can you confirm that by using the latest Vyper version and removing the @external tag in AccessControl.vy, you can successfully compile all snekmate contracts?

Remove src

[fubuloubu]

Also, can you confirm that by using the latest Vyper version and removing the @external tag in AccessControl.vy, you can successfully compile all snekmate contracts?

I really don't have time at the moment to do this

[pcaversaccio]

well in order to compile it with Vyper, I need to have either the full path:

import src.tokens.interfaces.IERC20Permit as IERC20Permit
implements: IERC20Permit

or a relative import

from ..tokens.interfaces.IERC20Permit import IERC20Permit
implements: IERC20Permit

I can consider using a relative import. But after that, if it doesn't compile with ape, it must be considered as an ape issue. I will stop discussing issue 3 now and if you face such an additional issue I would suggest opening an issue in ape.

[fubuloubu]

well in order to compile it with Vyper, I need to have either the full path:

import src.tokens.interfaces.IERC20Permit as IERC20Permit
implements: IERC20Permit

or a relative import

from ..tokens.interfaces.IERC20Permit import IERC20Permit
implements: IERC20Permit

I can consider using a relative import. But after that, if it doesn't compile with ape, it must be considered as an ape issue. I will stop discussing issue 3 now and if you face such an additional issue I would suggest opening an issue in ape.

the compilation depends on what folder you execute it from. Ape standardizes the folder to be whatever the top level folder is configured to be (e.g. src/). So it may compile for you when you execute it a specific way, but it's not necessarily repeatable when packaging the sources together

[pcaversaccio]

FYI https://github.com/pcaversaccio/snekmate/pull/113