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

chore: fixed clippy warning #304

Closed
wants to merge 1 commit into from
Closed

Conversation

RyanKung
Copy link
Contributor

@RyanKung RyanKung commented Feb 5, 2024

No description provided.

@RyanKung RyanKung closed this Feb 5, 2024
@RyanKung RyanKung reopened this Feb 5, 2024
@RyanKung
Copy link
Contributor Author

RyanKung commented Feb 5, 2024

@microsoft-github-policy-service agree [company="{Rings Network}"]

@RyanKung
Copy link
Contributor Author

RyanKung commented Feb 5, 2024

@microsoft-github-policy-service agree company="rings network"

@srinathsetty
Copy link
Collaborator

What command did you use to get the clippy warning? Clippy checks are part of the CI and there has been no error related to this.

@RyanKung
Copy link
Contributor Author

RyanKung commented Feb 7, 2024

@srinathsetty here is my clippy cmd

➜  Nova git:(main) cargo clippy --all --no-deps
    Checking nova-snark v0.34.0 (/Users/ryan/Dev/bns/Nova)
error: field `0` is never read
  --> src/bellpepper/test_shape_cs.rs:19:14
   |
19 |   Constraint(usize),
   |   ---------- ^^^^^
   |   |
   |   field in this variant
   |
note: the lint level is defined here
  --> src/lib.rs:4:3
   |
4  |   unused,
   |   ^^^^^^
   = note: `#[deny(dead_code)]` implied by `#[deny(unused)]`
help: consider changing the field to be of unit type to suppress this warning while preserving the field numbering, or remove the field
   |
19 |   Constraint(()),
   |              ~~

error: field `0` is never read
  --> src/bellpepper/test_shape_cs.rs:20:7
   |
20 |   Var(Variable),
   |   --- ^^^^^^^^
   |   |
   |   field in this variant
   |
help: consider changing the field to be of unit type to suppress this warning while preserving the field numbering, or remove the field
   |
20 |   Var(()),
   |       ~~

error: missing documentation for a type alias
  --> src/r1cs/mod.rs:81:1
   |
81 | pub type CommitmentKeyHint<E> = dyn Fn(&R1CSShape<E>) -> usize;
   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |
note: the lint level is defined here
  --> src/lib.rs:8:3
   |
8  |   missing_docs
   |   ^^^^^^^^^^^^

error: missing documentation for a method
   --> src/r1cs/mod.rs:168:3
    |
168 | /   pub fn multiply_vec(
169 | |     &self,
170 | |     z: &[E::Scalar],
171 | |   ) -> Result<(Vec<E::Scalar>, Vec<E::Scalar>, Vec<E::Scalar>), NovaError> {
    | |__________________________________________________________________________^

error: could not compile `nova-snark` (lib) due to 4 previous errors

@RyanKung
Copy link
Contributor Author

RyanKung commented Feb 7, 2024

My clippy version is:

➜  Nova git:(main) cargo clippy --version
clippy 0.1.77 (75c68cfd 2024-01-07)

@RyanKung
Copy link
Contributor Author

RyanKung commented Feb 7, 2024

@srinathsetty And there is also a doc fmt issue, maybe a doc style checker should be added to CI.

➜  Nova git:(main) cargo doc --open
 Documenting nova-snark v0.34.0 (/Users/ryan/Dev/bns/Nova)
error: missing documentation for a type alias
  --> src/r1cs/mod.rs:81:1
   |
81 | pub type CommitmentKeyHint<E> = dyn Fn(&R1CSShape<E>) -> usize;
   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |
note: the lint level is defined here
  --> src/lib.rs:8:3
   |
8  |   missing_docs
   |   ^^^^^^^^^^^^

error: missing documentation for a method
   --> src/r1cs/mod.rs:168:3
    |
168 | /   pub fn multiply_vec(
169 | |     &self,
170 | |     z: &[E::Scalar],
171 | |   ) -> Result<(Vec<E::Scalar>, Vec<E::Scalar>, Vec<E::Scalar>), NovaError> {
    | |__________________________________________________________________________^

error: this URL is not a hyperlink
 --> src/provider/hyperkzg.rs:2:122
  |
2 | //! HyperKZG is based on the transformation from univariate PCS to multilinear PCS in the Gemini paper (section 2.4.2 in https://eprint.iacr.org/2022/420.pdf).
  |                                                                                                                          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use an automatic link instead: `<https://eprint.iacr.org/2022/420.pdf>`
  |
  = note: bare URLs are not automatically turned into clickable links
note: the lint level is defined here
 --> src/lib.rs:3:3
  |
3 |   warnings,
  |   ^^^^^^^^
  = note: `#[deny(rustdoc::bare_urls)]` implied by `#[deny(warnings)]`

error: could not document `nova-snark`

@srinathsetty
Copy link
Collaborator

@srinathsetty And there is also a doc fmt issue, maybe a doc style checker should be added to CI.

I think this doc error might be an issue with the r1cs module public. There's a check for missing docs in src/lib.rs.

@@ -14,6 +14,7 @@ use ff::{Field, PrimeField};
#[derive(Clone, Copy)]
struct OrderedVariable(Variable);

#[allow(dead_code)]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the clippy error for this might be because of the latest Rust toolchain. So, we should fix.

However, I'm not sure allowing dead code is the right way to fix this problem (allowing dead_code is just ignoring the issue rather than fixing it!)

@RyanKung
Copy link
Contributor Author

RyanKung commented Feb 8, 2024

@srinathsetty And there is also a doc fmt issue, maybe a doc style checker should be added to CI.

I think this doc error might be an issue with the r1cs module public. There's a check for missing docs in src/lib.rs.

Yes, I forgot to reset to the origin commit, sorry. Without public r1cs module, cargo doc works fine.

@RyanKung
Copy link
Contributor Author

RyanKung commented Feb 8, 2024

I see the issue is fixed by #[allow(deadcode], So I close this PR. Thx.

@RyanKung RyanKung closed this Feb 8, 2024
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 this pull request may close these issues.

2 participants