Add support for enum values in NatSpec #15956
Conversation
| @@ -430,6 +430,7 @@ bool ASTJsonExporter::visit(EnumValue const& _node) | |||
| setJsonNode(_node, "EnumValue", { | |||
| std::make_pair("name", _node.name()), | |||
| std::make_pair("nameLocation", sourceLocationToString(_node.nameLocation())), | |||
| std::make_pair("documentation", _node.documentation() ? toJson(*_node.documentation()) : Json()), | |||
There was a problem hiding this comment.
Could also use the overload toJson(ASTNode*) which already tests and returns the proper value.
| // "kind": "user", | ||
| // "methods": {}, | ||
| // "version": 1 | ||
| // } |
There was a problem hiding this comment.
The test is the same as the other one?
There was a problem hiding this comment.
Looks like this is also the case for docstring_enum.sol/enum_no_docs.sol these seem to be based on.
Apparently both were added in #13626, but docstring_enum.sol was originally a syntax test. Since Natspec JSON tests are a superset of syntax tests (they do full analysis and IIRC can also display errors the way syntax tests do), when I created it, I moved some Natspec-related syntax tests there (dc68480). So I could have just removed it, because it is simply redundant.
We should do that now. Let's leave just one of each and call them enum.sol and enum_value.sol.
| /// @title example of title | ||
| /// @author example of author | ||
| /// @notice example of notice | ||
| /// @dev example of dev | ||
| Green |
There was a problem hiding this comment.
I see that we accept @param, @return and @inheritdoc as well. These do not make sense for enum values. We should disallow them. And have a test covering them.
There was a problem hiding this comment.
Actually, looks like they're also allowed on enums in general. Or structs. And they would compile on enum values before this PR as well, they were just not included in the AST. That would make disallowing them breaking...
And to complicate things even more, looks like Foundry is planning to actually interpret @param on enums as referring to the values: foundry-rs/foundry#10022 (review).
So I don't think we can really disallow them, but then we should document them. Natspec Format > Tags does not list @param, @return and @inheritdoc for enums, structs and probably other things that support them, even though these values are not only allowed but also captured as StructuredDocumentation in the AST.
There was a problem hiding this comment.
The tags allowed for enum values should be documented in Natspec Format > Tags.
Unless it is assumed to be covered by enum? I guess struct fields allow Natspec as well and they are not explicitly listed there. On the other hand we don't capture Natspec in AST for struct fields but we now do for enum values so the situation is not the same.
There was a problem hiding this comment.
Enums are also not mentioned at all in this bit:
Documentation is inserted above each
contract,interface,library,function, andeventusing the Doxygen notation format
| /// @title example of title | ||
| /// @author example of author | ||
| /// @notice example of notice | ||
| /// @dev example of dev |
There was a problem hiding this comment.
Please include a @custom tag as well.
| @@ -4,6 +4,7 @@ Language Features: | |||
|
|
|||
|
|
|||
| Compiler Features: | |||
| * NatSpec: Add support for NatSpec documentation in ``enum`` value definitions. | |||
There was a problem hiding this comment.
I see that this is how we were describing such additions in the past, but "support" is quite ambiguous. This Natspec will not show up in --userdoc or --devdoc output. Using the tags also wasn't even disallowed on enum values before. The only thing that changes is that it's now captured in the AST. How about we say just that?
| * NatSpec: Add support for NatSpec documentation in ``enum`` value definitions. | |
| * NatSpec: Capture Natspec documentation of `enum` values in the AST. |
Perhaps this should also be stated in the docs. Right now they're rather vague and even incorrect, depending on how you define "support". After reading them and checking this PR I'm really confused about what we actually support.
There was a problem hiding this comment.
For completeness, we should also test an enum defined outside of a contract.
There was a problem hiding this comment.
And I'd also test what happens with Natspec that's after the last value. Is it ignored or does it cause an error?
|
@nikola-matic Please link to the original PR in the description (#14193). Since this PR completely replaces it, you can even do |
References #12295.
Closes #14193.
Thanks @veniger for the original PR.