Skip to content

Lint suggestion: Misleading usage of ? with let Some(...) #16140

@Wilfred

Description

@Wilfred

What it does

I've seen Rust newcomers bitten by ? semantics with Option when they're explicitly using let Some(...). This is a footgun that's discussed at length on this Rust internals thread. As languages like TypeScript grow in popularity, I think it will be easier to make this mistake.

fn display_name(nicknames: Option<Vec<String>>, use_placeholder: bool) -> Option<String> {
    if use_placeholder {
        // This looks totally innocent to a Rust newbie. If they're expecting
        // TypeScript semantics then `?.` does not look like an early return.
        if let Some(first_nick) = nicknames?.first() {
            return Some(first_nick.to_owned());
        }

        return Some("Anonymous".to_owned());
    }

    nicknames.unwrap_or_default().first().cloned()
}

fn main() {
    let use_placeholder = true;

    // The programmer expected Some("Anonymous"), but got None.
    println!("Display name: {:?}", display_name(None, use_placeholder));
}

I would love to have a lint that warns on let Some(...) = foo?, if let Some(...) = foo? or if let Some(...) = foo().bar? when ? is used on an Option. Mixing let Some(...) and ? can be misleading.

Advantage

Mixing let Some(...) and ? inside a function returning Option can make the early return less obvious. Avoiding ? in this case would make the code more explicit.

Drawbacks

This potentially annoys experienced users who are accustomed to ? semantics, but I think this combination is rare in practice. Grepping some open source code for let Some\(.*=.*\? looks like the majority are for ? with Result.

Another alternative would be banning ? entirely for Option in codebases with newbie contributors. That feels more extreme and I don't see any clippy lint that offers it today.

Example

fn display_name(nicknames: Option<Vec<String>>, use_placeholder: bool) -> Option<String> {
    if use_placeholder {
        if let Some(first_nick) = nicknames?.first() {
            return Some(first_nick.to_owned());
        }

        return Some("Anonymous".to_owned());
    }

    nicknames.unwrap_or_default().first().cloned()
}

Could be written as:

fn display_name(nicknames: Option<Vec<String>>, use_placeholder: bool) -> Option<String> {
    if use_placeholder {
        // Alternatively, use .and_then() on the option.
        if let Some(nicknames) = nicknames {
            if let Some(first_nick) = nicknames.first() {
                return Some(first_nick.to_owned());
            }
        }

        return Some("Anonymous".to_owned());
    }

    nicknames.unwrap_or_default().first().cloned()
}

Comparison with existing lints

There's no pre-existing lint for this case as far as I can see.

Additional Context

No response

Metadata

Metadata

Assignees

No one assigned

    Labels

    A-lintArea: New lints

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions