Skip to content
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

add a non-default lint to ban question mark operators for Option #13804

Open
sunshowers opened this issue Dec 9, 2024 · 3 comments
Open

add a non-default lint to ban question mark operators for Option #13804

sunshowers opened this issue Dec 9, 2024 · 3 comments
Labels
A-lint Area: New lints

Comments

@sunshowers
Copy link

sunshowers commented Dec 9, 2024

What it does

(This is a suggestion for a non-default lint.)

With this lint, uses of ? would be banned in any methods that returned Option<T>. Instead, code would be written using the let Some(v) = ... else { return None } pattern.

Advantage

Over time I've become quite disenchanted with the question mark operator for Option<T> -- while it has some uses, in my experience it is one of the most common sources of bugs in code I write.

A recent example of a bug is this commit, where I refactored a Result<Option<T>> into an Option<T>: nextest-rs/nextest@7f68c0e. There were some ? in this code whose meaning changed dramatically due to the refactor, and the compiler/clippy didn't point this out at all. I had to fix that later in nextest-rs/nextest@e92436f (partly by completely removing Option).

This is not the first time this has happened, and the presence of ? for Option has started making me wary of using Option in general -- often you don't just want to return None if a child function returned None, unlike errors where you do generally want to do that.

So I would love a lint which lets me ban ? in general, while possibly enabling it for particular blocks where the ? pattern is genuinely useful. The alternative recommended (let Some(v) ... else { ... }) is wordier, but also more robust to semantic changes during refactors.

Drawbacks

The alternative is more verbose.

Example

fn foo() -> Option<T> { ... }

fn bar() -> Option<T> {
    let v = foo()?;
    ...
}

Could be written as:

fn foo() -> Option<T> { ... }

fn bar() -> Option<T> {
    let Some(v) = foo() else {
        return None;
    }
    ...
}
@sunshowers sunshowers added the A-lint Area: New lints label Dec 9, 2024
@Manishearth
Copy link
Member

This makes sense; and would work as a restriction lint.

@y21
Copy link
Member

y21 commented Dec 10, 2024

We have the question_mark_used lint already for linting any use of ?, maybe we can reuse that in some way here, like adding a configuration option to only lint the use of ? on specific types (with * as the default to preserve the old behavior for existing users)?

@Manishearth
Copy link
Member

Hmm, potentially? That could work, I'm not sure if people have a use case for using both all-question-mark-banning and specific-question-mark-banning at once.

Should it be an allowlist, denylist, or both? Should we restrict to known types or something else?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-lint Area: New lints
Projects
None yet
Development

No branches or pull requests

3 participants