new lint: [or_else_then_unwrap]

[illicitonion]

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]

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

[ada4a]

Looking pretty good:) Left some minor comments

View changes since this review

[ada4a]

is_type_diagnostic_item contains a call to cx.tcx.is_diagnostic_item, which is a bit expensive. A more efficient way to do this would be to first get the DefId of ty, and then call cx.tcx.get_diagnostic_name on that.

Also, by pulling the get_content_if_ctor_matches_in_closure call into the match arm pattern, you can avoid the need to late-initalize title and or_else_arg_content

Suggested change

	let title;
	let or_else_arg_content: Span;
	if is_type_diagnostic_item(cx, ty, sym::Option) {
	title = "found `.or_else(|| Some(…)).unwrap()`";
	if let Some(content) = get_content_if_ctor_matches_in_closure(cx, or_else_arg, LangItem::OptionSome) {
	or_else_arg_content = content;
	} else {
	return;
	}
	} else if is_type_diagnostic_item(cx, ty, sym::Result) {
	title = "found `.or_else(|| Ok(…)).unwrap()`";
	if let Some(content) = get_content_if_ctor_matches_in_closure(cx, or_else_arg, LangItem::ResultOk) {
	or_else_arg_content = content;
	} else {
	return;
	}
	} else {
	// Someone has implemented a struct with .or(...).unwrap() chaining,
	// but it's not an Option or a Result, so bail
	return;
	}
	let (title, or_else_arg_content) = match ty
	.ty_adt_def()
	.map(AdtDef::did)
	.and_then(|did| cx.tcx.get_diagnostic_name(did))
	{
	Some(sym::Option)
	if let Some(content) = get_content_if_ctor_matches_in_closure(cx, or_else_arg, LangItem::OptionSome) =>
	{
	("found `.or_else(|| Some(…)).unwrap()`", content)
	},
	Some(sym::Result)
	if let Some(content) = get_content_if_ctor_matches_in_closure(cx, or_else_arg, LangItem::ResultOk) =>
	{
	("found `.or_else(|| Ok(…)).unwrap()`", content)
	},
	// Someone has implemented a struct with .or(...).unwrap() chaining,
	// but it's not an Option or a Result, so bail
	_ => return,
	};

To avoid the rather verbose scrutinee, you could pull it into a let-chain, something like:

if let Some(did) = ty.ty_adt_def().map(AdtDef::did)
    && let Some(name) = cx.tcx.get_diagnostic_name(did)
    && let (title, or_else_arg_content) = match name {
        sym::Option
            if let Some(content) =
                get_content_if_ctor_matches_in_closure(cx, or_else_arg, LangItem::OptionSome) =>
        {
            ("found `.or_else(|| Some(…)).unwrap()`", content)
        },
        sym::Result
            if let Some(content) = get_content_if_ctor_matches_in_closure(cx, or_else_arg, LangItem::ResultOk) =>
        {
            ("found `.or_else(|| Ok(…)).unwrap()`", content)
        },
        // Someone has implemented a struct with .or(...).unwrap() chaining,
        // but it's not an Option or a Result, so bail
        _ => return,
    }
{
    // lint
}

[ada4a]

having both of these comments is a bit redundant imo.. If you want to have a comment by the side, you could do something like:

Suggested change

	let _ = option.or_else(|| Some(Wrapper::new("fallback"))).unwrap(); // should trigger lint
	//
	//~^^ or_else_then_unwrap
	let _ = option.or_else(|| Some(Wrapper::new("fallback"))).unwrap(); //~ or_else_then_unwrap

Which is not very common across the test suite, but pretty clean imo

[ada4a]

This diagnostic/suggestion looks a bit crowded -- could you try replacing this:

span_lint_and_sugg(
    cx,
    OR_ELSE_THEN_UNWRAP,
    unwrap_expr.span.with_lo(or_span.lo()),
    title,
    "try",
    suggestion,
    applicability,
);

with span_lint_and_then containing a call to Diag::span_suggestion_verbose?

[ada4a]

It took me a bit to parse this 😅 Could you please add backticks, and specify the whole name while we're at it?

Suggested change

	// or takes no argument
	// `or_else` takes no argument

[ada4a]

Suggested change

	&& let ExprKind::Call(some_expr, [arg]) = body.kind
	&& is_res_lang_ctor(cx, path_res(cx, some_expr), item)
	&& let ExprKind::Call(some_or_ok, [arg]) = body.kind
	&& is_res_lang_ctor(cx, path_res(cx, some_or_ok), item)

[ada4a]

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