Option<Option<_>>

[llogiq]

I don't know if it's used anywhere, but it does seem like misuse.

[Manishearth]

Suggesting an enum instead?

I dunno, the few times I've seen it used are when you have a few option-using APIs being stacked up.

[llogiq]

Nested options are a unary counting system. But what does the presence of the absence of a value mean? (apart from: Your program should be refactored)

[Manishearth]

Sure.

But there are plenty of std APIs that deal with options, and there could be legitimate use cases for using nested options in case one is composing such APIs.

Though I'm okay with linting on AST Option<Option<T>>s. My concern is mostly with expr_ty nested options.

[durka]

It happens (with the comment in libsyntax providing one rationale):

$ ag -Q 'Some(None)'
src/libcore/option.rs
933:                    Some(None) => {

src/librustdoc/markdown.rs
63:        markdown::PLAYGROUND_KRATE.with(|s| { *s.borrow_mut() = Some(None); });

src/libsyntax/ext/format.rs
264:    /// yet, `Some(None)` means that we've seen the argument, but no format was
269:    /// that: `Some(None) == Some(Some(x))`

src/libsyntax/parse/parser.rs
4122:            Some(None) => true,

In my code I have an enum for "sensor currently in use" and None is one of the options, so I have a Some(None) which means "I succeeded in checking the sensors, and none of them are in use". Should I be refactoring this?

[llogiq]

We should probably make this more clear: This lint should not catch Some(None). It should catch Option<Option<..>> types in function signatures and perhaps typedefs. To correctly match Option::Some(Option::None) would require expr_ty, as Manish noted above.

[Manishearth]

Though I'm okay with linting on AST Option<Option>s. My concern is mostly with expr_ty nested options.

So basically if there's a literal Option<Option<_>> ast::Ty node (not some expression of the type Option<Option<_>>, we lint). We allow it in typedefs.

This can perhaps be done cleanly in check_ty, using ast_maps parenting to check if we're in a typedef.

I'd suggest we allow nested options in typedefs actually, and disallow elsewhere.

[killercup]

Oh! I think we have a valid use-case for this in Diesel: Updating or skipping nullable fields (these in-progress docs for AsChangeset show how it's used). None doesn't update the field, Some(None) sets it to NULL in the database, and Some(Some(x)) sets it to x.

I know this is an edge case, but just wanted to give you a heads up as it's an edge case that we can predict some diesel users will encounter :)

cc @mikeritewho wrote a PR to add this lint

[oli-obk]

@killercup wouldn't an enum be better here?

enum DbUpdate<T> {
    Keep,
    Clear,
    Update(T),
}

I mean, you just had to explain to us what each of the three cases of the nested Options does ;) So that's 3 variants... which is what an enum is perfect for, right?

[felix91gr]

This, or part of it, was implemented in #2299. Is there still work to be done? Maybe this issue can be closed :)