Rm deadcode of EVM circuit

[ChihChengLiang]

Description

• Removed #[allow(dead_code)] and #![allow(unused_imports)].
• Removed most of the dead code. Kept a few with the "_" initial name.
• Reorganize dev modules and feature flags for "zkevm-circuits" crate.
  • "test" flag is only used when test utils are shared within "zkevm-circuits" "test" flag is gone.
  • "test-util" flag is used when utils are shared with testool crate.
  • "test-circuits" flag is reserved for test circuits that are shared with integration tests.

Issue Link

Type of change

• New feature (non-breaking change which adds functionality)

Rationale

These are the reason I keep a method:

• The method is a sensible API and is tested. Although it is unused but it makes sense to keep it. The remainder method of the Constant Division Gadget is such an example.
• Variants in Enums. We use the match macro for them so it is important we don't miss any variant. It makes sense to keep them even when we don't use them.
• Work in progress and recent works. Cell manager and _error_depth are examples.

I remove a method or a gadget for these reasons.

• If it has been there for a long time but never used.
• Default removing them to question us hard if the gadget really sparks joy. batched_is_zero and binary number gadgets are such cases.

How Has This Been Tested?

cargo build --all-features
cargo test // just for build, without running the full tests

[ed255]

Nice clean up! I think it's great to remove unused code to keep the codebase clean :)

At first when reviewing the PR I was a bit puzzled by all the _ prefixes added, then I realized that this is the way Rust stops complaining about unused stuff. I would suggest a different solution where possible for these two cases:

• Methods/Fields/Enums that are only used in testing, instead of prefixing them with underscore, annotate them with #[cfg(test)]. This applies to some getters, debug functions, etc.
• Enums that have unused options: annotate just the enum with allow_dead_code. I believe this will look cleaner.

I think we should be more tolerant with allowing testing dead code than non-testing dead code. For example, I think it's ok to have a testing enum with options that are unused (like the StateCircuit enum to overwrite assignments). But then if an enum is used in non-testing setting and has a non-used case, perhaps it's better to remove the case (like the EVM Circuit StepLast).

What do you think about this?

[ed255]

For future reference and indexing purposes (so that we can search via github), here's a list of deleted EVM math gadgets in this PR:

• BatchedIsZeroGadget
• BinaryNumberGadget
• MinMaxWordGadget

[ChihChengLiang]

I would suggest a different solution where possible for these two cases:

• Methods/Fields/Enums that are only used in testing, instead of prefixing them with underscore, annotate them with #[cfg(test)]. This applies to some getters, debug functions, etc.
• Enums that have unused options: annotate just the enum with allow_dead_code. I believe this will look cleaner.

I think we should be more tolerant with allowing testing dead code than non-testing dead code. For example, I think it's ok to have a testing enum with options that are unused (like the StateCircuit enum to overwrite assignments). But then if an enum is used in non-testing setting and has a non-used case, perhaps it's better to remove the case (like the EVM Circuit StepLast).

There are reasons I like the underscore a lot more than #[cfg(test)]. and allow(dead_code). Generally speaking,

• underscore is a less verbose way and a very visible way to tell devs and compilers that a variable is unused.
• #[cfg(test)] comes with annoying side effects if a method depends on some imports, we have to mark that imports with #[cfg(test)]. For example, the overrides property in StateCircuit depends on HashMap, which requires us to mark #[cfg(test)] for use std::collections::HashMap;. The code would look noisy in this way. Generally, I see the scattering of #[cfg(test)] in the codebase as an anti-pattern. The desired way should be confining all test dependencies inside a module and we only mark the mod testdeps with the #[cfg(test)].
• "allow(dead_code)" is bad because it is overkill. When we mark an enum as dead_code, we don't know if the variants are dead code or if the whole enum is dead code. Again I believe like the cfg test, the allow(dead_code) should be only applied to the module level. An unstable module that is in active development is a suitable place for allow(dead_code).

[ed255]

There are reasons I like the underscore a lot more than #[cfg(test)]. and allow(dead_code). Generally speaking,

* underscore is a less verbose way and a very visible way to tell devs and compilers that a variable is unused.

* `#[cfg(test)]` comes with annoying side effects if a method depends on some imports, we have to mark that imports with `#[cfg(test)]`. For example, the `overrides` property in StateCircuit depends on `HashMap`, which requires us to mark `#[cfg(test)]` for `use std::collections::HashMap;`. The code would look noisy in this way. Generally, I see the scattering of `#[cfg(test)]` in the codebase as an anti-pattern. The desired way should be confining all test dependencies inside a module and we only mark the `mod testdeps` with the `#[cfg(test)]`.

* "allow(dead_code)" is bad because it is overkill. When we mark an enum as dead_code, we don't know if the variants are dead code or if the whole enum is dead code. Again I believe like the cfg test, the allow(dead_code) should be only applied to the module level. An unstable module that is in active development is a suitable place for allow(dead_code).

Thanks for the detailed reasoning! I didn't think about the messy imports coming from having #[cfg(test)] on things that are not modules; the code gets very messy to manage.

I'm now convinced about this approach :)

[ed255]

LGTM! Thanks for taking care of this clean up :D

[hero78119]

It's good we cleanup quite lot of unused methods.
However, I think there are better interpretation for methods left for now comparing with just purely prefix with "_"

We can add #[allow(dead_code, reason = "XXXX")] specificly on field/method to explain why we allow it's there. I randomly select few typical case in comments.

Add-on, to use reason="" in allow linter, we just need to add
#![feature(lint_reasons)]
in src/lib.rs

wdyt~?

[ChihChengLiang]

Hi @hero78119, please see my previous reasoning with Edu about why I refrain from using allow(dead_code)

#1525 (review)

I like the idea of having the lint reasons feature #![feature(lint_reasons)]. But if we removed all allow(dead_code), then we don't need it in this PR.

[ChihChengLiang]

Sorry, turns out the _keccak_table indicates something wrong about loading the table. Let me fix it soon.

[hero78119]

I read through the comment before I proposed the allow(dead_code, reason="xxxxx") method, and totally agreed with viewpoint on cfg(test) as it force the depends respective unnessesary and messed changes.

• "allow(dead_code)" is bad because it is overkill. When we mark an enum as dead_code, we don't know if the variants are dead code or if the whole enum is dead code. Again I believe like the cfg test, the allow(dead_code) should be only applied to the module level. An unstable module that is in active development is a suitable place for allow(dead_code).

For enum example, I think to clarify the variants are dead code or if the whole enum is dead code, it can be clearly explained by "reason", comparing with purely adding prefix "_", I think our meaningful insight are lost because by peeking on "_"foo the reader if not following this pull-request discussion, reader still confuse why adding prefix underscore here? does this underscore means it's a private variable not being used? and use somewhere under conditional features compiling? and why this method only used in conditional features compiling, because it's a debugging method? or just a method which we expect to be used in the future? So many thoughts in mind and the only way is to read through the whole codebase thoroughly to know the reason.

In my option, left a reason here is more meaningful than just prefix underscore. It's not only resolve the linter complains but also retain the meaningful insights.

I dont think dead_code should only applied on module level (otherwise rust wont allow it put in the place other than modules), but admit allow(dead_code) looks scared on its meaning. However compare with prefix, I voted +1 once it combine with reason.
There are better lint proposed wordings rust-lang/rust#54503 (comment)

#[suppress(unused, reason = "foo and bar and baz")]

but haven't finally adopted.

[ChihChengLiang]

@hero78119 Thanks for expanding on the reasoning.

For public or public(crate) methods, I think #[allow(dead_code, reason = "useful debug function")] makes sense. It clarifies why the method is unused more than the underscore way.

I'm still not convinced the same can be applied to Enum's variants. For the the variants are dead code or if the whole enum is dead code problem, I want the compiler to determine it. The reason field only explains to humans.

[hero78119]

Thanks for the review adoption on public method:) It looks really nice now 💯

Enum case is indeed overkill, and underscore prefix make more sense yes.

Thanks again for the big effort 👍