Needless return

[jrqc]

Fixes #5858
changelog: fix false positive [needless_return]

[rust-highfive]

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]

@bors delegate=ebroto

[bors]

✌️ @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]

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.

[ebroto]

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]

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]

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]

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]

Before emitting, we could add something like:

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

[jrqc]

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]

Suggested change

	if last_statement_borrows(cx, expr) {
	return;
	}

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

[jrqc]

My bad, I misunderstood the change

[ebroto]

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]

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

[ebroto]

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]

Sure :)

[jrqc]

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]

hi @ebroto
Your suggestion worked pretty well thanks!

[ebroto]

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]

Thanks!

@bors r=ebroto,flip1995

Thanks for the review @ebroto

[bors]

📌 Commit baa4cb1 has been approved by ebroto,flip1995

[bors]

⌛ Testing commit baa4cb1 with merge dff7e74...

[bors]

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