Remove all #![allow(...)]

[MuhtasimTanmoy]

Closes https://github.com/risechain/pevm/issues/410

[i1i1]

I think there is no workaround for this allow(unused_crate_dependencies). The issue is related to 2 things:

• Rust compiles integration tests and crate itself as different compile targets (that is why you have to use all the dependencies in tests specifically)
• You can't disable lints specifically for tests

So #![allow(unused_crate_dependencies)] in each of the tests is probably the best workaround.

Of course tests can be squashed and that would make only 1 allow on the very toplevel.

WDUT?

[hai-rise]

These false positives are very noisy so I'd vote to remove unused_crate_dependencies if there's no clean workaround. Can replace it with cargo-machete in the CI but practically, at the current scale we can manually bump and audit dependencies per every Reth or our own release.

[i1i1]

Makes sense! False positives are annoying.

Can we make an issue for cargo machete or generally for checking unused dependencies? I think it is really important to keep track of it and keep the repo in shape

[hai-rise]

Okay, I've opened #420 and @MuhtasimTanmoy let's remove the unused_crate_dependencies lint in this PR 🙏.

[MuhtasimTanmoy]

Done.

What this config #![cfg_attr(test, allow(unused_crate_dependencies))] in crates/pevm/src/lib.rs does?

[hai-rise]

What this config #![cfg_attr(test, allow(unused_crate_dependencies))] in crates/pevm/src/lib.rs does?

It allows unused_crate_dependencies for tests which we can also remove now that we've turned off the lint.

@MuhtasimTanmoy CI is failing for missing docs. It'd be more appropriate for us to add them so let's keep the allow in for now, and just make this PR remove unused_crate_dependencies. What do you think 🙏?

[MuhtasimTanmoy]

It allows unused_crate_dependencies for tests which we can also remove now that we've turned off the lint.

Done.

Total 63 missing docs. Can we go for bare minimum doc? I was sort of going through the codebase. You can provide some context to some of them.

[hai-rise]

Can we go for bare minimum doc?

@MuhtasimTanmoy Yes, that should work for now as the missing ones are only in test/bench anyway. There's existing // that can be quickly converted to /// too 🙏.

[MuhtasimTanmoy]

Going for this approach then.

[MuhtasimTanmoy]

Done.

Only two left with warnings during documenting macros.

The following lines giving warning: missing documentation for a function

criterion_group!(benches, criterion_benchmark);
criterion_main!(benches);

After converting to

/// Benchmark
criterion_group!(benches, benchmark_gigagas);

Warning persists.

/// Benchmark
| ^^^^^^^^^^^^^ rustdoc does not generate documentation for macro invocations

May be missing something.

[MuhtasimTanmoy]

@hai-rise Cant document macro invocation. Also, it's library provided. Any insight?

Context:

• Rust document macro invocations
• Generating documentation in macros

[i1i1]

Hey @MuhtasimTanmoy! Thanks again for working on this :pray:

I think we can't do much about macro invocation, so lets just #[allow(missing_docs)] and merge it for now.

I've tried to fix it and it seems it requires some workaround like follows:

-criterion_group!(benches, criterion_benchmark); 
-criterion_main!(benches);
+#[allow(missing_docs)]
+mod benches {
+    use super::*;
+    criterion_group!(benches, criterion_benchmark);
+}
+
+criterion_main!(benches::benches);

Could you do smth like this and just add some comment like // HACK: we can't document public items inside of the macro? :pray:

[MuhtasimTanmoy]

Ok, I @i1i1

I got a similar solution on StackOverflow from my question as well.

[MuhtasimTanmoy]

Done.

[MuhtasimTanmoy]

Sure. Should have checked.