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.

Sign up for GitHub

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

Fixing cargo clippy complaints #3448

Merged
merged 13 commits into from
Aug 16, 2022
Merged

Fixing cargo clippy complaints #3448

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
cde4709
Fixing cargo clippy complaints
Aug 15, 2022
beb250d
Merge branch 'main' into cargo_clippy_fix
franklee26 Aug 15, 2022
84db729
Removed derive Eq and disabled clippy warnings
Aug 15, 2022
2b49aa7
Merge branch 'main' of https://github.com/aws/s2n-tls into cargo_clip…
Aug 15, 2022
b9872e7
Merge branch 'cargo_clippy_fix' of https://github.com/aws/s2n-tls int…
Aug 15, 2022
cd21b2c
Fixing cargo clippy complaints
Aug 15, 2022
af3c471
Removed derive Eq and disabled clippy warnings
Aug 15, 2022
634e7b8
Moved clippy macro to lib.rs
Aug 16, 2022
96377a8
Merge branch 'cargo_clippy_fix' of https://github.com/aws/s2n-tls int…
Aug 16, 2022
33c6ffd
Formatting
Aug 16, 2022
79871a3
Fixing rebase
Aug 16, 2022
0ce13d5
Merge branch 'cargo_clippy_fix' of https://github.com/aws/s2n-tls int…
Aug 16, 2022
861b04d
Merge branch 'main' into cargo_clippy_fix
franklee26 Aug 16, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions bindings/rust/generate/src/main.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ use libc::{iovec, FILE};
"#;

fn gen_bindings(entry: &str, s2n_dir: &Path, functions: FunctionCallbacks) -> bindgen::Builder {
let builder = bindgen::Builder::default()
bindgen::Builder::default()
.use_core()
.layout_tests(true)
.detect_include_paths(true)
Expand All @@ -99,8 +99,7 @@ fn gen_bindings(entry: &str, s2n_dir: &Path, functions: FunctionCallbacks) -> bi
.ctypes_prefix("::libc")
.parse_callbacks(Box::new(functions))
.clang_arg(format!("-I{}/api", s2n_dir.display()))
.clang_arg(format!("-I{}", s2n_dir.display()));
builder
.clang_arg(format!("-I{}", s2n_dir.display()))
}

fn gen_files(input: &Path, out: &Path) -> io::Result<()> {
Expand Down Expand Up @@ -199,7 +198,8 @@ impl FunctionCallbacks {
let mut o = io::BufWriter::new(&mut tests);

writeln!(o, "{}", COPYRIGHT)?;
for (feature, function) in functions.iter() {
let iter = functions.iter();
for (feature, function) in iter {
Comment on lines +201 to +202
Copy link
Contributor

Choose a reason for hiding this comment

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

why is this needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Error:

> cargo clippy --manifest-path bindings/rust/Cargo.toml --all-targets -- -D warnings                        

    Checking generate v0.1.0 (/Volumes/workplace/s2n-tls/bindings/rust/generate)
    Checking s2n-tls v0.0.11 (/Volumes/workplace/s2n-tls/bindings/rust/s2n-tls)
error: temporary with significant `Drop` in `for` loop condition will live until the end of the `for` expression
   --> generate/src/main.rs:201:36
    |
201 |         for (feature, function) in functions.iter() {
    |                                    ^^^^^^^^^^^^^^^^
202 |             // don't generate tests for types
203 |             if types.contains(function) {
    |                ----- another value with significant `Drop` created here
...
208 |             if feature.is_some() && functions.contains(&(None, function.to_string())) {
    |                                     --------- another value with significant `Drop` created here
...
224 |         }
    |          - temporary lives until here
    |
    = note: `-D clippy::significant-drop-in-scrutinee` implied by `-D warnings`
    = note: this might lead to deadlocks or other unexpected behavior
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#significant_drop_in_scrutinee

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm hesitant to even fix this. This lint is unhelpful and doesn't make the code any more readable. In fact, I would argue it makes it less readable since it introduces an unnecessary variable in scope. From my reading, it looks like they're going to disable it by default in the next release: rust-lang/rust-clippy#8987

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree it doesn't seem helpful, but I don't think it's bad enough to bother suppressing. One unnecessary variable seems fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think an extra let declaration is ok, especially if it keeps clippy happy. When clippy disables this lint, we can remove this change.

Copy link
Contributor

Choose a reason for hiding this comment

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

When clippy disables this lint, we can remove this change.

Not without a TODO and an issue or something we won't :) But I also don't think it's important enough to spend time reverting.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah not a blocker. Especially since this code isn't even consumed outside of publishing. I'm just kind of annoyed with clippy pushing new warnings by default that are not universally applicable.

// don't generate tests for types
if types.contains(function) {
continue;
Expand Down
1 change: 1 addition & 0 deletions bindings/rust/s2n-tls/src/config.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ use std::{
sync::atomic::{AtomicUsize, Ordering},
};

#[allow(clippy::derive_partial_eq_without_eq)]
#[derive(Debug, PartialEq)]
pub struct Config(NonNull<s2n_config>);

Expand Down
6 changes: 6 additions & 0 deletions bindings/rust/s2n-tls/src/enums.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ use crate::error::Error;
use core::convert::TryFrom;
use s2n_tls_sys::*;

#[allow(clippy::derive_partial_eq_without_eq)]
franklee26 marked this conversation as resolved.
Show resolved Hide resolved
#[derive(Debug, PartialEq, Copy, Clone)]
pub enum CallbackResult {
Success,
Expand All @@ -31,6 +32,7 @@ impl<T, E> From<Result<T, E>> for CallbackResult {
}
}

#[allow(clippy::derive_partial_eq_without_eq)]
#[derive(Debug, PartialEq, Copy, Clone)]
pub enum Mode {
Server,
Expand All @@ -46,6 +48,7 @@ impl From<Mode> for s2n_mode::Type {
}
}

#[allow(clippy::derive_partial_eq_without_eq)]
#[non_exhaustive]
#[derive(Debug, PartialEq, Copy, Clone)]
pub enum Version {
Expand Down Expand Up @@ -74,6 +77,7 @@ impl TryFrom<s2n_tls_version::Type> for Version {
}
}

#[allow(clippy::derive_partial_eq_without_eq)]
#[non_exhaustive]
#[derive(Debug, PartialEq, Copy, Clone)]
pub enum Blinding {
Expand All @@ -90,6 +94,7 @@ impl From<Blinding> for s2n_blinding::Type {
}
}

#[allow(clippy::derive_partial_eq_without_eq)]
#[non_exhaustive]
#[derive(Debug, PartialEq, Copy, Clone)]
pub enum ClientAuthType {
Expand All @@ -108,6 +113,7 @@ impl From<ClientAuthType> for s2n_cert_auth_type::Type {
}
}

#[allow(clippy::derive_partial_eq_without_eq)]
#[non_exhaustive]
#[derive(Debug, PartialEq, Copy, Clone)]
pub enum AlertBehavior {
Expand Down
4 changes: 4 additions & 0 deletions bindings/rust/s2n-tls/src/error.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ use libc::c_char;
use s2n_tls_sys::*;
use std::{convert::TryFrom, ffi::CStr};

#[allow(clippy::derive_partial_eq_without_eq)]
#[non_exhaustive]
#[derive(Debug, PartialEq)]
pub enum ErrorType {
Expand All @@ -21,6 +22,7 @@ pub enum ErrorType {
UsageError,
}

#[allow(clippy::derive_partial_eq_without_eq)]
#[non_exhaustive]
#[derive(Debug, PartialEq)]
pub enum ErrorSource {
Expand All @@ -44,12 +46,14 @@ impl From<libc::c_int> for ErrorType {
}
}

#[allow(clippy::derive_partial_eq_without_eq)]
#[derive(Clone, PartialEq)]
pub enum Context {
InvalidInput,
Code(s2n_status_code::Type, Errno),
}

#[allow(clippy::derive_partial_eq_without_eq)]
#[derive(Clone, PartialEq)]
pub struct Error(Context);

Expand Down