[contracts] Add docs generator for the contracts API to the #[define_env] macro
#13032
Conversation
agryaznov
added
A0-please_review
There was a problem hiding this comment.
Good approach 🎉
These are mainly nits concerning the emitted handwritten docs. They need to be especially nice since this is how we expect people to learn about the API.
One other thing:
I feel it is not optimal that we need to document private items to see these docs. This adds a lot of noise to the documentation that makes it hard to read if we enable this for docs.rs builds. The API surface is crafted in a way that it only exports types that are relevant to users (not developers) of this pallet. Contract(language) authors for which these docs are meant are part of the user group. What do you think about exporting the various modules emitted here in the crate root?
/// Contains the documentation of the API available to contracts.
///
/// This is not meant to be used by code. It is meant to be consumed by humans through rustdoc.
///
/// Every function described in this module's sub module's traits uses this sub module's identifier
/// as its imported module name. The identifier of the function is the function's imported name.
/// According to the [wasm spec of imports](https://webassembly.github.io/spec/core/text/modules.html#text-import).
mod api_doc {
// The mod seal_* go here
}The mod api_doc would be emitted by your macro. It is re-exported to the crate root.
| #[proc_macro_attribute] | ||
| pub fn define_env(attr: TokenStream, item: TokenStream) -> TokenStream { | ||
| if !attr.is_empty() { | ||
| let msg = "Invalid `define_env` attribute macro: expected no attributes: `#[define_env]`."; | ||
| if !attr.is_empty() && !(attr.to_string() == "doc".to_string()) { |
There was a problem hiding this comment.
I think we should rename doc to emit_doc. I think this makes it clearer what is happening. I won't die on that hill if you disagree, though.
There was a problem hiding this comment.
to me, concise variant is more elegant and could hardly be misinterpreted
There was a problem hiding this comment.
But isn't it the same attribute name used for rust doc comments itself? To be that seems confusing. Also, why doesn't this PR add this attribute to the define_env invocation in runtime.rs?
There was a problem hiding this comment.
But isn't it the same attribute name used for rust doc comments itself?
Yeah, that's exactly the point. The idea is to make it short, appropriate and familiar to the user: at the end of the day, it is for generating the rustdoc. I don't see it is confusing, as here doc attr is used as input to define_env(doc) rather than solo (as when it's for a single doc comment)
Also, why doesn't this PR add this attribute to the define_env invocation in runtime.rs?
I was thinking to leave it off by default. But could be done vice versa, why not. Added.
There was a problem hiding this comment.
I don't see it is confusing, as here doc attr is used as input to define_env(doc) rather than solo (as when it's for a single doc comment)
Fair enough.
I was thinking to leave it off by default. But could be done vice versa, why not. Added.
Most important point for me is that the standard docs.rs builds of this crate contain this documentation. Therefore it should be on.
There was a problem hiding this comment.
Most important point for me is that the standard docs.rs builds of this crate contain this documentation. Therefore it should be on.
Gotcha. The only objection comes to my mind is that it probably should be turned off for production use not to bloat runtime size in vain. Should we add a note to the macro docs at least?
There was a problem hiding this comment.
It should not bloat the runtime size as it is dead code and hence removed by the compiler. At least when using lto.
But you have a point there. I think you should just add #[cfg(doc)] to the emitted api_doc module (and its re-rexports). Then this code will be compiled out for every target but rustdoc.
Co-authored-by: Alexander Theißen <[email protected]>
unrelated elder docs;
agryaznov
changed the title
#[define_env] macro#[define_env] macro
|
@athei here goes the next version:
|
|
bot merge |
…_env]` macro (paritytech#13032) * macro to expand traits for host functions documentation * other way: same Doc trait in seal modules * added docs for macro, and remove `doc` attribute * fmt * Apply suggestions from code review Co-authored-by: Alexander Theißen <[email protected]> * make docs to be generated into re-exported `api_doc` module; fix unrelated elder docs; * make it compile without `doc` attr passed to macro * make alias functions indicated explicitly in docs * tidy up docs * refactored a bit * macro to auto-add doc warning for unstable functions * invoke macro with no doc generation by default * addressed review comments * hide api_doc module behind cfg(doc) Co-authored-by: Alexander Theißen <[email protected]>
…_env]` macro (paritytech#13032) * macro to expand traits for host functions documentation * other way: same Doc trait in seal modules * added docs for macro, and remove `doc` attribute * fmt * Apply suggestions from code review Co-authored-by: Alexander Theißen <[email protected]> * make docs to be generated into re-exported `api_doc` module; fix unrelated elder docs; * make it compile without `doc` attr passed to macro * make alias functions indicated explicitly in docs * tidy up docs * refactored a bit * macro to auto-add doc warning for unstable functions * invoke macro with no doc generation by default * addressed review comments * hide api_doc module behind cfg(doc) Co-authored-by: Alexander Theißen <[email protected]>
Follow-up to #11888
This PR makes the documentation for all the defined host functions available trough the rustdoc of the pallet_contracts.
To build the docs:
docattribute to the macro like#[define_env(doc)],it will make it expand additional
pallet_contracts::api_doc::seal0,pallet_contracts::api_doc::seal1,...modules each having itsApitrait containing functions holding documentation for every host function defined by the macro;cargo docas usual;