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

Jump to bottom

Fix for issue 6640 #6665

Merged
merged 4 commits into from
Feb 18, 2021
Merged

Fix for issue 6640 #6665

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
bfbc083
Fix for issue 6640
pag4k Feb 2, 2021
e0e51e4
Fixed test
pag4k Feb 3, 2021
6165ccc
Added detailled suggs for new case
pag4k Feb 17, 2021
a78271b
Changed fn body suggestion msg
pag4k Feb 18, 2021
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
92 changes: 59 additions & 33 deletions clippy_lints/src/unnecessary_wraps.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
use crate::utils::{
contains_return, in_macro, is_type_diagnostic_item, match_qpath, paths, return_ty, snippet, span_lint_and_then,
visitors::find_all_ret_expressions,
contains_return, in_macro, is_type_diagnostic_item, match_qpath, paths, return_ty, snippet, span_lint_and_sugg,
span_lint_and_then, visitors::find_all_ret_expressions,
};
use if_chain::if_chain;
use rustc_errors::Applicability;
Expand Down Expand Up @@ -64,6 +64,7 @@ impl<'tcx> LateLintPass<'tcx> for UnnecessaryWraps {
span: Span,
hir_id: HirId,
) {
// Abort if public function/method or closure.
match fn_kind {
FnKind::ItemFn(.., visibility, _) | FnKind::Method(.., Some(visibility), _) => {
if visibility.node.is_pub() {
Expand All @@ -74,6 +75,7 @@ impl<'tcx> LateLintPass<'tcx> for UnnecessaryWraps {
_ => (),
}

// Abort if the method is implementing a trait or of it a trait method.
if let Some(Node::Item(item)) = cx.tcx.hir().find(cx.tcx.hir().get_parent_node(hir_id)) {
if matches!(
item.kind,
Expand All @@ -83,22 +85,43 @@ impl<'tcx> LateLintPass<'tcx> for UnnecessaryWraps {
}
}

let (return_type, path) = if is_type_diagnostic_item(cx, return_ty(cx, hir_id), sym::option_type) {
// Check if return type is Option or Result. If neither, abort.
let return_ty = return_ty(cx, hir_id);
let (return_type_label, path) = if is_type_diagnostic_item(cx, return_ty, sym::option_type) {
("Option", &paths::OPTION_SOME)
} else if is_type_diagnostic_item(cx, return_ty(cx, hir_id), sym::result_type) {
} else if is_type_diagnostic_item(cx, return_ty, sym::result_type) {
("Result", &paths::RESULT_OK)
} else {
return;
};

// Take the first inner type of the Option or Result. If can't, abort.
let inner_ty = if_chain! {
pag4k marked this conversation as resolved.
Show resolved Hide resolved
// Skip Option or Result and take the first outermost inner type.
if let Some(inner_ty) = return_ty.walk().nth(1);
if let GenericArgKind::Type(inner_ty) = inner_ty.unpack();
then {
inner_ty
} else {
return;
}
};

// Check if all return expression respect the following condition and collect them.
let mut suggs = Vec::new();
let can_sugg = find_all_ret_expressions(cx, &body.value, |ret_expr| {
if_chain! {
// Abort if in macro.
Copy link
Contributor

Choose a reason for hiding this comment

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

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

if !in_macro(ret_expr.span);
// Check if a function call.
if let ExprKind::Call(ref func, ref args) = ret_expr.kind;
// Get the Path of the function call.
if let ExprKind::Path(ref qpath) = func.kind;
// Check if OPTION_SOME or RESULT_OK, depending on return type.
if match_qpath(qpath, path);
// Make sure the function call has only one argument.
if args.len() == 1;
// Make sure the function argument does not contain a return expression.
if !contains_return(&args[0]);
then {
suggs.push((ret_expr.span, snippet(cx, args[0].span.source_callsite(), "..").to_string()));
Expand All @@ -110,39 +133,42 @@ impl<'tcx> LateLintPass<'tcx> for UnnecessaryWraps {
});

if can_sugg && !suggs.is_empty() {
pag4k marked this conversation as resolved.
Show resolved Hide resolved
span_lint_and_then(
cx,
UNNECESSARY_WRAPS,
span,
format!(
"this function's return value is unnecessarily wrapped by `{}`",
return_type
)
.as_str(),
|diag| {
let inner_ty = return_ty(cx, hir_id)
.walk()
.skip(1) // skip `std::option::Option` or `std::result::Result`
.take(1) // take the first outermost inner type
.filter_map(|inner| match inner.unpack() {
GenericArgKind::Type(inner_ty) => Some(inner_ty.to_string()),
_ => None,
});
inner_ty.for_each(|inner_ty| {
// Issue 6640: If the inner type is Unit, emit lint similar to clippy::unused_unit.
if inner_ty.is_unit() {
span_lint_and_sugg(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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 🙃

Copy link
Contributor Author

@pag4k pag4k Feb 13, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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).

Copy link
Member

Choose a reason for hiding this comment

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

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? 🙃

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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
		}
	}
}
  1. 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?

Copy link
Member

Choose a reason for hiding this comment

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

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 :).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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. :-)

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

@flip1995 flip1995 Feb 17, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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

cx,
UNNECESSARY_WRAPS,
fn_decl.output.span(),
"unneeded wrapped unit return type",
format!("remove the `-> {}<()>`", return_type_label).as_str(),
String::new(),
Applicability::MaybeIncorrect,
);
} else {
span_lint_and_then(
cx,
UNNECESSARY_WRAPS,
span,
format!(
"this function's return value is unnecessarily wrapped by `{}`",
return_type_label
)
.as_str(),
|diag| {
diag.span_suggestion(
Copy link
Contributor Author

@pag4k pag4k Feb 2, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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

fn_decl.output.span(),
format!("remove `{}` from the return type...", return_type).as_str(),
inner_ty,
format!("remove `{}` from the return type...", return_type_label).as_str(),
inner_ty.to_string(),
Applicability::MaybeIncorrect,
);
});
diag.multipart_suggestion(
"...and change the returning expressions",
suggs,
Applicability::MaybeIncorrect,
);
},
);
diag.multipart_suggestion(
"...and change the returning expressions",
suggs,
Applicability::MaybeIncorrect,
);
},
);
}
}
}
}
28 changes: 28 additions & 0 deletions tests/ui/unnecessary_wraps.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,8 +116,36 @@ fn issue_6384(s: &str) -> Option<&str> {
})
}

// should be linted
fn issue_6640_1(a: bool, b: bool) -> Option<()> {
if a && b {
return Some(());
}
if a {
Some(());
Some(())
} else {
return Some(());
}
}

// should be linted
fn issue_6640_2(a: bool, b: bool) -> Result<(), i32> {
if a && b {
return Ok(());
}
if a {
Ok(());
Ok(())
} else {
return Ok(());
}
}

fn main() {
// method calls are not linted
func1(true, true);
func2(true, true);
issue_6640_1(true, true);
issue_6640_2(true, true);
}
109 changes: 6 additions & 103 deletions tests/ui/unnecessary_wraps.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,106 +1,9 @@
error: this function's return value is unnecessarily wrapped by `Option`
--> $DIR/unnecessary_wraps.rs:8:1
|
LL | / fn func1(a: bool, b: bool) -> Option<i32> {
LL | | if a && b {
LL | | return Some(42);
LL | | }
... |
LL | | }
LL | | }
| |_^
|
= note: `-D clippy::unnecessary-wraps` implied by `-D warnings`
help: remove `Option` from the return type...
|
LL | fn func1(a: bool, b: bool) -> i32 {
| ^^^
help: ...and change the returning expressions
|
LL | return 42;
LL | }
LL | if a {
LL | Some(-1);
LL | 2
LL | } else {
...

error: this function's return value is unnecessarily wrapped by `Option`
--> $DIR/unnecessary_wraps.rs:21:1
|
LL | / fn func2(a: bool, b: bool) -> Option<i32> {
LL | | if a && b {
LL | | return Some(10);
LL | | }
... |
LL | | }
LL | | }
| |_^
|
help: remove `Option` from the return type...
|
LL | fn func2(a: bool, b: bool) -> i32 {
| ^^^
help: ...and change the returning expressions
|
LL | return 10;
LL | }
LL | if a {
LL | 20
LL | } else {
LL | 30
|

error: this function's return value is unnecessarily wrapped by `Option`
--> $DIR/unnecessary_wraps.rs:51:1
|
LL | / fn func5() -> Option<i32> {
LL | | Some(1)
LL | | }
| |_^
|
help: remove `Option` from the return type...
|
LL | fn func5() -> i32 {
| ^^^
help: ...and change the returning expressions
|
LL | 1
|

error: this function's return value is unnecessarily wrapped by `Result`
--> $DIR/unnecessary_wraps.rs:61:1
|
LL | / fn func7() -> Result<i32, ()> {
LL | | Ok(1)
LL | | }
| |_^
|
help: remove `Result` from the return type...
|
LL | fn func7() -> i32 {
| ^^^
help: ...and change the returning expressions
|
LL | 1
|

error: this function's return value is unnecessarily wrapped by `Option`
--> $DIR/unnecessary_wraps.rs:93:5
|
LL | / fn func12() -> Option<i32> {
LL | | Some(1)
LL | | }
| |_____^
|
help: remove `Option` from the return type...
|
LL | fn func12() -> i32 {
| ^^^
help: ...and change the returning expressions
|
LL | 1
error[E0282]: type annotations needed
--> $DIR/unnecessary_wraps.rs:138:9
|
LL | Ok(());
| ^^ cannot infer type for type parameter `E` declared on the enum `Result`

error: aborting due to 5 previous errors
error: aborting due to previous error

For more information about this error, try `rustc --explain E0282`.