steve/issue 7 ci (#15)

rustfmt and clippy check, with skip check option for markdown files and
docs

rustfmt check taken from zkEVM

@leolara These edits change the source code (by fixing warnings and
conforming to rustfmt). Most are cosmetic, but I'd still suggest you
review them. Thanks!

---------

Co-authored-by: Steve Wang <qwang97@wharton.upenn.edu>
Co-authored-by: Chih Cheng Liang <chihchengliang@gmail.com>

.github/workflows/rust.yml

@@ -10,13 +10,75 @@ env:
   CARGO_TERM_COLOR: always
 
 jobs:
-  build:
+  skip_check:
-
     runs-on: ubuntu-latest
+    outputs:
+      should_skip: ${{ steps.skip_check.outputs.should_skip }}
+    steps:
+      - id: skip_check
+        uses: fkirc/skip-duplicate-actions@v5
+        with:
+          cancel_others: 'true'
+          concurrent_skipping: 'same_content_newer'
+          paths_ignore: '["**/README.md", "**/Docs**", "**/Appendix.md"]'
 
+  build:
+    needs: [skip_check]
+    runs-on: ubuntu-latest
     steps:
     - uses: actions/checkout@v3
     - name: Build
       run: cargo build --verbose
     - name: Run tests
       run: cargo test --verbose
+
+  clippy_check:
+    needs: [skip_check]
+    name: Clippy Check
+    runs-on: ubuntu-latest
+    steps:
+      - name: Checkout repository
+        uses: actions/checkout@v2
+      - name: Setup Rust
+        uses: actions-rs/toolchain@v1
+        with:
+          profile: minimal
+          toolchain: stable
+          components: clippy
+          override: true
+      - name: Run Clippy
+        run: cargo clippy --all-targets --all-features -- -D warnings
+
+  fmt:
+    needs: [skip_check]
+    if: |
+      github.event.pull_request.draft == false &&
+      (github.event.action == 'ready_for_review' || needs.skip_check.outputs.should_skip != 'true')
+    name: Rustfmt
+    timeout-minutes: 30
+    runs-on: ubuntu-latest
+    steps:
+      - uses: actions/checkout@v2
+      - uses: actions-rs/toolchain@v1
+        with:
+          override: false
+      - name: Cargo cache
+        uses: actions/cache@v3
+        with:
+          path: |
+            ~/.cargo/bin/
+            ~/.cargo/registry/index/
+            ~/.cargo/registry/cache/
+            ~/.cargo/git/db/
+            target/
+          key: ${{ runner.os }}-cargo-${{ hashFiles('**/Cargo.lock') }}
+      - name: cargo check
+        uses: actions-rs/cargo@v1
+        with:
+          command: check
+          args: --all-features
+      - run: rustup component add rustfmt
+      - uses: actions-rs/cargo@v1
+        with:
+          command: fmt
+          args: --all -- --check

examples/bytecode.rs

@@ -27,7 +27,7 @@ impl<F: FieldExt> IsZero<F> {
         let value: Expr<F> = value.into();
         let is_zero_expression = (1.expr() - value.clone()) * value_inv;
 
-        ctx.constr(value.clone() * is_zero_expression.clone());
+        ctx.constr(value * is_zero_expression.clone());
 
         IsZero {
             value_inv,
@@ -48,37 +48,33 @@ impl<F: Field> IsZero<F> {
 
 #[derive(Debug)]
 struct BytecodeLine<F: Debug> {
-    hash: F,
+    _hash: F,
-    index: F,
+    _index: F,
-    length: F,
+    _length: F,
-    is_code: F,
+    _is_code: F,
-    value: F,
+    _value: F,
-    push_data_left: F,
+    _push_data_left: F,
 }
 
 fn main() {
-    let mut bytecode_circuit =
+    let bytecode_circuit =
         circuit::<Fr, Vec<Vec<BytecodeLine<Fr>>>, BytecodeLine<Fr>, _>("bytecode circuit", |ctx| {
             use chiquito::dsl::cb::*;
 
             let length = ctx.forward("length");
             let index = ctx.forward("index");
-            let hash = ctx.forward("hash");
             let is_code = ctx.forward("is_code");
             let value = ctx.forward("value");
-            let value_rlc = ctx.forward("value_rlc");
 
             let s1 = ctx.step_type("header");
             ctx.step_type_def(s1, |ctx| {
                 ctx.constr(isz(index));
                 ctx.constr(eq(value, length));
-
-                ctx.wg(|ctx, v| {})
             });
             let s2 = ctx.step_type("byte");
             ctx.step_type_def(s2, |ctx| {
                 let push_data_left = ctx.internal("push_data_left");
-                let push_data_size = ctx.internal("push_data_size");
+                // let push_data_size = ctx.internal("push_data_size");
                 let push_data_left_inv = ctx.internal("push_data_left_inv");
 
                 let push_data_left_is_zero = IsZero::setup(ctx, push_data_left, push_data_left_inv);
@@ -95,12 +91,6 @@ fn main() {
                     push_data_left_is_zero.wg(ctx, Fr::zero());
                 });
             });
-
-            ctx.trace(|ctx, bytecodes| {
-                for bytecode in bytecodes {
-                    println!("todo");
-                }
-            })
         });
 
     let compiler = Compiler::new(SingleRowCellManager {}, SimpleStepSelectorBuilder {});

examples/example.rs

@@ -3,7 +3,7 @@ use chiquito::{
     compiler::{
         cell_manager::SingleRowCellManager, step_selector::SimpleStepSelectorBuilder, Compiler,
     },
-    dsl::{circuit, cb::lookup},
+    dsl::{cb::lookup, circuit},
 };
 use halo2_proofs::halo2curves::bn256::Fr;
 
@@ -23,7 +23,7 @@ fn main() {
             ctx.transition(a + 1);
 
             ctx.add_lookup(lookup().add(a, b).enable(d).add(d + a, f * c));
-            
+
             ctx.wg(move |ctx, _| {
                 ctx.assign(a, 13.field());
                 ctx.assign(b, 13.field());

examples/fibonacci.rs

@@ -108,32 +108,21 @@ fn fibo_circuit<F: FieldExt>() -> Circuit<F, (), (u64, u64)> {
             let mut a = 1;
             let mut b = 2;
 
-            for i in 1..10 {
+            for _i in 1..10 {
-                // input values for witness generation function are (a, b) in this step instance
                 ctx.add(&fibo_step, (a, b));
 
                 let prev_a = a;
                 a = b;
-                b = prev_a + b;
+                b += prev_a;
             }
             
             ctx.add(&fibo_last_step, (a, b));
         })
     });
-    
+
-    // uncomment for debugging:
-    // println!("{:#?}", fibo);
-    
-    // create a new compiler using a cell manager instance and a selector builder instance
     let compiler = Compiler::new(SingleRowCellManager {}, SimpleStepSelectorBuilder {});
-    
-    // compile the circuit into an IR object
-    let compiled = compiler.compile(&fibo);
 
-    // uncomment for debugging:
+    compiler.compile(&fibo)
-    //println!("{:#?}", compiled);
-
-    compiled
 }
 
 // After compiling Chiquito AST to an IR, it is further parsed by a Chiquito Halo2 backend and integrated into a Halo2 circuit,

rustfmt.toml

@@ -0,0 +1,9 @@
+edition = "2021"
+
+comment_width = 100
+imports_granularity = "Crate"
+max_width = 100
+newline_style = "Unix"
+normalize_comments = true
+reorder_imports = true
+wrap_comments = true

src/ast.rs

@@ -1,11 +1,12 @@
 pub mod expr;
 
-use std::fmt::Debug;
+use std::{collections::HashMap, fmt::Debug, rc::Rc};
-use std::{collections::HashMap, rc::Rc};
 
-use crate::compiler::{FixedGenContext, TraceContext, WitnessGenContext};
+use crate::{
-use crate::dsl::StepTypeHandler;
+    compiler::{FixedGenContext, TraceContext, WitnessGenContext},
-use crate::util::uuid;
+    dsl::StepTypeHandler,
+    util::uuid,
+};
 
 pub use expr::*;
 
@@ -256,8 +257,9 @@ impl<F> Default for Lookup<F> {
 }
 
 impl<F: Debug + Clone> Lookup<F> {
-    // Function: adds (constraint, expression) to exprs if there's no enabler, OR add (enabler * constraint, expression) to exprs if there's enabler
+    // Function: adds (constraint, expression) to exprs if there's no enabler, OR add (enabler *
-    // Note that constraint_annotation and constraint_expr are passed in as separate parameters, and then reconstructed as Constraint,
+    // constraint, expression) to exprs if there's enabler Note that constraint_annotation and
+    // constraint_expr are passed in as separate parameters, and then reconstructed as Constraint,
     // because dsl uses cb::Constraint while ast uses ast::Constraint
     pub fn add(
         &mut self,
@@ -283,7 +285,8 @@ impl<F: Debug + Clone> Lookup<F> {
         }
     }
 
-    // Function: setup the enabler field and multiply all LHS constraints by the enabler if there's no enabler, OR panic if there's an enabler already
+    // Function: setup the enabler field and multiply all LHS constraints by the enabler if there's
+    // no enabler, OR panic if there's an enabler already
     pub fn enable(&mut self, enable_annotation: String, enable_expr: Expr<F>) {
         let enable = Constraint {
             annotation: enable_annotation.clone(),
@@ -305,7 +308,11 @@ impl<F: Debug + Clone> Lookup<F> {
     // Function: helper function for multiplying enabler to constraint
     fn multiply_constraints(enable: Constraint<F>, constraint: Constraint<F>) -> Constraint<F> {
         Constraint {
-            annotation: constraint.annotation.clone(), // annotation only takes the constraint's annotation, because enabler's annotation is already included in the enable function above in the format of "if {enable}"
+            annotation: constraint.annotation.clone(), /* annotation only takes the constraint's
+                                                        * annotation, because enabler's
+                                                        * annotation is already included in the
+                                                        * enable function above in the format of
+                                                        * "if {enable}" */
             expr: enable.expr * constraint.expr,
         }
     }

src/ast/expr.rs

@@ -1,5 +1,7 @@
-use std::fmt::Debug;
+use std::{
-use std::ops::{Add, Mul, Neg, Sub};
+    fmt::Debug,
+    ops::{Add, Mul, Neg, Sub},
+};
 
 use halo2_proofs::{arithmetic::Field, plonk::Expression};
 

src/backend/halo2.rs

@@ -19,8 +19,9 @@ use crate::{
     },
     dsl::StepTypeHandler,
     ir::{
-        Circuit, Column as cColumn, ColumnType::Advice as cAdvice, ColumnType::Fixed as cFixed,
+        Circuit, Column as cColumn,
-        ColumnType::Halo2Advice, ColumnType::Halo2Fixed, PolyExpr,
+        ColumnType::{Advice as cAdvice, Fixed as cFixed, Halo2Advice, Halo2Fixed},
+        PolyExpr,
     },
 };
 
@@ -226,7 +227,9 @@ impl<F: FieldExt, TraceArgs, StepArgs: Clone> ChiquitoHalo2<F, TraceArgs, StepAr
             .expect("q_enable column not found");
 
         for i in 0..height {
-            region.assign_fixed(|| "q_enable=1", *q_enable, i, || Value::known(F::one()));
+            region
+                .assign_fixed(|| "q_enable=1", *q_enable, i, || Value::known(F::one()))
+                .expect("assignment of q_enable fixed column failed");
         }
 
         if let Some(q_first) = self.circuit.q_first.clone() {
@@ -235,7 +238,9 @@ impl<F: FieldExt, TraceArgs, StepArgs: Clone> ChiquitoHalo2<F, TraceArgs, StepAr
                 .get(&q_first.uuid())
                 .expect("q_enable column not found");
 
-            region.assign_fixed(|| "q_first=1", *q_first, 0, || Value::known(F::one()));
+            region
+                .assign_fixed(|| "q_first=1", *q_first, 0, || Value::known(F::one()))
+                .expect("assignment of q_first fixed column failed");
         }
 
         if let Some(q_last) = self.circuit.q_last.clone() {
@@ -244,12 +249,14 @@ impl<F: FieldExt, TraceArgs, StepArgs: Clone> ChiquitoHalo2<F, TraceArgs, StepAr
                 .get(&q_last.uuid())
                 .expect("q_enable column not found");
 
-            region.assign_fixed(
+            region
-                || "q_first=1",
+                .assign_fixed(
-                *q_last,
+                    || "q_first=1",
-                height - 1,
+                    *q_last,
-                || Value::known(F::one()),
+                    height - 1,
-            );
+                    || Value::known(F::one()),
+                )
+                .expect("assignment of q_last fixed column failed");
         }
     }
 

src/compiler.rs

@@ -1,8 +1,10 @@
 use core::fmt::Debug;
 use std::{collections::HashMap, rc::Rc};
 
-use halo2_proofs::plonk::Column as Halo2Column;
+use halo2_proofs::{
-use halo2_proofs::{arithmetic::Field, plonk::Advice};
+    arithmetic::Field,
+    plonk::{Advice, Column as Halo2Column},
+};
 
 use crate::{
     ast::{

src/compiler/step_selector.rs

@@ -1,7 +1,9 @@
 use std::{collections::HashMap, rc::Rc};
 
-use halo2_proofs::plonk::Column as Halo2Column;
+use halo2_proofs::{
-use halo2_proofs::{arithmetic::Field, plonk::Advice};
+    arithmetic::Field,
+    plonk::{Advice, Column as Halo2Column},
+};
 
 use crate::ast::StepType;
 

src/dsl.rs

@@ -1,5 +1,5 @@
 use crate::{
-    ast::{query::Queriable, Circuit, Expr, StepType, StepTypeUUID, Lookup},
+    ast::{query::Queriable, Circuit, StepType, StepTypeUUID},
     compiler::{FixedGenContext, TraceContext, WitnessGenContext},
     util::uuid,
 };