Skip to content

[question_mark]: also trigger on return statements #11994

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.

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

Merged
merged 2 commits into from
Dec 23, 2023

Conversation

y21
Copy link
Member

@y21 y21 commented Dec 21, 2023

This fixes the false negative mentioned in #11993: the lint only used to check for return expressions, and not a statement containing a return expression (doesn't close the issue tho since there's still a useful suggestion that we could make, which is to suggest .ok_or()?/.ok_or_else()? for else { return Err(..) })

changelog: [question_mark]: also trigger on return statements

@rustbot
Copy link
Collaborator

rustbot commented Dec 21, 2023

r? @xFrednet

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Dec 21, 2023
@samueltardieu
Copy link
Contributor

I like how it immediately flagged 3 instances in Clippy sources. I wonder how many will trigger in popular crates.

@samueltardieu
Copy link
Contributor

From the top 500 crates, only 4 crates triggered this lint, totaling a number of 72 instances (most of them in the plotters-backend crate).

@y21
Copy link
Member Author

y21 commented Dec 22, 2023

Nice. This comment made me think that this would cause us to lint in a lot more cases due to rustfmt adding the semicolon and suppressing the lint that way for correctly-formatted code, but double-checking now, it doesn't seem to be case that it does that.

Hopefully those 4 are legit warnings and not FPs? Also thanks for running lintcheck, I meant to do that myself but forgot 😅

@samueltardieu
Copy link
Contributor

Hopefully those 4 are legit warnings and not FPs?

Looks like it. I'm currently submitting a PR for the libc crate, and may submit others as well.

Copy link
Member

@xFrednet xFrednet left a comment

Choose a reason for hiding this comment

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

LGTM, small NIT and then we can r+ this :D

expr: Some(els),
..
} = els
&& let Some(els) = find_let_else_ret_expression(els)
Copy link
Member

Choose a reason for hiding this comment

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

This change looks good to me. Could you also add a check for comments, with span_contains_comment and a test?

    let Some(x) = y else {
        // Roses are red,
        // violets are blue,
        // please keep this comment,
        // it's art, you know?
        return None;
    };

Copy link
Member Author

@y21 y21 Dec 23, 2023

Choose a reason for hiding this comment

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

Good idea! Fixed. (also love that quote)

@xFrednet
Copy link
Member

Perfection! And what a masterpiece.

@bors r+

@bors
Copy link
Contributor

bors commented Dec 23, 2023

📌 Commit e44caea has been approved by xFrednet

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Dec 23, 2023

⌛ Testing commit e44caea with merge 830f1c5...

@bors
Copy link
Contributor

bors commented Dec 23, 2023

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: xFrednet
Pushing 830f1c5 to master...

@bors bors merged commit 830f1c5 into rust-lang:master Dec 23, 2023
bors added a commit to rust-lang/libc that referenced this pull request Jan 7, 2024
Simplify build.rs by using the question mark operator

The `question_mark` clippy lint currently overlooks some patterns that could be replaced by the question mark operator. This will be fixed shortly by rust-lang/rust-clippy#11994.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants