Skip to content

Commit

Permalink
chore: apply some new lints across workspace (#5736)
Browse files Browse the repository at this point in the history
# Description

## Problem\*

Resolves <!-- Link to GitHub Issue -->

## Summary\*

This PR applies a couple of lints across the workspace, most notably
amongst these being that we disallow using the qualified name for an
import when we've imported that name into the current module already.

New lints:
```
trivial_casts = "warn"
trivial_numeric_casts = "warn"
unused_import_braces = "warn"
unused_qualifications = "warn"
```

I haven't applied these lints to `noirc_frontend` as it's got the most
violations and so we can apply that in a later PR.

## Additional Context



## Documentation\*

Check one:
- [x] No documentation needed.
- [ ] Documentation included in this PR.
- [ ] **[For Experimental Features]** Documentation to be submitted in a
separate PR.

# PR Checklist\*

- [x] I have tested the changes locally.
- [ ] I have formatted the changes with [Prettier](https://prettier.io/)
and/or `cargo fmt` on default settings.
  • Loading branch information
TomAFrench authored Aug 16, 2024
1 parent e2f7e95 commit 4274efa
Show file tree
Hide file tree
Showing 48 changed files with 135 additions and 60 deletions.
6 changes: 6 additions & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,12 @@ rust-version = "1.74.1"
license = "MIT OR Apache-2.0"
repository = "https://github.com/noir-lang/noir/"

[workspace.lints.rust]
trivial_casts = "warn"
trivial_numeric_casts = "warn"
unused_import_braces = "warn"
unused_qualifications = "warn"

[workspace.dependencies]

# ACVM workspace dependencies
Expand Down
3 changes: 3 additions & 0 deletions acvm-repo/acir/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,9 @@ license.workspace = true
rust-version.workspace = true
repository.workspace = true

[lints]
workspace = true

# See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html

[dependencies]
Expand Down
8 changes: 4 additions & 4 deletions acvm-repo/acir/src/circuit/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,7 @@ impl ErrorSelector {
impl Serialize for ErrorSelector {
fn serialize<S>(&self, serializer: S) -> Result<S::Ok, S::Error>
where
S: serde::Serializer,
S: Serializer,
{
self.0.to_string().serialize(serializer)
}
Expand All @@ -112,7 +112,7 @@ impl Serialize for ErrorSelector {
impl<'de> Deserialize<'de> for ErrorSelector {
fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
where
D: serde::Deserializer<'de>,
D: Deserializer<'de>,
{
let s: String = Deserialize::deserialize(deserializer)?;
let as_u64 = s.parse().map_err(serde::de::Error::custom)?;
Expand Down Expand Up @@ -224,7 +224,7 @@ impl<F> Circuit<F> {
}

impl<F: Serialize> Program<F> {
fn write<W: std::io::Write>(&self, writer: W) -> std::io::Result<()> {
fn write<W: Write>(&self, writer: W) -> std::io::Result<()> {
let buf = bincode::serialize(self).unwrap();
let mut encoder = flate2::write::GzEncoder::new(writer, Compression::default());
encoder.write_all(&buf)?;
Expand All @@ -250,7 +250,7 @@ impl<F: Serialize> Program<F> {
}

impl<F: for<'a> Deserialize<'a>> Program<F> {
fn read<R: std::io::Read>(reader: R) -> std::io::Result<Self> {
fn read<R: Read>(reader: R) -> std::io::Result<Self> {
let mut gz_decoder = flate2::read::GzDecoder::new(reader);
let mut buf_d = Vec::new();
gz_decoder.read_to_end(&mut buf_d)?;
Expand Down
3 changes: 3 additions & 0 deletions acvm-repo/acir_field/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,9 @@ license.workspace = true
rust-version.workspace = true
repository.workspace = true

[lints]
workspace = true

# See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html

[dependencies]
Expand Down
4 changes: 2 additions & 2 deletions acvm-repo/acir_field/src/field_element.rs
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,7 @@ impl<F: PrimeField> From<i128> for FieldElement<F> {
}
}

impl<T: ark_ff::PrimeField> Serialize for FieldElement<T> {
impl<T: PrimeField> Serialize for FieldElement<T> {
fn serialize<S>(&self, serializer: S) -> Result<S::Ok, S::Error>
where
S: serde::Serializer,
Expand All @@ -124,7 +124,7 @@ impl<T: ark_ff::PrimeField> Serialize for FieldElement<T> {
}
}

impl<'de, T: ark_ff::PrimeField> Deserialize<'de> for FieldElement<T> {
impl<'de, T: PrimeField> Deserialize<'de> for FieldElement<T> {
fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
where
D: serde::Deserializer<'de>,
Expand Down
4 changes: 2 additions & 2 deletions acvm-repo/acir_field/src/generic_ark.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ use num_bigint::BigUint;

/// This trait is extremely unstable and WILL have breaking changes.
pub trait AcirField:
std::marker::Sized
Sized
+ std::fmt::Display
+ std::fmt::Debug
+ Default
Expand All @@ -24,7 +24,7 @@ pub trait AcirField:
// + From<u8>
+ From<bool>
+ std::hash::Hash
+ std::cmp::Eq
+ Eq
{
fn one() -> Self;
fn zero() -> Self;
Expand Down
3 changes: 3 additions & 0 deletions acvm-repo/acvm/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,9 @@ license.workspace = true
rust-version.workspace = true
repository.workspace = true

[lints]
workspace = true

# See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html

[dependencies]
Expand Down
3 changes: 3 additions & 0 deletions acvm-repo/acvm_js/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,9 @@ license.workspace = true
rust-version.workspace = true
repository.workspace = true

[lints]
workspace = true

# See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html

[lib]
Expand Down
8 changes: 3 additions & 5 deletions acvm-repo/acvm_js/src/js_execution_error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -54,16 +54,14 @@ impl JsExecutionError {
None => JsValue::UNDEFINED,
};
let assertion_payload = match assertion_payload {
Some(raw) => <wasm_bindgen::JsValue as JsValueSerdeExt>::from_serde(&raw)
Some(raw) => <JsValue as JsValueSerdeExt>::from_serde(&raw)
.expect("Cannot serialize assertion payload"),
None => JsValue::UNDEFINED,
};

let brillig_function_id = match brillig_function_id {
Some(function_id) => {
<wasm_bindgen::JsValue as JsValueSerdeExt>::from_serde(&function_id)
.expect("Cannot serialize Brillig function id")
}
Some(function_id) => <JsValue as JsValueSerdeExt>::from_serde(&function_id)
.expect("Cannot serialize Brillig function id"),
None => JsValue::UNDEFINED,
};

Expand Down
3 changes: 3 additions & 0 deletions acvm-repo/blackbox_solver/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,9 @@ license.workspace = true
rust-version.workspace = true
repository.workspace = true

[lints]
workspace = true

# See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html

[dependencies]
Expand Down
3 changes: 3 additions & 0 deletions acvm-repo/bn254_blackbox_solver/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,9 @@ license.workspace = true
rust-version.workspace = true
repository.workspace = true

[lints]
workspace = true

# See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html

[dependencies]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@ mod test {
fn test_derive_generators() {
let res = derive_generators("test domain".as_bytes(), 128, 0);

let is_unique = |y: Affine<grumpkin::GrumpkinParameters>, j: usize| -> bool {
let is_unique = |y: Affine<GrumpkinParameters>, j: usize| -> bool {
for (i, res) in res.iter().enumerate() {
if i != j && *res == y {
return false;
Expand Down
3 changes: 3 additions & 0 deletions acvm-repo/brillig/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,9 @@ license.workspace = true
rust-version.workspace = true
repository.workspace = true

[lints]
workspace = true

# See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html

[dependencies]
Expand Down
3 changes: 3 additions & 0 deletions acvm-repo/brillig_vm/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,9 @@ license.workspace = true
rust-version.workspace = true
repository.workspace = true

[lints]
workspace = true

# See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html

[dependencies]
Expand Down
3 changes: 3 additions & 0 deletions aztec_macros/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,9 @@ rust-version.workspace = true
license.workspace = true
repository.workspace = true

[lints]
workspace = true

# See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html

[dependencies]
Expand Down
3 changes: 3 additions & 0 deletions compiler/fm/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,9 @@ edition.workspace = true
rust-version.workspace = true
license.workspace = true

[lints]
workspace = true

# See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html

[dependencies]
Expand Down
3 changes: 3 additions & 0 deletions compiler/noirc_arena/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -5,3 +5,6 @@ authors.workspace = true
edition.workspace = true
rust-version.workspace = true
license.workspace = true

[lints]
workspace = true
3 changes: 3 additions & 0 deletions compiler/noirc_driver/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,9 @@ edition.workspace = true
rust-version.workspace = true
license.workspace = true

[lints]
workspace = true

# See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html

[build-dependencies]
Expand Down
7 changes: 2 additions & 5 deletions compiler/noirc_driver/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -274,11 +274,8 @@ pub fn check_crate(
crate_id: CrateId,
options: &CompileOptions,
) -> CompilationResult<()> {
let macros: &[&dyn MacroProcessor] = if options.disable_macros {
&[]
} else {
&[&aztec_macros::AztecMacro as &dyn MacroProcessor]
};
let macros: &[&dyn MacroProcessor] =
if options.disable_macros { &[] } else { &[&aztec_macros::AztecMacro] };

let mut errors = vec![];
let diagnostics = CrateDefMap::collect_defs(
Expand Down
3 changes: 3 additions & 0 deletions compiler/noirc_errors/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,9 @@ edition.workspace = true
rust-version.workspace = true
license.workspace = true

[lints]
workspace = true

# See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html

[dependencies]
Expand Down
2 changes: 1 addition & 1 deletion compiler/noirc_errors/src/position.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ pub struct Spanned<T> {

/// This is important for tests. Two Spanned objects are equal if their content is equal
/// They may not have the same span. Use into_span to test for Span being equal specifically
impl<T: std::cmp::PartialEq> PartialEq<Spanned<T>> for Spanned<T> {
impl<T: PartialEq> PartialEq<Spanned<T>> for Spanned<T> {
fn eq(&self, other: &Spanned<T>) -> bool {
self.contents == other.contents
}
Expand Down
3 changes: 3 additions & 0 deletions compiler/noirc_evaluator/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,9 @@ edition.workspace = true
rust-version.workspace = true
license.workspace = true

[lints]
workspace = true

# See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html

[dependencies]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2005,7 +2005,7 @@ impl<F: PartialEq> PartialEq for AcirVarData<F> {
}

// TODO: check/test this hash impl
impl<F> std::hash::Hash for AcirVarData<F> {
impl<F> Hash for AcirVarData<F> {
fn hash<H: std::hash::Hasher>(&self, state: &mut H) {
core::mem::discriminant(self).hash(state);
}
Expand Down
4 changes: 2 additions & 2 deletions compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -235,7 +235,7 @@ impl Debug for AcirDynamicArray {
#[derive(Debug, Clone)]
pub(crate) enum AcirValue {
Var(AcirVar, AcirType),
Array(im::Vector<AcirValue>),
Array(Vector<AcirValue>),
DynamicArray(AcirDynamicArray),
}

Expand Down Expand Up @@ -1650,7 +1650,7 @@ impl<'a> Context<'a> {
let read = self.acir_context.read_from_memory(source, &index_var)?;
Ok::<AcirValue, RuntimeError>(AcirValue::Var(read, AcirType::field()))
})?;
let array: im::Vector<AcirValue> = init_values.into();
let array: Vector<AcirValue> = init_values.into();
self.initialize_array(destination, array_len, Some(AcirValue::Array(array)))?;
Ok(())
}
Expand Down
9 changes: 5 additions & 4 deletions compiler/noirc_evaluator/src/ssa/ir/dfg.rs
Original file line number Diff line number Diff line change
Expand Up @@ -171,13 +171,14 @@ impl DataFlowGraph {
ctrl_typevars: Option<Vec<Type>>,
call_stack: CallStack,
) -> InsertInstructionResult {
use InsertInstructionResult::*;
match instruction.simplify(self, block, ctrl_typevars.clone(), &call_stack) {
SimplifyResult::SimplifiedTo(simplification) => SimplifiedTo(simplification),
SimplifyResult::SimplifiedTo(simplification) => {
InsertInstructionResult::SimplifiedTo(simplification)
}
SimplifyResult::SimplifiedToMultiple(simplification) => {
SimplifiedToMultiple(simplification)
InsertInstructionResult::SimplifiedToMultiple(simplification)
}
SimplifyResult::Remove => InstructionRemoved,
SimplifyResult::Remove => InsertInstructionResult::InstructionRemoved,
result @ (SimplifyResult::SimplifiedToInstruction(_)
| SimplifyResult::SimplifiedToInstructionMultiple(_)
| SimplifyResult::None) => {
Expand Down
2 changes: 1 addition & 1 deletion compiler/noirc_evaluator/src/ssa/ir/map.rs
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ impl<T> Id<T> {
// Need to manually implement most impls on Id.
// Otherwise rust assumes that Id<T>: Hash only if T: Hash,
// which isn't true since the T is not used internally.
impl<T> std::hash::Hash for Id<T> {
impl<T> Hash for Id<T> {
fn hash<H: std::hash::Hasher>(&self, state: &mut H) {
self.index.hash(state);
}
Expand Down
6 changes: 3 additions & 3 deletions compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ pub(super) struct Loop {
}

/// The queue of functions remaining to compile
type FunctionQueue = Vec<(ast::FuncId, IrFunctionId)>;
type FunctionQueue = Vec<(FuncId, IrFunctionId)>;

impl<'a> FunctionContext<'a> {
/// Create a new FunctionContext to compile the first function in the shared_context's
Expand Down Expand Up @@ -1005,14 +1005,14 @@ impl SharedContext {
}

/// Pops the next function from the shared function queue, returning None if the queue is empty.
pub(super) fn pop_next_function_in_queue(&self) -> Option<(ast::FuncId, IrFunctionId)> {
pub(super) fn pop_next_function_in_queue(&self) -> Option<(FuncId, IrFunctionId)> {
self.function_queue.lock().expect("Failed to lock function_queue").pop()
}

/// Return the matching id for the given function if known. If it is not known this
/// will add the function to the queue of functions to compile, assign it a new id,
/// and return this new id.
pub(super) fn get_or_queue_function(&self, id: ast::FuncId) -> IrFunctionId {
pub(super) fn get_or_queue_function(&self, id: FuncId) -> IrFunctionId {
// Start a new block to guarantee the destructor for the map lock is released
// before map needs to be acquired again in self.functions.write() below
{
Expand Down
12 changes: 4 additions & 8 deletions compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -396,11 +396,11 @@ impl<'a> FunctionContext<'a> {
/// return a reference to each element, for use with the store instruction.
fn codegen_array_index(
&mut self,
array: super::ir::value::ValueId,
index: super::ir::value::ValueId,
array: ValueId,
index: ValueId,
element_type: &ast::Type,
location: Location,
length: Option<super::ir::value::ValueId>,
length: Option<ValueId>,
) -> Result<Values, RuntimeError> {
// base_index = index * type_size
let index = self.make_array_index(index);
Expand Down Expand Up @@ -438,11 +438,7 @@ impl<'a> FunctionContext<'a> {
/// Prepare a slice access.
/// Check that the index being used to access a slice element
/// is less than the dynamic slice length.
fn codegen_slice_access_check(
&mut self,
index: super::ir::value::ValueId,
length: Option<super::ir::value::ValueId>,
) {
fn codegen_slice_access_check(&mut self, index: ValueId, length: Option<ValueId>) {
let index = self.make_array_index(index);
// We convert the length as an array index type for comparison
let array_len = self
Expand Down
3 changes: 3 additions & 0 deletions compiler/noirc_printable_type/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,9 @@ edition.workspace = true
rust-version.workspace = true
license.workspace = true

[lints]
workspace = true

# See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html

[dependencies]
Expand Down
Loading

0 comments on commit 4274efa

Please sign in to comment.