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

Remove nightly Rust clippy test #107

Closed
wants to merge 1 commit into from
Closed

Conversation

mgeisler
Copy link
Contributor

@mgeisler mgeisler commented Mar 8, 2024

Description of changes:

All PRs currently fail due to a Clippy lint:

warning: assigning the result of `Clone::clone()` may be inefficient
   --> mls-rs/src/tree_kem/tree_hash.rs:193:13
    |
193 |             filtered_sets[n] = filtered_sets[p as usize].clone();
    |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `clone_from()`: `filtered_sets[n].clone_from(&filtered_sets[p as usize])`
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#assigning_clones
    = note: `#[warn(clippy::assigning_clones)]` on by default

Trying to fix this as recommended results in a compilation error since we cannot call a mutating method on filtered_sets while we also read from it…

In general, Nightly Rust is a moving target (moving faster much than stable) and as such, it’s inconvenient to have Clippy fail builds due to new (experimental) lints being enabled there.

There are already two Clippy runs (using stable Rust) in the native_build.yml file, so I simply removed the unstable Rust run from no_std__build.yml.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 and MIT license.

All PRs currently fail due to a Clippy lint:

```
warning: assigning the result of `Clone::clone()` may be inefficient
   --> mls-rs/src/tree_kem/tree_hash.rs:193:13
    |
193 |             filtered_sets[n] = filtered_sets[p as usize].clone();
    |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `clone_from()`: `filtered_sets[n].clone_from(&filtered_sets[p as usize])`
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#assigning_clones
    = note: `#[warn(clippy::assigning_clones)]` on by default
```

Trying to fix this as recommended results in a compilation error since
we cannot call a mutating method on `filtered_sets` while we also read
from it…

In general, Nightly Rust is a moving target (moving faster much than
stable) and as such, it’s inconvenient to have Clippy fail builds due
to new (experimental) lints being enabled there.

There are already two Clippy runs (using stable Rust) in the
`native_build.yml` file, so I simply removed the unstable Rust run
from `no_std__build.yml`.
@mgeisler mgeisler requested a review from a team as a code owner March 8, 2024 14:06
@mulmarta
Copy link
Contributor

mulmarta commented Mar 8, 2024

I fully agree that running nightly causes problems -- but why remove clippy altogether? We can run stable clippy

@tomleavy
Copy link
Contributor

tomleavy commented Mar 8, 2024

Closing since #108 covered this

@tomleavy tomleavy closed this Mar 8, 2024
@mgeisler
Copy link
Contributor Author

mgeisler commented Mar 8, 2024

I fully agree that running nightly causes problems -- but why remove clippy altogether? We can run stable clippy

It looked like the Clippy checks here were redundant with the ones in the normal build (I might be wrong, but I don't think a no_std build will trigger other lint warnings than a regular build).

But I'm glad it's fixed now!

@mgeisler mgeisler deleted the fix-clippy branch March 11, 2024 09:36
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.

3 participants