Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
zerosnacks opened this issue Mar 28, 2024 · 6 comments
Labels
enhancement New feature or request

Comments

@zerosnacks
Copy link
Member

zerosnacks commented Mar 28, 2024

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

@zerosnacks zerosnacks added the enhancement New feature or request label Mar 28, 2024
@zerosnacks zerosnacks changed the title [Feature] Ability to add documentation inside of sol! macro to satisfy missing_docs lint rule [Bug] Ability to add documentation inside of sol! macro to satisfy missing_docs lint rule Mar 28, 2024
@zerosnacks zerosnacks changed the title [Bug] Ability to add documentation inside of sol! macro to satisfy missing_docs lint rule [Feature] Ability to add documentation inside of sol! macro to satisfy missing_docs lint rule Mar 28, 2024
@alexfertel
Copy link
Contributor

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
Copy link
Member

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
Copy link
Contributor

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

For me this gives a warning as well.

@DaniPopes
Copy link
Member

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

@alexfertel
Copy link
Contributor

Yeah, I think that works 👍

alexfertel added a commit to OpenZeppelin/rust-contracts-stylus that referenced this issue Apr 9, 2024
Main things that changed:

- Syntax for conditional compilation was wrong, so fixed that.
`#[cfg(erc20)]` -> `#[cfg(feature = "erc20")]`.
- Added a `tests` flag as a workaround to
rust-lang/cargo#13595
- Moved to using workspace dependencies.
- Added example erc20.
- `alloy-primitives` did not need to be a dependency of `lib/crypto`.

As a result of setting features properly, we now have a missing docs
warning that we cannot fix. See
alloy-rs/core#588
@alexfertel
Copy link
Contributor

alexfertel commented May 5, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants