Needless return by jrqc · Pull Request #5903 · rust-lang/rust-clippy

jrqc · 2020-08-13T13:05:36Z

Fixes #5858
changelog: fix false positive [needless_return]

rust-highfive · 2020-08-13T13:05:39Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @matthiaskrgr (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.

matthiaskrgr · 2020-08-13T13:50:49Z

@bors delegate=ebroto

bors · 2020-08-13T13:50:50Z

✌️ @ebroto can now approve this pull request

ebroto

This looks good on a general note, thanks!

I left some comments. Also, instead of adding a new module, can we reuse the let_and_return one?. This way we don't have to repeat the code to check if the last statement borrows. I would suggest to change the name of the current returns.rs to unused_unit.rs because it's the last lint remaining there, and rename let_and_return.rs to returns.rs.

ebroto · 2020-08-13T14:53:53Z

+    }
+}
+
+fn check_final_expr(cx: &LateContext<'_>, expr: &Expr<'_>, span: Option<Span>, replacement: RetReplacement) {


Suggested change

fn check_final_expr(cx: &LateContext<'_>, expr: &Expr<'_>, span: Option<Span>, replacement: RetReplacement) {

fn check_final_expr<'tcx>(

cx: &LateContext<'tcx>,

expr: &'tcx Expr<'tcx>,

span: Option<Span>,

replacement: RetReplacement,

) {

if last_statement_borrows(cx, expr) {

return;

}

This way we could get rid of the rest of calls to last_statement_borrows.

Oh sorry, I just realized the placement of the call to last_statement_borrows is wrong. This would lead to false negatives in cases like the following

fn read_line2(value: bool) -> String { if value { use std::io::BufRead; let stdin = ::std::io::stdin(); let _a = stdin.lock().lines().next().unwrap().unwrap(); return String::from("test"); } else { return String::new(); } }

because the visitor would walk recursively and see the borrow in the if block. The check should be done in the ExprKind::Ret(..) part, I'm adding a further comment.

We could add this snippet as an extra test too.

ebroto · 2020-08-13T14:54:53Z

+    attr.meta_item_list().is_some() && attr.has_name(sym!(cfg))
+}
+
+fn check_block_return(cx: &LateContext<'_>, block: &Block<'_>) {


Suggested change

fn check_block_return(cx: &LateContext<'_>, block: &Block<'_>) {

fn check_block_return<'tcx>(cx: &LateContext<'tcx>, block: &Block<'tcx>) {

We need explicit lifetimes here to apply my previous suggestion.

ebroto · 2020-08-13T14:57:10Z

+            FnKind::Closure(_) => {
+                if !last_statement_borrows(cx, &body.value) {
+                    check_final_expr(cx, &body.value, Some(body.value.span), RetReplacement::Empty)
+                }
+            },


Suggested change

FnKind::Closure(_) => {

if !last_statement_borrows(cx, &body.value) {

check_final_expr(cx, &body.value, Some(body.value.span), RetReplacement::Empty)

}

},

FnKind::Closure(_) => check_final_expr(cx, &body.value, Some(body.value.span), RetReplacement::Empty),

We can get rid of the if here after moving the call to last_statement_borrows

ebroto · 2020-08-13T14:58:29Z

+                    if let Some(expr) = block.expr {
+                        if !last_statement_borrows(cx, expr) {
+                            check_final_expr(cx, expr, Some(expr.span), RetReplacement::Empty);
+                        }
+                    } else if let Some(stmt) = block.stmts.iter().last() {
+                        match stmt.kind {
+                            StmtKind::Expr(ref expr) | StmtKind::Semi(ref expr) => {
+                                if !last_statement_borrows(cx, expr) {
+                                    check_final_expr(cx, expr, Some(stmt.span), RetReplacement::Empty);
+                                }
+                            },
+                            _ => (),
+                        }
+                    }


Suggested change

if let Some(expr) = block.expr {

if !last_statement_borrows(cx, expr) {

check_final_expr(cx, expr, Some(expr.span), RetReplacement::Empty);

}

} else if let Some(stmt) = block.stmts.iter().last() {

match stmt.kind {

StmtKind::Expr(ref expr) | StmtKind::Semi(ref expr) => {

if !last_statement_borrows(cx, expr) {

check_final_expr(cx, expr, Some(stmt.span), RetReplacement::Empty);

}

},

_ => (),

}

}

check_block_return(cx, block);

We can simplify here now too

ebroto · 2020-08-13T15:48:54Z

+        ExprKind::Ret(ref inner) => {
+            // allow `#[cfg(a)] return a; #[cfg(b)] return b;`
+            if !expr.attrs.iter().any(attr_is_cfg) {
+                emit_return_lint(


Before emitting, we could add something like:

let borrows = inner.map_or(false, |inner| last_statement_borrows(cx, inner)); if !borrows {

jrqc · 2020-08-13T16:24:01Z

Thanks!

I will publish the changes according to the latest review

ebroto

Almost there, just some small changes!

Also, could you:

Change the "changelog:" line to show the lint between backticks like this: [needless_return]? This way the link in the CHANGELOG.md will work.
Please rebase to get rid of the merge commits.

ebroto · 2020-08-13T21:47:32Z

+    if last_statement_borrows(cx, expr) {
+        return;
+    }


Suggested change

if last_statement_borrows(cx, expr) {

return;

}

Sorry if I was not clear, this early return should be removed

My bad, I misunderstood the change

ebroto · 2020-08-13T21:50:09Z

+            return stdin.lock().lines().next().unwrap().unwrap();
+        }
+
+        fn read_line2(value: bool) -> String {


We should check that this is linted because the borrow does not happen in the last statement of the block. Removing the early return I mentioned in my previous comment will fix this.

We could maybe change the name to make the intent more clear, what about borrows_but_not_last?

ebroto · 2020-08-13T21:50:48Z

+mod no_lint_if_stmt_borrows {
+    mod issue_5858 {
+        fn read_line() -> String {
+            use std::io::BufRead;


Let's factor out this import at the module level, removing this one and the one in the other function.

ebroto · 2020-08-13T21:53:46Z

+    /// **Known problems:** The lint currently misses unit return types in types,
+    /// e.g., the `F` in `fn generic_unit<F: Fn() -> ()>(f: F) { .. }`.


Suggested change

/// **Known problems:** The lint currently misses unit return types in types,

/// e.g., the `F` in `fn generic_unit<F: Fn() -> ()>(f: F) { .. }`.

/// **Known problems:** None.

Now that we're at it, could you make this change, please? I should have done it in a previous PR of mine 😅

jrqc · 2020-08-14T06:39:21Z

Thanks for the review again!
I've just completed the necessary changes

ebroto

Thanks! There seems to be a problem with the rebase though, the merge commits are still there and there's now additional commits from other PRs.

The process for doing the rebase should be something like the following. There are other ways, this is how I would do it (well, how my editor does it 😄):

# If you already have the remote added as "upstream", skip this first step
git remote add -f upstream https\://github.com/rust-lang/rust-clippy.git

# This will update our local master
git fetch --all
git checkout master
git reset --hard upstream/master

# This will be the actual rebase
git checkout needless_return
git rebase --interactive master

# When the editor window appears, keep just your commits. Since we have master pointing 
# to the latests upstream/master, maybe git will see that and only show your commits,  
# in that case you're already good to go

git push --force

After that only your commits should be on your branch.

If you have any problem with the process, we can make it work for you, but we will have to ping an actual collaborator since I don't have permissions to write to your fork (don't worry, I have to ping them anyway for a last review 😄).

jrqc · 2020-08-15T08:41:01Z

hi @ebroto
Your suggestion worked pretty well thanks!

ebroto · 2020-08-15T20:39:20Z

LGTM, thanks!

The CI errors are unrelated to your PR and have been fixed by this other PR, another rebase should do the job. FYI this happens once in a while, when rustc breaks clippy the fixes need to be synced.

cc @flip1995 this should be ready to merge when CI passes.

flip1995 · 2020-08-16T16:03:15Z

Thanks!

@bors r=ebroto,flip1995

Thanks for the review @ebroto

bors · 2020-08-16T16:03:16Z

📌 Commit baa4cb1 has been approved by ebroto,flip1995

bors · 2020-08-16T17:08:50Z

⌛ Testing commit baa4cb1 with merge dff7e74...

bors · 2020-08-16T17:30:09Z

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

rust-highfive assigned matthiaskrgr Aug 13, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Aug 13, 2020

ebroto suggested changes Aug 13, 2020

View reviewed changes

ebroto reviewed Aug 13, 2020

View reviewed changes

ebroto suggested changes Aug 13, 2020

View reviewed changes

ebroto suggested changes Aug 14, 2020

View reviewed changes

jrqc force-pushed the needless_return branch from 7bdc4f2 to 771c92c Compare August 15, 2020 08:32

jrqc added 7 commits August 16, 2020 00:24

Make needless_return a late lint pass

6d18fe7

Visitor added

85f4ef0

Borrow checker added

65d10c7

Known problems changed

cd6ca72

Modifications according to the code review

a7d5c2f

cargo dev update_lints

96efaee

early return removed

baa4cb1

jrqc force-pushed the needless_return branch from 771c92c to baa4cb1 Compare August 15, 2020 21:29

bors merged commit dff7e74 into rust-lang:master Aug 16, 2020

bors mentioned this pull request Aug 16, 2020

Warn about explicit self-assignment #5894

Merged

jrqc deleted the needless_return branch August 17, 2020 08:43

ebroto mentioned this pull request Sep 9, 2020

Downgrade interior_mutable_const lints to warn by default #6012

Closed

dswij mentioned this pull request Jun 16, 2022

emit needless_return on return stmts using borrow #9008

Closed

-fn check_final_expr(cx: &LateContext<'_>, expr: &Expr<'_>, span: Option<Span>, replacement: RetReplacement) {
+fn check_final_expr<'tcx>(
+    cx: &LateContext<'tcx>,
+    expr: &'tcx Expr<'tcx>,
+    span: Option<Span>,
+    replacement: RetReplacement,
+) {
+    if last_statement_borrows(cx, expr) {
+        return;
+    }

	fn check_block_return(cx: &LateContext<'_>, block: &Block<'_>) {
	fn check_block_return<'tcx>(cx: &LateContext<'tcx>, block: &Block<'tcx>) {

		/// Known problems: The lint currently misses unit return types in types,
		/// e.g., the `F` in `fn generic_unit<F: Fn() -> ()>(f: F) { .. }`.

	/// Known problems: The lint currently misses unit return types in types,
	/// e.g., the `F` in `fn generic_unit<F: Fn() -> ()>(f: F) { .. }`.
	/// Known problems: None.

Conversation

jrqc commented Aug 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rust-highfive commented Aug 13, 2020

Uh oh!

matthiaskrgr commented Aug 13, 2020

Uh oh!

bors commented Aug 13, 2020

Uh oh!

ebroto left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ebroto Aug 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jrqc commented Aug 13, 2020

Uh oh!

ebroto left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jrqc commented Aug 14, 2020

Uh oh!

ebroto left a comment

Choose a reason for hiding this comment

Uh oh!

jrqc commented Aug 15, 2020

Uh oh!

ebroto commented Aug 15, 2020

Uh oh!

flip1995 commented Aug 16, 2020

Uh oh!

bors commented Aug 16, 2020

Uh oh!

bors commented Aug 16, 2020

Uh oh!

bors commented Aug 16, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

jrqc commented Aug 13, 2020 •

edited

Loading

ebroto Aug 13, 2020 •

edited

Loading