chore: fix all lints #100

eightfilms · 2024-06-05T14:38:43Z

There's a lot of changes - I'll briefly summarize what I did:

I ran, as much as possible, the following:

cargo fix --all --allow-dirty
cargo fmt
cargo clippy --fix --all --allow--dirty

Some parts, I had to manually fix, or was out of scope of the current PR, so I added lint allowances + TODOs instead. An example of such 'out of scope' fixes include the unused_must_uses in r1cs/snarkjs.rs - these would require additional work on error handling which should probably belong in its own PR. For these cases, I prefer lint allowances since they allow us to easily find the spots where we want to fix later on.

Note that some of the lint allowances seem sane, so I didn't add TODOs for those, but only for those I felt would improve code quality if removed.

src/backends/kimchi/asm.rs

mimoo · 2024-06-05T20:15:02Z

src/backends/kimchi/prover.rs

@@ -62,6 +62,7 @@ pub struct VerifierIndex {
 // Setup
 //

+#[allow(clippy::type_complexity)]


I guess we should create an issue on removing all the #[allow.(.. ^^

mimoo · 2024-06-05T20:16:08Z

src/backends/r1cs/mod.rs

@@ -1,3 +1,7 @@
+// TODO: There is a bunch of places where there are unused vars.
+// Remove this lint allowance when fixed.
+#![allow(unused_variables)]


imo this is dangerous, as these unused variables might be bugs and we should address that instead of silencing it

Created #101

src/backends/r1cs/mod.rs

src/backends/r1cs/snarkjs.rs

mimoo

mmm, I've always felt like some bugs could be hiding behind the unused variables or unused returned values, so I don't think we should silence them. The compiler warnings should be a daily reminder that we need to address them :D so I propose to not silence anything. I'm happy if this PR is a bunch of benign clippy fixes, but it might be too early to enable clippy in CI

(so I guess, we could have a global issue where people can tackle these one at a time)

eightfilms · 2024-06-06T02:26:33Z

@mimoo agreed on compiler warnings being useful to address possible critical bugs, tackling this PR has helped discover these problems. We can hold off on this PR for now and create issues to handle the more important warnings such as the unused results or variables, so that this PR ends up being just simple fixes that are just simple QoL changes.

mimoo · 2024-06-14T17:09:44Z

I was thinking, if you remove anything that's not in the scope of this PR, and it contains only easy fixes to clippy, then we can merge it in : o

eightfilms · 2024-06-14T17:29:52Z

@mimoo makes sense, I'll remove the ones you mentioned. Seems like the easy fixes are:

usage of panic! to assert!
doc comments
~~#[must_use] annotations~~
other minor one-time fixes

mimoo · 2024-06-14T17:31:46Z

I thought the must_use were false positives? they don't seem correct

There's a lot of changes - I'll briefly summarize what I did: I ran, as much as possible, the following: ```rust cargo fix --all --allow-dirty cargo fmt cargo clippy --fix --all --allow--dirty ``` Some parts, I had to manually fix, or was out of scope of the current PR, so I added lint allowances + TODOs instead. An example of such 'out of scope' fixes include the `unused_must_use`s in `r1cs/snarkjs.rs` - these would require additional work on error handling which should probably belong in its own PR. For these cases, I prefer lint allowances since they allow us to easily find the spots where we want to fix later on. Note that some of the lint allowances seem sane, so I didn't add TODOs for those, but only for those I felt would improve code quality if removed.

eightfilms · 2024-06-16T15:48:49Z

src/type_checker/mod.rs

+    // TODO: Pass in `String`, instead of `&str`, so we don't hide an
+    // allocation within.
+    pub fn new(module: &ModulePath, name: &str) -> Self {
        let module = match module {
            ModulePath::Local => None,
            ModulePath::Alias(_) => unreachable!(),
            ModulePath::Absolute(user_repo) => Some(user_repo.clone()),
        };
        Self {
            module,
-            name: name.clone(),
+            name: name.to_string(),


imo name should be passed as a String, so that we don't do a clone internally here.

eightfilms · 2024-06-16T16:28:19Z

src/backends/mod.rs

+// TODO: Fix this by `Box`ing `KimchiVesta`.
+#[allow(clippy::large_enum_variant)]


eightfilms · 2024-06-16T16:30:19Z

I did a second pass through after removing all #[must_use] annotations, seems like most of the changes now are trivial changes.

.github/workflows/rust.yml

src/backends/kimchi/mod.rs

mimoo · 2024-06-16T21:54:59Z

src/backends/kimchi/mod.rs

-            let values = values.iter().map(|v| v.pretty()).join(" | ");
+            let values = values
+                .iter()
+                .map(super::super::helpers::PrettyField::pretty)


super::super o_O? just use crate::helpers::

Ah, this was an automatic clippy fix. Changed to PrettyField::pretty

src/backends/kimchi/mod.rs

src/backends/mod.rs

mimoo · 2024-07-16T13:46:15Z

src/imports.rs

    Span,
-) -> Result<Option<Var<B::Field, B::Var>>>;


what was the lint here?

mimoo · 2024-07-16T13:47:00Z

src/type_checker/checker.rs

@@ -210,7 +210,7 @@ impl<B: Backend> TypeChecker<B> {
                    }

                    // `struct.field = <rhs>`
-                    ExprKind::FieldAccess { lhs, rhs } => {


removal of variables shouldn't be in this PR as we want to make sure we're not forgetting to do something with them

mimoo · 2024-07-16T13:47:24Z

src/var.rs

@@ -13,7 +13,8 @@ use crate::{
 };

 /// The signature of a hint function
-pub type HintFn<B: Backend> = dyn Fn(&B, &mut WitnessEnv<B::Field>) -> Result<B::Field>;
+pub type HintFn<B> =
+    dyn Fn(&B, &mut WitnessEnv<<B as Backend>::Field>) -> Result<<B as Backend>::Field>;


same here, I'm not sure I understand this change

mimoo · 2024-07-16T13:49:21Z

src/cli/cmd_build_and_check.rs

@@ -73,7 +73,7 @@ pub fn cmd_build(args: CmdBuild) -> miette::Result<()> {
        .path
        .unwrap_or_else(|| std::env::current_dir().unwrap().try_into().unwrap());

-    let (sources, prover_index, verifier_index) = build(&curr_dir, args.asm, args.debug)?;
+    let (_sources, _prover_index, verifier_index) = build(&curr_dir, args.asm, args.debug)?;


pls remove these

mimoo · 2024-07-16T13:49:35Z

src/circuit_writer/writer.rs

@@ -99,7 +98,7 @@ impl PartialEq for AnnotatedCell {

 impl PartialOrd for AnnotatedCell {
    fn partial_cmp(&self, other: &Self) -> Option<std::cmp::Ordering> {
-        self.cell.partial_cmp(&other.cell)
+        Some(self.cmp(other))


why move from partial_cmp to cmp?

eightfilms · 2024-07-16T13:50:04Z

@mimoo Got caught up with other matters and left this PR in the dust. I will have time tomorrow to address your comments, sorry!

mimoo · 2024-07-16T13:51:35Z

src/circuit_writer/mod.rs

    pub fn generate_witness(
        &self,
        witness_env: &mut WitnessEnv<B::Field>,
    ) -> Result<B::GeneratedWitness> {
        self.backend.generate_witness(witness_env)
    }

+    #[allow(clippy::type_complexity)]


I'd rather not have these allow and be forced to find a way to fix it later

mimoo · 2024-07-16T13:51:45Z

src/circuit_writer/mod.rs

@@ -1,3 +1,7 @@
+// TODO: There is a bunch of places where there are unused vars.
+// Remove this lint allowance when fixed.
+#![allow(unused_variables)]


let's remove this

mimoo · 2024-07-16T13:52:34Z

sry for reviving this ^^ just pointed out more places that I think we should not fix for now

mimoo · 2024-10-23T15:27:05Z

hey sorry but let's pun on this for now, as I think there were too many potentially dangerous changes :o

eightfilms force-pushed the bing/fix-lints branch from 35a486e to b522b33 Compare June 5, 2024 14:41