Add support for enum values in NatSpec #15956
Conversation
libsolidity/ast/ASTJsonExporter.cpp
Outdated
| @@ -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.
There was a problem hiding this comment.
@cameel unfortunately, this doesn't quite work in the case when a natspec doc is provided beyond the last value, as the natspec comment is not associated with any _node, which then segfaults here.
Going over the exporter in more detail now, and we perform a node.documentation() check for every single export, i.e. we don't use toJson(*_node.documentation()) by itself.
I'll therefore keep this as is.
There was a problem hiding this comment.
No problem keeping it as is, but this works for me:
| std::make_pair("documentation", _node.documentation() ? toJson(*_node.documentation()) : Json()), | |
| std::make_pair("documentation", toJson(&*_node.documentation())) |
or
| std::make_pair("documentation", _node.documentation() ? toJson(*_node.documentation()) : Json()), | |
| std::make_pair("documentation", toJson(_node.documentation().get())) |
I guess you forgot to pass the pointer and it is resolving to the overload which takes a reference instead.
There was a problem hiding this comment.
Aaah OK, guess I'm blind having not seen the second toJson() overload that was right below the one I was debugging.
Changelog.md
Outdated
| @@ -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?
There was a problem hiding this comment.
For completeness, we should also test an enum defined outside of a contract.
Isn't this one technically 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?
Good call, it segfaults in the ASTJSON test, but does not in the natspecJSON one, so it's likely an issue in the JSON exporter. Will investigate and fix.
There was a problem hiding this comment.
As it turns out, it wasn't segfaulting initially, but only after I applied Matheus' suggestion. Right now, it simply ignores any natspec doc comment that isn't associated with an actual AST node (in this case enum value), and this looks to be the case across the board for all such comments/docs.
|
@nikola-matic Please link to the original PR in the description (#14193). Since this PR completely replaces it, you can even do |
nikola-matic
force-pushed
the
make-enums-available-in-natspec
branch
2 times, most recently
from
ef742a7 to
7da89a9
Compare
nikola-matic
force-pushed
the
make-enums-available-in-natspec
branch
from
7da89a9 to
c4b454a
Compare
References #12295.
Closes #14193.
Thanks @veniger for the original PR.