Skip to content

Conversation

illicitonion
Copy link
Contributor

changelog: [or_else_then_unwrap]: New lint. This is the same as or_then_unwrap but for or_else calls with a closure immediately calling Some.

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Sep 22, 2025
@rustbot
Copy link
Collaborator

rustbot commented Sep 22, 2025

r? @llogiq

rustbot has assigned @llogiq.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@illicitonion illicitonion force-pushed the or-else-then-unwrap branch 4 times, most recently from 8307906 to 8f7cade Compare September 22, 2025 20:19
Copy link
Contributor

@ada4a ada4a left a comment

Choose a reason for hiding this comment

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

Looking pretty good:) Left some minor comments

View changes since this review

}

fn get_content_if_ctor_matches_in_closure(cx: &LateContext<'_>, expr: &Expr<'_>, item: LangItem) -> Option<Span> {
if let ExprKind::Closure(closure) = expr.kind
Copy link
Contributor

Choose a reason for hiding this comment

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

No idea how likely this is, but you could add a check for the Ok/Some coming from a macro expansion -- e.g., the user can't really do anything about a case like:

fn main() {
    macro_rules! some { () => { Some(5) } };
    None.or_else(|| some!()).unwrap();

See this section for more info: https://doc.rust-lang.org/clippy/development/macro_expansions.html#spanctxt-method

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 put in a test case for this with proc_macros::external!{ and it doesn't appear to be triggering the lint without adding in_external_macro check... I'd like to understand why that is before adding the code to intentionally avoid - any idea why this would be getting ignored already? Is that test the right approach, or is there e.g. something special to know about proc_macros::external?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, right, that's because methods/mod.rs mostly filters out any method calls coming from an expansion:

fn check_methods<'tcx>(&self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) {
// Handle method calls whose receiver and arguments may not come from expansion
if let Some((name, recv, args, span, call_span)) = method_call(expr) {

/// Extracts a method call name, args, and `Span` of the method name.
/// This ensures that neither the receiver nor any of the arguments
/// come from expansion.
pub fn method_call<'tcx>(recv: &'tcx Expr<'tcx>) -> Option<(Symbol, &'tcx Expr<'tcx>, &'tcx [Expr<'tcx>], Span, Span)> {
if let ExprKind::MethodCall(path, receiver, args, call_span) = recv.kind
&& !args.iter().any(|e| e.span.from_expansion())
&& !receiver.span.from_expansion()
{
Some((path.ident.name, receiver, args, path.ident.span, call_span))
} else {
None
}
}

This might change in the future, but for now, the check I was talking about is indeed unnecessary.

@rustbot
Copy link
Collaborator

rustbot commented Oct 19, 2025

This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

@rustbot

This comment has been minimized.

@rustbot rustbot added has-merge-commits PR has merge commits, merge with caution. S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) labels Oct 19, 2025
illicitonion and others added 10 commits October 19, 2025 23:31
This is the same as or_then_unwrap but for or_else calls with a closure
immediately calling `Some`.
Co-authored-by: Ada Alakbarova <[email protected]>
Co-authored-by: Ada Alakbarova <[email protected]>
Co-authored-by: Ada Alakbarova <[email protected]>
Co-authored-by: Ada Alakbarova <[email protected]>
Co-authored-by: Ada Alakbarova <[email protected]>
@rustbot rustbot removed has-merge-commits PR has merge commits, merge with caution. S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) labels Oct 19, 2025
@illicitonion
Copy link
Contributor Author

Thanks so much for the suggestions @ada4a, I really appreciate them!

I do want to call out - I pretty much copied this directly from the or_then_unwrap lint, so all of your suggestions probably apply there too. I'm happy to put together a PR updating that when we're happy with this one too. I did originally look for ways to try to share code between the two, but it felt like that would be more complicated than it was worth, but I'm happy to take another look if you'd prefer (particularly if you have any suggestions for how to make that work well!) :)

@ada4a
Copy link
Contributor

ada4a commented Oct 20, 2025

Glad to have helped @illicitonion :)

I took a look at or_then_unwrap -- I think you're right unifying the logic wouldn't be worth it. And yes, a PR applying the suggestions to that lint as well would be very welcome:)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs-fcp 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