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 [needless_return] false negative #13214

Merged
merged 2 commits into from
Sep 7, 2024
Merged

Fix [needless_return] false negative #13214

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
af35dcd
Fix [`needless_return`] false negative when returned expression borro…
apoisternex Aug 4, 2024
66283bf
apply [`needless_return`] suggestion
apoisternex Aug 4, 2024
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
22 changes: 18 additions & 4 deletions clippy_lints/src/returns.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
use clippy_utils::diagnostics::{span_lint_and_sugg, span_lint_hir_and_then};
use clippy_utils::source::{snippet_opt, snippet_with_context};
use clippy_utils::sugg::has_enclosing_paren;
use clippy_utils::visitors::{for_each_expr, Descend};
use clippy_utils::visitors::{for_each_expr, for_each_unconsumed_temporary, Descend};
use clippy_utils::{
binary_expr_needs_parentheses, fn_def_id, is_from_proc_macro, is_inside_let_else, is_res_lang_ctor, path_res,
path_to_local_id, span_contains_cfg, span_find_starting_semi,
Expand Down Expand Up @@ -384,10 +384,24 @@ fn check_final_expr<'tcx>(
}
};

let borrows = inner.map_or(false, |inner| last_statement_borrows(cx, inner));
if borrows {
return;
if let Some(inner) = inner {
if for_each_unconsumed_temporary(cx, inner, |temporary_ty| {
if temporary_ty.has_significant_drop(cx.tcx, cx.param_env)
&& temporary_ty
.walk()
.any(|arg| matches!(arg.unpack(), GenericArgKind::Lifetime(re) if !re.is_static()))
{
ControlFlow::Break(())
} else {
ControlFlow::Continue(())
}
})
.is_break()
{
return;
}
}

if ret_span.from_expansion() {
return;
}
Expand Down
4 changes: 2 additions & 2 deletions clippy_lints/src/useless_conversion.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -217,12 +217,12 @@ impl<'tcx> LateLintPass<'tcx> for UselessConversion {
},
ExprKind::MethodCall(.., args, _) => {
cx.typeck_results().type_dependent_def_id(parent.hir_id).map(|did| {
return (
(
did,
args,
cx.typeck_results().node_args(parent.hir_id),
MethodOrFunction::Method,
);
)
})
},
_ => None,
Expand Down
4 changes: 4 additions & 0 deletions tests/ui/needless_return.fixed
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -355,4 +355,8 @@ fn conjunctive_blocks() -> String {
({ "a".to_string() } + "b" + { "c" })
}

fn issue12907() -> String {
"".split("").next().unwrap().to_string()
}

fn main() {}
4 changes: 4 additions & 0 deletions tests/ui/needless_return.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -365,4 +365,8 @@ fn conjunctive_blocks() -> String {
return { "a".to_string() } + "b" + { "c" };
}

fn issue12907() -> String {
return "".split("").next().unwrap().to_string();
}

Copy link
Member

@Centri3 Centri3 Aug 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I think this just needs a test for the mentioned RefCell or any other type that fails to compile. But I think the significant drop check is adequate

There is the rustc_insigificant_dtor attribute. (I didn't even know this existed before checking.) This is applied to certain types like Rc, HashMap, and array::IntoIter. Not sure whether this attribute changes the behavior (i.e., compiling would succeed) but it'd be a good idea to check.

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 this just needs a test for the mentioned RefCell or any other type that fails to compile. But I think the significant drop check is adequate

So should i delete the test then?

Not sure whether this attribute changes the behavior (i.e., compiling would succeed) but it'd be a good idea to check.

Yes, it compiles with the rustc_insigificant_dtor attribute and the lint is triggered.

Copy link
Member

Choose a reason for hiding this comment

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

No I meant the only change it needs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is already a very similar test to the one mentioned:

fn temporary_outlives_local() -> String {
    let x = RefCell::<String>::default();
    return x.borrow().clone();
}

Copy link
Member

Choose a reason for hiding this comment

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

Yeah that does the same thing 👍 missed that before

fn main() {}
14 changes: 13 additions & 1 deletion tests/ui/needless_return.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -665,5 +665,17 @@ LL - return { "a".to_string() } + "b" + { "c" };
LL + ({ "a".to_string() } + "b" + { "c" })
|

error: aborting due to 53 previous errors
error: unneeded `return` statement
--> tests/ui/needless_return.rs:369:5
|
LL | return "".split("").next().unwrap().to_string();
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
help: remove `return`
|
LL - return "".split("").next().unwrap().to_string();
LL + "".split("").next().unwrap().to_string()
|

error: aborting due to 54 previous errors