From d0daca2da23b39508f641745db35546d18c6326c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fran=C3=A7ois=20Garillot?= <4142+huitseeker@users.noreply.github.com> Date: Wed, 27 Sep 2023 08:00:14 -0400 Subject: [PATCH 1/2] refactor: simplify type aliases in tests (#54) Generic type aliases are type functions. In our case, they are made distinct through the arguments they are passed, not by giving them different names. --- src/lib.rs | 21 +++++++++------------ 1 file changed, 9 insertions(+), 12 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index 870d518b..cf206444 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -812,12 +812,9 @@ mod tests { use core::fmt::Write; use super::*; - type EE1 = provider::ipa_pc::EvaluationEngine; - type EE2 = provider::ipa_pc::EvaluationEngine; - type S1 = spartan::snark::RelaxedR1CSSNARK>; - type S2 = spartan::snark::RelaxedR1CSSNARK>; - type S1Prime = spartan::ppsnark::RelaxedR1CSSNARK>; - type S2Prime = spartan::ppsnark::RelaxedR1CSSNARK>; + type EE = provider::ipa_pc::EvaluationEngine; + type S = spartan::snark::RelaxedR1CSSNARK>; + type SPrime = spartan::ppsnark::RelaxedR1CSSNARK>; use ::bellpepper_core::{num::AllocatedNum, ConstraintSystem, SynthesisError}; use core::marker::PhantomData; @@ -1157,10 +1154,10 @@ mod tests { assert_eq!(zn_secondary, vec![::Scalar::from(2460515u64)]); // produce the prover and verifier keys for compressed snark - let (pk, vk) = CompressedSNARK::<_, _, _, _, S1, S2>::setup(&pp).unwrap(); + let (pk, vk) = CompressedSNARK::<_, _, _, _, S, S>::setup(&pp).unwrap(); // produce a compressed SNARK - let res = CompressedSNARK::<_, _, _, _, S1, S2>::prove(&pp, &pk, &recursive_snark); + let res = CompressedSNARK::<_, _, _, _, S, S>::prove(&pp, &pk, &recursive_snark); assert!(res.is_ok()); let compressed_snark = res.unwrap(); @@ -1253,11 +1250,11 @@ mod tests { // run the compressed snark with Spark compiler // produce the prover and verifier keys for compressed snark - let (pk, vk) = CompressedSNARK::<_, _, _, _, S1Prime, S2Prime>::setup(&pp).unwrap(); + let (pk, vk) = CompressedSNARK::<_, _, _, _, SPrime, SPrime>::setup(&pp).unwrap(); // produce a compressed SNARK let res = - CompressedSNARK::<_, _, _, _, S1Prime, S2Prime>::prove(&pp, &pk, &recursive_snark); + CompressedSNARK::<_, _, _, _, SPrime, SPrime>::prove(&pp, &pk, &recursive_snark); assert!(res.is_ok()); let compressed_snark = res.unwrap(); @@ -1404,10 +1401,10 @@ mod tests { assert!(res.is_ok()); // produce the prover and verifier keys for compressed snark - let (pk, vk) = CompressedSNARK::<_, _, _, _, S1, S2>::setup(&pp).unwrap(); + let (pk, vk) = CompressedSNARK::<_, _, _, _, S, S>::setup(&pp).unwrap(); // produce a compressed SNARK - let res = CompressedSNARK::<_, _, _, _, S1, S2>::prove(&pp, &pk, &recursive_snark); + let res = CompressedSNARK::<_, _, _, _, S, S>::prove(&pp, &pk, &recursive_snark); assert!(res.is_ok()); let compressed_snark = res.unwrap(); From 36d9f5baed6b95ed4b1829a75c140131392556bb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fran=C3=A7ois=20Garillot?= <4142+huitseeker@users.noreply.github.com> Date: Thu, 28 Sep 2023 08:49:38 -0400 Subject: [PATCH 2/2] Simplify map_or expressions, remove clippy override noise, improve doc (#57) * refactor: Rewrite some exact instances of `Result::ok()` * refactor: Increase Clippy lint complexity limits and remove overrides - Removed Clippy linting restrictions on complexity and argument count across several modules including r1cs, lib, supernova/circuit, and bellpepper/shape_cs among others. - added .clippy.toml config file for project-wide limits * doc: Improve markdown formatting - Improved code documentation throughout by adding markdown styling and backticks for code readability. - No major functionality changes conducted, --- .clippy.toml | 2 ++ src/bellpepper/shape_cs.rs | 1 - src/bellpepper/test_shape_cs.rs | 1 - src/circuit.rs | 19 +++++++------------ src/digest.rs | 2 +- src/gadgets/r1cs.rs | 26 ++++++-------------------- src/lib.rs | 1 - src/nifs.rs | 3 --- src/provider/ipa_pc.rs | 1 - src/provider/secp_secq.rs | 2 +- src/r1cs.rs | 1 - src/spartan/polys/univariate.rs | 4 ++-- src/spartan/sumcheck.rs | 2 -- src/traits/circuit.rs | 2 +- 14 files changed, 20 insertions(+), 47 deletions(-) create mode 100644 .clippy.toml diff --git a/.clippy.toml b/.clippy.toml new file mode 100644 index 00000000..961117ac --- /dev/null +++ b/.clippy.toml @@ -0,0 +1,2 @@ +type-complexity-threshold = 9999 +too-many-arguments-threshold = 20 \ No newline at end of file diff --git a/src/bellpepper/shape_cs.rs b/src/bellpepper/shape_cs.rs index 9b57a30c..fde57868 100644 --- a/src/bellpepper/shape_cs.rs +++ b/src/bellpepper/shape_cs.rs @@ -10,7 +10,6 @@ pub struct ShapeCS where G::Scalar: PrimeField, { - #[allow(clippy::type_complexity)] /// All constraints added to the `ShapeCS`. pub constraints: Vec<( LinearCombination, diff --git a/src/bellpepper/test_shape_cs.rs b/src/bellpepper/test_shape_cs.rs index cf43a7a2..983ce2ca 100644 --- a/src/bellpepper/test_shape_cs.rs +++ b/src/bellpepper/test_shape_cs.rs @@ -55,7 +55,6 @@ where { named_objects: HashMap, current_namespace: Vec, - #[allow(clippy::type_complexity)] /// All constraints added to the `TestShapeCS`. pub constraints: Vec<( LinearCombination, diff --git a/src/circuit.rs b/src/circuit.rs index 5b3f2bde..01321094 100644 --- a/src/circuit.rs +++ b/src/circuit.rs @@ -59,7 +59,6 @@ pub struct NovaAugmentedCircuitInputs { impl NovaAugmentedCircuitInputs { /// Create new inputs/witness for the verification circuit - #[allow(clippy::too_many_arguments)] pub fn new( params: G::Scalar, i: G::Base, @@ -126,7 +125,7 @@ impl<'a, G: Group, SC: StepCircuit> NovaAugmentedCircuit<'a, G, SC> { // Allocate the params let params = alloc_scalar_as_base::( cs.namespace(|| "params"), - self.inputs.get().map_or(None, |inputs| Some(inputs.params)), + self.inputs.as_ref().map(|inputs| inputs.params), )?; // Allocate i @@ -154,9 +153,7 @@ impl<'a, G: Group, SC: StepCircuit> NovaAugmentedCircuit<'a, G, SC> { // Allocate the running instance let U: AllocatedRelaxedR1CSInstance = AllocatedRelaxedR1CSInstance::alloc( cs.namespace(|| "Allocate U"), - self.inputs.get().as_ref().map_or(None, |inputs| { - inputs.U.get().as_ref().map_or(None, |U| Some(U)) - }), + self.inputs.as_ref().and_then(|inputs| inputs.U.as_ref()), self.params.limb_width, self.params.n_limbs, )?; @@ -164,17 +161,16 @@ impl<'a, G: Group, SC: StepCircuit> NovaAugmentedCircuit<'a, G, SC> { // Allocate the instance to be folded in let u = AllocatedR1CSInstance::alloc( cs.namespace(|| "allocate instance u to fold"), - self.inputs.get().as_ref().map_or(None, |inputs| { - inputs.u.get().as_ref().map_or(None, |u| Some(u)) - }), + self.inputs.as_ref().and_then(|inputs| inputs.u.as_ref()), )?; // Allocate T let T = AllocatedPoint::alloc( cs.namespace(|| "allocate T"), - self.inputs.get().map_or(None, |inputs| { - inputs.T.get().map_or(None, |T| Some(T.to_coordinates())) - }), + self + .inputs + .as_ref() + .and_then(|inputs| inputs.T.map(|T| T.to_coordinates())), )?; Ok((params, i, z_0, z_i, U, u, T)) @@ -207,7 +203,6 @@ impl<'a, G: Group, SC: StepCircuit> NovaAugmentedCircuit<'a, G, SC> { /// Synthesizes non base case and returns the new relaxed `R1CSInstance` /// And a boolean indicating if all checks pass - #[allow(clippy::too_many_arguments)] fn synthesize_non_base_case::Base>>( &self, mut cs: CS, diff --git a/src/digest.rs b/src/digest.rs index c0b1e482..5eb0c990 100644 --- a/src/digest.rs +++ b/src/digest.rs @@ -58,7 +58,7 @@ impl<'a, F: PrimeField, T: Digestible> DigestComputer<'a, F, T> { digest } - /// Create a new DigestComputer + /// Create a new `DigestComputer` pub fn new(inner: &'a T) -> Self { DigestComputer { inner, diff --git a/src/gadgets/r1cs.rs b/src/gadgets/r1cs.rs index 49f3251e..6f9d444d 100644 --- a/src/gadgets/r1cs.rs +++ b/src/gadgets/r1cs.rs @@ -36,17 +36,11 @@ impl AllocatedR1CSInstance { // Check that the incoming instance has exactly 2 io let W = AllocatedPoint::alloc( cs.namespace(|| "allocate W"), - u.get().map_or(None, |u| Some(u.comm_W.to_coordinates())), + u.map(|u| u.comm_W.to_coordinates()), )?; - let X0 = alloc_scalar_as_base::( - cs.namespace(|| "allocate X[0]"), - u.get().map_or(None, |u| Some(u.X[0])), - )?; - let X1 = alloc_scalar_as_base::( - cs.namespace(|| "allocate X[1]"), - u.get().map_or(None, |u| Some(u.X[1])), - )?; + let X0 = alloc_scalar_as_base::(cs.namespace(|| "allocate X[0]"), u.map(|u| u.X[0]))?; + let X1 = alloc_scalar_as_base::(cs.namespace(|| "allocate X[1]"), u.map(|u| u.X[1]))?; Ok(AllocatedR1CSInstance { W, X0, X1 }) } @@ -80,24 +74,17 @@ impl AllocatedRelaxedR1CSInstance { ) -> Result { let W = AllocatedPoint::alloc( cs.namespace(|| "allocate W"), - inst - .get() - .map_or(None, |inst| Some(inst.comm_W.to_coordinates())), + inst.map(|inst| inst.comm_W.to_coordinates()), )?; let E = AllocatedPoint::alloc( cs.namespace(|| "allocate E"), - inst - .get() - .map_or(None, |inst| Some(inst.comm_E.to_coordinates())), + inst.map(|inst| inst.comm_E.to_coordinates()), )?; // u << |G::Base| despite the fact that u is a scalar. // So we parse all of its bytes as a G::Base element - let u = alloc_scalar_as_base::( - cs.namespace(|| "allocate u"), - inst.get().map_or(None, |inst| Some(inst.u)), - )?; + let u = alloc_scalar_as_base::(cs.namespace(|| "allocate u"), inst.map(|inst| inst.u))?; // Allocate X0 and X1. If the input instance is None, then allocate default values 0. let X0 = BigNat::alloc_from_nat( @@ -231,7 +218,6 @@ impl AllocatedRelaxedR1CSInstance { } /// Folds self with a relaxed r1cs instance and returns the result - #[allow(clippy::too_many_arguments)] pub fn fold_with_r1cs::Base>>( &self, mut cs: CS, diff --git a/src/lib.rs b/src/lib.rs index cf206444..770f6858 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -8,7 +8,6 @@ missing_docs )] #![allow(non_snake_case)] -#![allow(clippy::type_complexity)] #![forbid(unsafe_code)] // private modules diff --git a/src/nifs.rs b/src/nifs.rs index 2b8661ab..605cf06f 100644 --- a/src/nifs.rs +++ b/src/nifs.rs @@ -1,6 +1,5 @@ //! This module implements a non-interactive folding scheme #![allow(non_snake_case)] -#![allow(clippy::type_complexity)] use crate::{ constants::{NUM_CHALLENGE_BITS, NUM_FE_FOR_RO}, @@ -30,7 +29,6 @@ impl NIFS { /// a folded Relaxed R1CS instance-witness tuple `(U, W)` of the same shape `shape`, /// with the guarantee that the folded witness `W` satisfies the folded instance `U` /// if and only if `W1` satisfies `U1` and `W2` satisfies `U2`. - #[allow(clippy::too_many_arguments)] pub fn prove( ck: &CommitmentKey, ro_consts: &ROConstants, @@ -208,7 +206,6 @@ mod tests { test_tiny_r1cs_bellpepper_with::(); } - #[allow(clippy::too_many_arguments)] fn execute_sequence( ck: &CommitmentKey, ro_consts: &<::RO as ROTrait<::Base, ::Scalar>>::Constants, diff --git a/src/provider/ipa_pc.rs b/src/provider/ipa_pc.rs index b2de1a70..264cd3c2 100644 --- a/src/provider/ipa_pc.rs +++ b/src/provider/ipa_pc.rs @@ -1,5 +1,4 @@ //! This module implements `EvaluationEngine` using an IPA-based polynomial commitment scheme -#![allow(clippy::too_many_arguments)] use crate::{ errors::NovaError, provider::pedersen::CommitmentKeyExtTrait, diff --git a/src/provider/secp_secq.rs b/src/provider/secp_secq.rs index 4b8b5d14..bd604d25 100644 --- a/src/provider/secp_secq.rs +++ b/src/provider/secp_secq.rs @@ -1,4 +1,4 @@ -//! This module implements the Nova traits for secp::Point, secp::Scalar, secq::Point, secq::Scalar. +//! This module implements the Nova traits for `secp::Point`, `secp::Scalar`, `secq::Point`, `secq::Scalar`. use crate::{ impl_traits, provider::{ diff --git a/src/r1cs.rs b/src/r1cs.rs index 0e7726a2..85cb1b44 100644 --- a/src/r1cs.rs +++ b/src/r1cs.rs @@ -1,5 +1,4 @@ //! This module defines R1CS related types and a folding scheme for Relaxed R1CS -#![allow(clippy::type_complexity)] use crate::{ constants::{BN_LIMB_WIDTH, BN_N_LIMBS}, digest::{DigestComputer, SimpleDigestible}, diff --git a/src/spartan/polys/univariate.rs b/src/spartan/polys/univariate.rs index 29040b0b..afc50087 100644 --- a/src/spartan/polys/univariate.rs +++ b/src/spartan/polys/univariate.rs @@ -1,6 +1,6 @@ //! Main components: -//! - UniPoly: an univariate dense polynomial in coefficient form (big endian), -//! - CompressedUniPoly: a univariate dense polynomial, compressed (omitted linear term), in coefficient form (little endian), +//! - `UniPoly`: an univariate dense polynomial in coefficient form (big endian), +//! - `CompressedUniPoly`: a univariate dense polynomial, compressed (omitted linear term), in coefficient form (little endian), use ff::PrimeField; use rayon::prelude::{IntoParallelIterator, ParallelIterator}; use serde::{Deserialize, Serialize}; diff --git a/src/spartan/sumcheck.rs b/src/spartan/sumcheck.rs index 30f73edc..e3a733e9 100644 --- a/src/spartan/sumcheck.rs +++ b/src/spartan/sumcheck.rs @@ -1,5 +1,3 @@ -#![allow(clippy::too_many_arguments)] -#![allow(clippy::type_complexity)] use crate::errors::NovaError; use crate::spartan::polys::{ multilinear::MultilinearPolynomial, diff --git a/src/traits/circuit.rs b/src/traits/circuit.rs index 8ca3e513..d2cd2aae 100644 --- a/src/traits/circuit.rs +++ b/src/traits/circuit.rs @@ -12,7 +12,7 @@ pub trait StepCircuit: Send + Sync + Clone { fn arity(&self) -> usize; /// Sythesize the circuit for a computation step and return variable - /// that corresponds to the output of the step z_{i+1} + /// that corresponds to the output of the step `z_{i+1}` fn synthesize>( &self, cs: &mut CS,