Add new lint unnecessary_wrap

[hkmatsumoto]

Fixes #5969

changelog: New lint [unnecessary_wraps]

[rust-highfive]

r? @flip1995

(rust_highfive has picked a reviewer for you, use r? to override)

[ghost]

This lint looks really similar to the unnecessary_filter_map lint. I think you should probably reuse some code between them - particularly the visitor. I've also seen the same visitor implemented in bind_instead_of_map but it looked much more complicated. I get the feeling the one in this PR is missing some edge cases.

[flip1995]

I agree with @mikerite, that the visitor implemented here is pretty similar to bind_instead_of_map. I also think, that the bind_instead_of_map visitor is way more complicated than it has to be though.

Anyway, you can at least reuse the visitor of bind_instead_of_map and then use this code to collect all the return paths:

rust-clippy/clippy_lints/src/methods/bind_instead_of_map.rs

Lines 129 to 142 in 78fbb04

	if_chain! {
	if !in_macro(ret_expr.span);
	if let hir::ExprKind::Call(ref func_path, ref args) = ret_expr.kind;
	if let hir::ExprKind::Path(ref qpath) = func_path.kind;
	if match_qpath(qpath, Self::BAD_VARIANT_QPATH);
	if args.len() == 1;
	if !contains_return(&args[0]);
	then {
	suggs.push((ret_expr.span, args[0].span.source_callsite()));
	true
	} else {
	false
	}
	}

rustfix doesn't work with multipart suggestions, so don't worry about that. (You can use diag.multipart_suggestion() directly instead of the one from utils, since the utils one is just a useless wrapper, we should remove sometime)

[hkmatsumoto]

Sorry for the late reply, I could not use computers as I was replacing my desk.

I agree with @mikerite, that the visitor implemented here is pretty similar to bind_instead_of_map. I also think, that the bind_instead_of_map visitor is way more complicated than it has to be though.

Anyway, you can at least reuse the visitor of bind_instead_of_map and then use this code to collect all the return paths:

rust-clippy/clippy_lints/src/methods/bind_instead_of_map.rs

Lines 129 to 142 in 78fbb04

	if_chain! {
	if !in_macro(ret_expr.span);
	if let hir::ExprKind::Call(ref func_path, ref args) = ret_expr.kind;
	if let hir::ExprKind::Path(ref qpath) = func_path.kind;
	if match_qpath(qpath, Self::BAD_VARIANT_QPATH);
	if args.len() == 1;
	if !contains_return(&args[0]);
	then {
	suggs.push((ret_expr.span, args[0].span.source_callsite()));
	true
	} else {
	false
	}
	}

Got it, made the changes accordingly.
find_all_ret_expressions() is convenient and potential contributors might make use of it as well, so I think the function can go into utils crate.

[flip1995]

I would prefer if you could split the suggestion in two parts, so that it looks like the following:

error: this function returns unnecessarily wrapping data
  --> $DIR/unnecessary_wrap.rs:39:1
   |
LL | / fn func4() -> Option<i32> {
LL | |     Some(1)
LL | | }
   | |_^
   |
help: remove `Option` from the return type...
   |
LL | fn func4() -> i32 {
   |               ^^^
help: ...and change the returning expressions
   |
LL |     1
   |

[giraffate]

FYI it looks like that rust-lang/rust-clippy repo follows rustc no-merge policy. For example #5694 (comment).

[hkmatsumoto]

FYI it looks like that rust-lang/rust-clippy repo follows rustc no-merge policy. For example #5694 (comment).

Oops, thanks for pointing out.

[hkmatsumoto]

Some lines in clippy itself are linted and I noticed that this lint should not check fns named partial_cmp as they likely to return an expression like Some(self.cmp(other)).

[bors]

☔ The latest upstream changes (presumably #6267) made this pull request unmergeable. Please resolve the merge conflicts.

Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels:

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

[hkmatsumoto]

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

[flip1995]

@bors r+

Thanks!

[bors]

📌 Commit 2881bbd has been approved by flip1995

[flip1995]

@bors r+

Thanks!

[bors]

📌 Commit c7445d7 has been approved by flip1995

[bors]

⌛ Testing commit c7445d7 with merge 6c116ed...

[bors]

💔 Test failed - checks-action_test

[flip1995]

@bors retry

[bors]

⌛ Testing commit c7445d7 with merge 7fcf385...

[bors]

💔 Test failed - checks-action_test

[flip1995]

@bors retry

[bors]

⌛ Testing commit c7445d7 with merge 44d9445...

[bors]

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: flip1995
Pushing 44d9445 to master...

[couchand]

Maybe it's just me, but this lint seems awfully aggressive. There are plenty of reasons an interface might be written to return an Option or Result even if the specific implementation in question doesn't fail.

For instance, I'm very frequently factoring out the last step of initialization into a helper method; the pub constructors all return a Result because they can fail their validation, but once validated they delegate directly to the infallible internal constructor. All of these usages are now linted against here. But moving the Ok-wrapping to the pub constructors seems to be a violation of DRY.

Please move this to pedantic.

[emilk]

I'm very frequently factoring out the last step of initialization into a helper method; the pub constructors all return a Result because they can fail their validation, but once validated they delegate directly to the infallible internal constructor. All of these usages are now linted against here. But moving the Ok-wrapping to the pub constructors seems to be a violation of DRY.

I'm not sure I follow you. Wrapping an infallible call in Ok(...) seems exactly the right thing to do!

It seems to me that the situation you describe is best solved with this:

impl Foo {
    pub fn new(args: Args) -> Result<Self, Error> {
        if args.are_bad() {
           Err(...)
        } else {
            Ok(Foo::private_new(args))
        }
    }

    fn private_new(args: Args) -> Self { ... }
}

But I feel like I am probably missing something!

[couchand]

@emilk Personally, I don't think it's the role of linting to make these sorts of invasive signature requirements by default. The authors seem to agree, at least in part, given that the lint doesn't apply to functions with a visiblity of pub.

That being said, I don't think I have the bandwidth to pursue this further. Thanks for your consideration.