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

Clippy gave different result depend on the version of compiler and clippy #178

Closed
chiro-hiro opened this issue Jun 6, 2023 · 2 comments · Fixed by #180
Closed

Clippy gave different result depend on the version of compiler and clippy #178

chiro-hiro opened this issue Jun 6, 2023 · 2 comments · Fixed by #180

Comments

@chiro-hiro
Copy link
Contributor

chiro-hiro commented Jun 6, 2023

cargo clippy --all-targets -- -D warnings this check on CI/CD and my computer are giving different result. Are there any work around to keep other people in the same page?.

rustc 1.70.0 (90c541806 2023-05-31)
clippy 0.1.70 (90c54180 2023-05-31)

Here is my result:

442 |     let mut acc = p.clone();
    |                    ^^^^^^^^ help: remove this
    |
note: cloned value is neither consumed nor mutated
   --> src/gadgets/ecc.rs:442:19
    |
442 |     let mut acc = p.clone();
    |                   ^^^^^^^^^
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#redundant_clone
note: the lint level is defined here
   --> src/lib.rs:3:3
    |
3   |   warnings,
    |   ^^^^^^^^
    = note: `#[deny(clippy::redundant_clone)]` implied by `#[deny(warnings)]`

error: could not compile `nova-snark` (lib) due to previous error
warning: build failed, waiting for other jobs to finish...
error: redundant clone
    --> src/gadgets/ecc.rs:1070:18
     |
1070 |     let mut b = a.clone();
     |                  ^^^^^^^^ help: remove this
     |
note: cloned value is neither consumed nor mutated
    --> src/gadgets/ecc.rs:1070:17
     |
1070 |     let mut b = a.clone();
     |                 ^^^^^^^^^
     = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#redundant_clone

error: could not compile `nova-snark` (lib test) due to 2 previous errors
@huitseeker
Copy link
Contributor

It's normal for clippy to give different results on different versions of the compiler, and CI is currently testing on the latest stable, which changed 5 days ago: https://releases.rs/docs/1.70.0/
The solution would be to add a toolchain file to this repo and update it regularly:
https://rust-lang.github.io/rustup/overrides.html#the-toolchain-file

@huitseeker
Copy link
Contributor

Also, in this case, clippy seems to be wrong, see:
https://gist.github.com/huitseeker/91ceb640943a5cf221552766f24d1c76
(i.e. there's a couple of undetected borrow-after-moves that prevent the straightforward fix)

huitseeker added a commit to huitseeker/Nova that referenced this issue Dec 15, 2023
* fix: correct bench group typo

* refactor: Refactor snark benchmark code to remove duplication

- Constants have been introduced to replace hard coded values previously used in the verifier circuit and number of samples.
- `bench_compressed_snark` function has been restructured, with a new `bench_compressed_snark_internal` function introduced to reduce duplication.
- Similar refactoring has also been applied to `bench_compressed_snark_with_computational_commitments`, ensuring uniformity in the code and decreasing duplication.
- The benchmark function has been updated to incorporate the use of the new constants as well as the refactored functions.

* refactor: Refactor Supernova SNARK benchmarks for code reuse

- Refactored benchmarking code in `compressed-snark-supernova.rs` for improved reuse and readability.
- Introduced the use of constants for constant values.
- Replaced hardcoded assignments with references to declared constants.
- Eliminated duplicated benchmarking codes and introduced a shared function for increased efficiency.

* refactor: Refactor recursive SNARK benchmarking code

- Improved the benchmarking system by introducing a new function `bench_recursive_snark_internal_with_arity`, which offers code reuse.
- Replaced a hardcoded number with a constant (`NUM_CONS_VERIFIER_CITCUIT_PRIMARY`) and introduced a new constant (`NUM_SAMPLES`) for setting the benchmark group sample size,
- Refactored the functions `bench_one_augmented_circuit_recursive_snark` and `bench_two_augmented_circuit_recursive_snark` to reduce code redundancy and improve maintainability.
- Removed unnecessary assertions and error handling process with more concise `.expect` methods.

* refactor: Refactor constants in recursive-snark.rs

- Introduced named constants (`NUM_CONS_VERIFIER_CIRCUIT_PRIMARY` and `NUM_SAMPLES`) for an enhancement of readability and maintainability.
- Improved the benchmarking function through the replacement of hard-coded values with the newly initialized constants.

* fix: update num_cons_verifier_circuit_primary

* test: Refine supernova recursive circuit tests and benchmarks

- Updated the `NUM_CONS_VERIFIER_CIRCUIT_PRIMARY` constant value in both `recursive-snark-supernova` and `compressed-snark-supernova` benchmarks for accurate evaluation.
- Enhanced the documentation for `NUM_CONS_VERIFIER_CIRCUIT_PRIMARY` with detailed comments explaining its correlation with `test_supernova_recursive_circuit_pasta` and `num_augmented_circuits`.
- Extended the `src/supernova/circuit.rs` module with a new test suite and additional functions to test supernova recursive circuits,

* fix: also update constraint values in the loop

* refactor: Refine public parameter size hints in compressed supernova bench

- Updated public params sizing in `compressed-snark-supernova.rs`.
srinathsetty pushed a commit that referenced this issue Dec 15, 2023
* fix: correct bench group typo

* refactor: Refactor snark benchmark code to remove duplication

- Constants have been introduced to replace hard coded values previously used in the verifier circuit and number of samples.
- `bench_compressed_snark` function has been restructured, with a new `bench_compressed_snark_internal` function introduced to reduce duplication.
- Similar refactoring has also been applied to `bench_compressed_snark_with_computational_commitments`, ensuring uniformity in the code and decreasing duplication.
- The benchmark function has been updated to incorporate the use of the new constants as well as the refactored functions.

* refactor: Refactor Supernova SNARK benchmarks for code reuse

- Refactored benchmarking code in `compressed-snark-supernova.rs` for improved reuse and readability.
- Introduced the use of constants for constant values.
- Replaced hardcoded assignments with references to declared constants.
- Eliminated duplicated benchmarking codes and introduced a shared function for increased efficiency.

* refactor: Refactor recursive SNARK benchmarking code

- Improved the benchmarking system by introducing a new function `bench_recursive_snark_internal_with_arity`, which offers code reuse.
- Replaced a hardcoded number with a constant (`NUM_CONS_VERIFIER_CITCUIT_PRIMARY`) and introduced a new constant (`NUM_SAMPLES`) for setting the benchmark group sample size,
- Refactored the functions `bench_one_augmented_circuit_recursive_snark` and `bench_two_augmented_circuit_recursive_snark` to reduce code redundancy and improve maintainability.
- Removed unnecessary assertions and error handling process with more concise `.expect` methods.

* refactor: Refactor constants in recursive-snark.rs

- Introduced named constants (`NUM_CONS_VERIFIER_CIRCUIT_PRIMARY` and `NUM_SAMPLES`) for an enhancement of readability and maintainability.
- Improved the benchmarking function through the replacement of hard-coded values with the newly initialized constants.

* fix: update num_cons_verifier_circuit_primary

* test: Refine supernova recursive circuit tests and benchmarks

- Updated the `NUM_CONS_VERIFIER_CIRCUIT_PRIMARY` constant value in both `recursive-snark-supernova` and `compressed-snark-supernova` benchmarks for accurate evaluation.
- Enhanced the documentation for `NUM_CONS_VERIFIER_CIRCUIT_PRIMARY` with detailed comments explaining its correlation with `test_supernova_recursive_circuit_pasta` and `num_augmented_circuits`.
- Extended the `src/supernova/circuit.rs` module with a new test suite and additional functions to test supernova recursive circuits,

* fix: also update constraint values in the loop

* refactor: Refine public parameter size hints in compressed supernova bench

- Updated public params sizing in `compressed-snark-supernova.rs`.
huitseeker added a commit to huitseeker/Nova that referenced this issue Dec 18, 2023
* fix: correct bench group typo

* refactor: Refactor snark benchmark code to remove duplication

- Constants have been introduced to replace hard coded values previously used in the verifier circuit and number of samples.
- `bench_compressed_snark` function has been restructured, with a new `bench_compressed_snark_internal` function introduced to reduce duplication.
- Similar refactoring has also been applied to `bench_compressed_snark_with_computational_commitments`, ensuring uniformity in the code and decreasing duplication.
- The benchmark function has been updated to incorporate the use of the new constants as well as the refactored functions.

* refactor: Refactor Supernova SNARK benchmarks for code reuse

- Refactored benchmarking code in `compressed-snark-supernova.rs` for improved reuse and readability.
- Introduced the use of constants for constant values.
- Replaced hardcoded assignments with references to declared constants.
- Eliminated duplicated benchmarking codes and introduced a shared function for increased efficiency.

* refactor: Refactor recursive SNARK benchmarking code

- Improved the benchmarking system by introducing a new function `bench_recursive_snark_internal_with_arity`, which offers code reuse.
- Replaced a hardcoded number with a constant (`NUM_CONS_VERIFIER_CITCUIT_PRIMARY`) and introduced a new constant (`NUM_SAMPLES`) for setting the benchmark group sample size,
- Refactored the functions `bench_one_augmented_circuit_recursive_snark` and `bench_two_augmented_circuit_recursive_snark` to reduce code redundancy and improve maintainability.
- Removed unnecessary assertions and error handling process with more concise `.expect` methods.

* refactor: Refactor constants in recursive-snark.rs

- Introduced named constants (`NUM_CONS_VERIFIER_CIRCUIT_PRIMARY` and `NUM_SAMPLES`) for an enhancement of readability and maintainability.
- Improved the benchmarking function through the replacement of hard-coded values with the newly initialized constants.

* fix: update num_cons_verifier_circuit_primary

* test: Refine supernova recursive circuit tests and benchmarks

- Updated the `NUM_CONS_VERIFIER_CIRCUIT_PRIMARY` constant value in both `recursive-snark-supernova` and `compressed-snark-supernova` benchmarks for accurate evaluation.
- Enhanced the documentation for `NUM_CONS_VERIFIER_CIRCUIT_PRIMARY` with detailed comments explaining its correlation with `test_supernova_recursive_circuit_pasta` and `num_augmented_circuits`.
- Extended the `src/supernova/circuit.rs` module with a new test suite and additional functions to test supernova recursive circuits,

* fix: also update constraint values in the loop

* refactor: Refine public parameter size hints in compressed supernova bench

- Updated public params sizing in `compressed-snark-supernova.rs`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants