Fix for issue 6640

[pag4k]

Please write a short comment explaining your change (or "none" for internal only changes)
changelog: unnecessary_wraps will now suggest to remove unnecessary wrapped return unit type, like Option<()>
fixes #6640

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Manishearth (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

[pag4k]

Note that I added many comments in the lint because it helped me understand the code. I left them there. Please tell me if I should remove them or if they are wrong.

[pag4k]

This part is now much simpler now that the inner type is found earlier.

[pag4k]

This is basically a copy from unused_unit. Should I change it?

[xFrednet]

You should probably use span_lint_and_then here as well because this lint suggests changes at several places:

1. Changing the return type
2. Changing the actual return expression

span_lint_and_sugg only works for one suggestion this would leave the return statements unchanged which would lead to syntax errors until the user updates them. span_lint_and_then allows a suggesting for the return type and the for the expressions using multipart_suggestion.

---

I personally would try to reuse the span_lint_and_then and just adjust the suggestions and message string according to the case.

1. We change or remove the return type (type_snippet)
2. We have a message which is changed according to the suggestion (type_change)
3. The generated suggestions are also adapted based on the change so either
   • return Some(XYZ); -> return XYZ; (Some(XYZ) -> XYZ)
   • return Some(()); -> return; (Some(()) -> ``)

This would allow the span_lint_and_then reuse like this:

let (type_change, type_snippet) = if inner_ty.is_unit() {
    (
        format!("Remove the return declaration like this:"),
        String::new(),
    )
} else {
    (
        format!("remove `{}` from the return type like this:", return_type_label),
        return_type_label.to_string(),
    )
};

let msg = format!("this function's return value is unnecessarily wrapped by `{}`", return_type_label);

span_lint_and_then(
    cx,
    UNNECESSARY_WRAPS,
    span,
    msg.as_str(),
    |diag| {
        diag.span_suggestion(
            fn_decl.output.span(),
            type_change.as_str(),
            type_snippet,
            Applicability::MaybeIncorrect,
        );
        diag.multipart_suggestion(
            "And then change the returning expressions like this:",
            suggs,
            Applicability::MaybeIncorrect,
        );
    },
);

That's just my own style, so there might be better ways or some other way which seems cleaner to you. Just use the one you are most comfortable with 🙃

[pag4k]

Thank you for the great suggestion. However, there is one thing I am not certain with. One issue I see with using span_lint_and_then() for cases like -> Option<()> is that depending on the function, we would have two different kinds of suggestions: 1) return Some(()); -> return; and 2) Some(()) -> remove the line. I wonder if we can always propose good suggestions for 2).

[xFrednet]

We should still notify the user even if they should just remove a line. multipart_suggestion can also be used to remove code by just suggesting an empty string. You already use this in the replacement of the -> Option<()>. The only thing you would need to adapt is the suggestion generation (Here is the change in pseudo rust):

// line 110

// Check if all return expression respect the following condition and collect them.
let is_inner_union = inner_ty.is_unit();
let mut suggs = Vec::new();
let can_sugg = find_all_ret_expressions(cx, &body.value, |ret_expr| {
    if_chain! {
        [...]
        then {
            if is_inner_union {
                suggs.push((ret_expr.span, "".to_string()));
            } else {
                suggs.push((ret_expr.span, snippet(cx, args[0].span.source_callsite(), "..").to_string()));
            }
            true
        } else {
            false
        }
    }
});

The span is just set to ret_expr.span. I believe that this would only be the Some(()) part from return Some(()). The suggestion would therefore leave the simple return, and we don't need to handle any special cases. Could you maybe try this? 🙃

[pag4k]

Thanks again for the great suggestion. I had already started to try that, but I had given up for the above reason.
So it works, but I see two potential issues:

1. For the case with a return, it works, but since the white space between it and the return expression is not included in the latter span, I had to hack a function to included it. I works for my tests, but I don't know if it's robust enough:

// Given an expression, if it parents expression is a return expression, return its span with the
// whitespaces after `return` keyword. For example, if we have `return 1;`, by passing the
// expression `1`, this function would return the span of ` 1`.
pub fn get_ret_expr_span_with_prefix_space(cx: &LateContext<'_>, ret_expr: &Expr<'_>, span: Span) -> Option<Span> {
	if_chain! {
		// Try to get parent expression which would include `return`.
		if let Some(parent_expr) = get_parent_expr(cx, ret_expr);
		// Verify if that expression is ExprKind::Ret.
		if let ExprKind::Ret(_) = parent_expr.kind;
		// Get the parent expression source.
		if let Ok(parent_expr_source) = cx
			.sess()
			.source_map()
			.span_to_snippet(span.with_lo(parent_expr.span.lo()).with_hi(parent_expr.span.hi()));
		// Find "return"
		if let Some(pos) = parent_expr_source.find("return");
		then {
			#[allow(clippy::cast_possible_truncation)]
			Some(parent_expr.span.with_lo(BytePos(parent_expr.span.lo().0 + pos as u32 + 6)))
		} else {
			None
		}
	}
}

2. As you said, the empty string does remove the Some(()), but it leaves an empty line (see between Some(()); and `} else {:

LL |         return;
LL |     }
LL |     if a {
LL |         Some(());
LL |
LL |     } else {

Is there a way to remove this? Or is it fine like this?

[xFrednet]

Creating such a function is a valid option and the more I think about it probably also the cleanest option. 👍

I would suggest adding it as an util function similar to utils::position_before_rarrow. This is probably something that will be reused in the future. You can then use this by

1. Calling utils::line_span with the expression span.
2. Snipping the returned span. This should then be the entire line as a string.
3. Get the offset with the new util function.
4. Add the offset to the line span.

All of these steps can probably also be done by the util function itself.

If you want to remove the entire line like you wanted in your example output you can simply use utils::line_span. The visualization will most likely still show the empty line though. The lint has multiple spans with suggestions and is therefor not automatically applicable anyways. The suggestion is therefor only for the user to see which changes should be done :).

Another way would be to suggest a change that includes the second to last statement like this:

    if {
        println!("Hello world!");
// From here    v
        Some(());
        return Option::Some(());
    }
//  ^ To here

The suggestion would then show the suggestion like:

LL |        Some(());
LL |    }

The suggestion is a bit cleaner but the code to get this suggestion a more complicated with more edge cases. These are the options I can think of. It might be worth to ask over on our Zulip channel if you want more suggestions :).

[pag4k]

Thank you again for the long reply.
To be honest, I think I prefer leaving an empty line. I am also worried that by trying to remove line, we might hit some weird edge cases. Just removing the the span of the return expression seems much safer even if the suggestion will not be as clean. So if you approve, I'll go this way.
About the utility function, a function position_after_return() would just be a call to find(), so I'm not sure it would be worth it. I could add this whole function, but I feel it is a bit too sketchy to go in utisl. :-P But you tell me. :-)

[flip1995]

Generally using ByteOffsets in suggestions is almost always a bad idea. That is because it almost always assumes a specific formatting of the code. Also it may remove comments.

In this case I would just suggest to remove the Some(()) and just keep the empty line. If the user cares about formatting, rustfmt will deal with that anyway. If not ¯\_(ツ)_/¯. Formatting shouldn't be a concern for Clippy.

---

I also thought about adding a utils function, that produces a suggestion to remove an entire line before. But then I found this case:

if xyz { Some(()) } else { Err(()) }

which is valid formatting according to rustfmt with some configurations. Just removing the entire line or everything rfind("\n") would break the code.

[pag4k]

Thank you for the comment.
Yes, I was a bit uncomfortable with using ByteOffset. However, without it, I don't know if there is a way to avoid suggesting replacing return Some(()); by return ;. That white space it annoying. :-P
Any alternative?

[flip1995]

Nope, just ignore that whitespace. Formatting shouldn't be a concern for Clippy.

If you want to use ByteOffset, always ask yourself, if there could be a (reasonable) case where it may break code. In this case, this is one: Playground

[pag4k]

I realized after the first commit that there was an error in the test file.

[pag4k]

I'm not too sure how to write this to be consistent with both Option and Result.

[xFrednet]

You can use snipped to copy the original text. This should return you a String with -> Option<()> or -> Result<(), XYZ> and therefore cover both cases 🙃

[camsteffen]

You can just say "remove this return type". No need to repeat the code that is shown and underlined.

The current version of the lint says

...and change the returning expressions

For the new cases added here, could you just change that message to

...and remove instances of `Some(())`

I don't think it's really necessary to detect whether the line needs to be removed. Unless I'm missing something?

If you go with a more "detailed" approach, just make sure there are more test cases to go with it.

[pag4k]

Thank you for you comment! :-)
Just to be certain to understand, are you suggesting that we do not provide suggestions for the body of the function? Is that what you call the detailed approach?

[camsteffen]

I was not very clear. I meant to use diag.multipart_suggestion(message, suggs, ..) just like the existing code, but just change the message for the "unit case" that you are adding. By "detailed" approach, I am referring to the new approach for linting the body that you have been discussing above.

[xFrednet]

Hey, it's awesome to see a new contributor welcome to clippy 🙃.

I'm not an official reviewer, so there might be some things which are debatable, but I hope that this answers some of your questions 👍. Please ask if something is unclear :)

[xFrednet]

You can use snipped to copy the original text. This should return you a String with -> Option<()> or -> Result<(), XYZ> and therefore cover both cases 🙃

[Manishearth]

r? @camsteffen if you wanna review this instead, feel free to bounce it back to me!

[camsteffen]

Some minor comments from me. Looks good overall!

[camsteffen]

Some of these comments are just restating what the code itself already says.

[camsteffen]

You can just say "remove this return type". No need to repeat the code that is shown and underlined.

The current version of the lint says

...and change the returning expressions

For the new cases added here, could you just change that message to

...and remove instances of `Some(())`

I don't think it's really necessary to detect whether the line needs to be removed. Unless I'm missing something?

If you go with a more "detailed" approach, just make sure there are more test cases to go with it.

[Manishearth]

@bors delegate=camsteffen

(You can r+ yourself now)

[bors]

✌️ @camsteffen can now approve this pull request

[camsteffen]

One nit about the message.

Can you also add a negative test with something like if true { Ok(()) } else { Err(()) }?

Otherwise looks good! @Manishearth do you want to do a final review?

[camsteffen]

I think this should be more clear.

...and then remove returned values of `Ok(())`

[pag4k]

About the new message, you are right, it would be more clear.
I am, however, not sure if I should hard-code the "Some" and "Ok" strings. I could get the text from the actual return expressions in the body of the function, but we never know how the user could format those.
Is there any alternative to hard-coding and this?

[camsteffen]

I think hard-coding them would be fine since it is just Some and Ok. I'd also be fine with taking that part out of the message like "remove the returned values".

[camsteffen]

Nice first contribution! I'll go ahead and wield my delegated powers.

@bors r+

[bors]

📌 Commit a78271b has been approved by camsteffen

[bors]

⌛ Testing commit a78271b with merge 0f70e88...

[bors]

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: camsteffen
Pushing 0f70e88 to master...

[pag4k]

Thank you so much to everybody for the help and patience. With such a great welcome, I certainly want to contribute more. :-)