alloy-rs / core

High-performance, well-tested & documented core libraries for Ethereum, in Rust
https://alloy.rs
Apache License 2.0
763 stars 137 forks source link

[Feature] Ability to add documentation inside of sol! macro to satisfy `missing_docs` lint rule #588

Open zerosnacks opened 5 months ago

zerosnacks commented 5 months ago

Component

sol! macro

Describe the feature you would like

When using the following common lint rule:

[lints]
rust.missing_docs = "warn"

It will warn users that this sol! block is missing documentation with the error: missing documentation for a struct field requested on the command line with "-W missing-docs"

sol! {
    #[sol(rpc, bytecode="6080...0033")]
    contract Counter {
        uint256 public number;

        function setNumber(uint256 newNumber) public {
            number = newNumber;
        }

        function increment() public {
            number++;
        }
    }
}

It is however currently not possible to add documentation inside of the sol! macro and therefore it requires adding a #[allow(missing_docs)] for now.

Additional context

Added a minimal repro here: https://github.com/zerosnacks/alloy-core-repro-588

Unclear if this should be marked as a feature request or a bug

alexfertel commented 5 months ago

Came here to open this exact issue!

Fwiw, this invocation gives the same warning:

sol! {
    /// Emitted when `value` tokens are moved from one account (`from`) to
    /// another (`to`).
    ///
    /// Note that `value` may be zero.
    event Transfer(address indexed from, address indexed to, uint256 value);
    /// Emitted when the allowance of a `spender` for an `owner` is set by a
    /// call to `approve`. `value` is the new allowance.
    event Approval(address indexed owner, address indexed spender, uint256 value);
}
DaniPopes commented 5 months ago

Is it my understanding that you would prefer something like the following, instead of allowing the lint with #[allow(missing_docs)]?

sol! {
    /// ...
    event Approval(
        /// ...
        address indexed owner,
        /// ...
        address indexed spender,
        /// ...
        uint256 value
    );
}
alexfertel commented 5 months ago

Is it my understanding that you would prefer something like the following

For me this gives a warning as well.

DaniPopes commented 5 months ago

Yes, it's not implemented, I am asking if that's what you would like

alexfertel commented 5 months ago

Yeah, I think that works 👍

alexfertel commented 4 months ago

Is it my understanding that you would prefer something like the following, instead of allowing the lint with #[allow(missing_docs)]

Ah @DaniPopes, so I was revisiting this and reading the code, and I realized that I probably misunderstood your question twice.

I think I'd prefer if we annotated the fields of the generated event struct with #[allow(missing_docs)], which is what you suggested?

I initially thought you meant:

sol! {
    /// ...
    event Approval(
        #[allow(missing_docs)]
        address indexed owner,
        #[allow(missing_docs)]
        address indexed spender,
        #[allow(missing_docs)]
        uint256 value
    );
}

It's a one-line change, so I'll just open a PR. I'm not entirely sure this addresses the original issue though, let me know if that's the case.