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

unnecessary_wraps is too strict #6427

Open
jan-auer opened this issue Dec 7, 2020 · 7 comments
Open

unnecessary_wraps is too strict #6427

jan-auer opened this issue Dec 7, 2020 · 7 comments
Labels
C-enhancement Category: Enhancement of lints, like adding more cases or adding help messages

Comments

@jan-auer
Copy link

jan-auer commented Dec 7, 2020

The recently introduced unnecessary_wraps lint is quite strict, and sometimes the affected function cannot be refactored. An example is when the function is used for #[serde(default)]:

fn field_default() -> Option<i32> {
    Some(42)
}

#[derive(serde::Deserialize)]
struct S {
    #[serde(default = "field_default")]
    field: Option<i32>,
}

It is a bit unfortunate having to allow the clippy lint explicitly. I was wondering if you would consider to add a function naming pattern that ignores the lint, such as an _opt suffix or a try_ prefix.

@Swatinem
Copy link
Contributor

Swatinem commented Dec 7, 2020

Or instead of heuristics based on name, when the function is passed as a callback that expects an Option or Result signature, this should be allowed.

For example:

fn my_callback_opt() -> Option<usize> { Some(0) }
fn my_callback() -> usize { 0 }
fn takes_callback<F>(_: F) where F: Fn() -> Option<usize> {}
fn main() {
    takes_callback(my_callback_opt);
    // rather than:
    takes_callback(|| { Some(my_callback()) });
}

Sometimes the functions taking the callback are from foreign crates, or really do need an Option inside, etc, so you can’t change those easily.

@giraffate
Copy link
Contributor

@rustbot modify labels: +L-enhancement

@rustbot rustbot added the C-enhancement Category: Enhancement of lints, like adding more cases or adding help messages label Dec 7, 2020
@cecton
Copy link

cecton commented Dec 29, 2020

I bumped on this issue today when using nightly.

The doc says:

Since this lint changes function type signature, you may need to adjust some code at callee side.

But that's not okay because the callee is an external crate to the project. The function signature might have to be this way.

Related to IMI-eRnD-Be/wasm-run#27

@cecton
Copy link

cecton commented Dec 29, 2020

It's also not recognized in the clippy.toml:

error: error reading Clippy's configuration file /home/cecile/repos/medigest/.clippy.toml: unknown field unnecessary-wraps, expected one of msrv, blacklisted-names, cognitive-complexity-threshold, cyclomatic-complexity-threshold, doc-valid-idents, too-many-arguments-threshold, type-complexity-threshold, single-char-binding-names-threshold, too-large-for-stack, enum-variant-name-threshold, enum-variant-size-threshold, verbose-bit-mask-threshold, literal-representation-threshold, trivial-copy-size-limit, pass-by-value-size-limit, too-many-lines-threshold, array-size-threshold, vec-box-size-threshold, max-trait-bounds, max-struct-bools, max-fn-params-bools, warn-on-all-wildcard-imports, disallowed-methods, unreadable-literal-lint-fractions, third-party at line 1 column 1

@jan-auer
Copy link
Author

This now hit stable.

@cecton
Copy link

cecton commented Feb 20, 2021

I didn't see any improvement in my case.

[0] [15:10:24] ~/r/t/t/test-crate main > c build
   Compiling memchr v2.3.4
   Compiling lazy_static v1.4.0
   Compiling unicode-segmentation v1.6.0
   Compiling regex-syntax v0.6.21
   Compiling thread_local v1.0.1
   Compiling heck v0.3.1
   Compiling aho-corasick v0.7.15
   Compiling regex v1.4.2
   Compiling twine v0.3.1 (/home/cecile/repos/twine)
   Compiling test-crate v0.1.0 (/home/cecile/repos/twine/tests/test-crate)
error: macro-expanded `macro_export` macros from the current crate cannot be referred to by absolute paths
  --> src/my_module.rs:1:5
   |
1  | use crate::t;
   |     ^^^^^^^^
   |
   = note: `#[deny(macro_expanded_macro_exports_accessed_by_absolute_paths)]` on by default
   = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!
   = note: for more information, see issue #52234 <https://github.com/rust-lang/rust/issues/52234>
note: the macro is defined here
  --> /home/cecile/repos/twine/tests/test-crate/target/debug/build/test-crate-09bdfd596ad8e1c3/out/i18n.rs:2:1
   |
2  | / macro_rules! t {
3  | | (fallback_to_default_lang $(, $fmt_args:expr)* => $lang:expr) => {
4  | | match $lang {
5  | | $crate::Lang::En(_) => format!("Hello" $(, $fmt_args)*),
...  |
60 | | }};
61 | | }
   | |_^

error: aborting due to previous error

error: could not compile `test-crate`

To learn more, run the command again with --verbose.
[101] [15:10:50] ~/r/t/t/test-crate main > rustc --version
rustc 1.50.0 (cb75ad5db 2021-02-10)

Is it supposed to be in stable 1.50? @jan-auer

bors added a commit that referenced this issue Feb 21, 2021
Change unnecessary_wraps to pedantic

changelog: Change unnecessary_wraps to pedantic

There seems to be enough evidence that this lint is not wanted as warn-by-default. Attempted before at #6380. False positives at #6721 and #6427. Actually requested to change the category at #6726.

Closes #6726
@cecton
Copy link

cecton commented Feb 24, 2021

What am I testing... 🤔 Ok nevermind my comment. I mixed up 2 different issues.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-enhancement Category: Enhancement of lints, like adding more cases or adding help messages
Projects
None yet
Development

No branches or pull requests

5 participants