Skip to content

Option<Option<_>> #97

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

Closed
llogiq opened this issue Jun 13, 2015 · 18 comments
Closed

Option<Option<_>> #97

llogiq opened this issue Jun 13, 2015 · 18 comments
Labels
A-lint Area: New lints good first issue These issues are a good way to get started with Clippy T-AST Type: Requires working with the AST

Comments

@llogiq
Copy link
Contributor

llogiq commented Jun 13, 2015

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

@Manishearth
Copy link
Member

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
Copy link
Contributor Author

llogiq commented Jun 13, 2015

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
Copy link
Member

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.

@Manishearth Manishearth added good first issue These issues are a good way to get started with Clippy T-AST Type: Requires working with the AST A-lint Area: New lints labels Aug 11, 2015
@durka
Copy link
Contributor

durka commented Aug 12, 2015

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
Copy link
Contributor Author

llogiq commented Aug 12, 2015

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
Copy link
Member

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
Copy link
Member

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
Copy link
Contributor

oli-obk commented Jan 12, 2018

@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
Copy link
Contributor

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

@flip1995
Copy link
Member

flip1995 commented Mar 6, 2019

#2299 seems to implement everything discussed in this issue. Thanks for pointing this out!

@flip1995 flip1995 closed this as completed Mar 6, 2019
yaahc pushed a commit to yaahc/rust-clippy that referenced this issue Mar 14, 2019
Closes rust-lang#97
Closes rust-lang#601 (this is an equivalent solution for that problem)
yaahc pushed a commit to yaahc/rust-clippy that referenced this issue Mar 14, 2019
As pointed in rust-lang#633, it's currently not possible for a package to reexport the
feature of another package due to the limitations of how features are defined.

This commit adds support for this ability by allowing features of the form
`foo/bar` in the `features` section of the manifest. This form indicates that
the dependency `foo` should have its `bar` feature enabled. Additionally, it is
not required that `foo` is an optional dependency.

This does not allow features of the form `foo/bar` in a `[dependencies]`
features section as dependencies shouldn't be enabling features for other
dependencies.

At the same time, this passes through features to build commands to solve a few more issues.

Closes rust-lang#97
Closes rust-lang#601 (this is an equivalent solution for that problem)
Closes rust-lang#633
Closes rust-lang#674
@schungx
Copy link

schungx commented Jul 23, 2019

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

I'm wondering if there is a standard type of this kind in the standard library. Right now, I'm forced to use Option<Option<T>> to keep the three states: replace, no-change and clear.

It will be super easy to create one myself, obviously. However, is that a good practice to constantly re-invent the wheel? And to re-implement all the nice support methods defined for Option...

@schungx
Copy link

schungx commented Jul 24, 2019

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)

It means exactly as it means: There is a value present, and it is nothing.

Option<Option<T>> is crucial to formulate tri-state values, such as:

  • Yes / No / Maybe
  • Set / Unset / Unchange
  • Inserted / Deleted / Left-alone
  • Permitted / Denied / Inherited Value

Think of how many three-state concepts you find in a typical system and you'll know that it is a big error not to have a tri-state enum built into Rust.

@Manishearth
Copy link
Member

Each tristate has different meanings though. In each of the examples you gave you really need a custom enum for it to be clear

@schungx
Copy link

schungx commented Jul 26, 2019

Each tristate has different meanings though. In each of the examples you gave you really need a custom enum for it to be clear

Yes, a custom enum will work just fine. However, I'll be giving up the whole set of nice useful utility methods defined on the Option, and even on the Option<Option>> like flatten.

Or I can just copy/paste the Option source code. Neither way is very elegant.

I have thought about making a custom enum and then DeRef it to Option<Option>>... but I can't figure out how to return an &Option<Option> on the deref method form the enum.

@Manishearth
Copy link
Member

The combinator method names also don't make as much sense in tristate contexts. Its not clear at all, you really should use a custom enum with well named method. Clippy is pushing you towards the right thing here.

(Deref won't work, it has to produce references and you don't have actual Options to borrow from. Furthermore, using deref for custom coercions is a bit of an antipattern)

@schungx
Copy link

schungx commented Jul 27, 2019

you really should use a custom enum

I suppose so. But if everybody does that, won't every program be littered with a bunch of similar-usage types that do similar things?

A Tri-State enum comes up so often that there really should at least be a standard crate to call upon...

@killercup
Copy link
Member

My personal concern mentioned above was based on the fact that a lot of code generation was involved, and I've come around over the last two years.

But if everybody does that, won't every program be littered with a bunch of similar-usage types that do similar things?

Similar, but not identical. If you wanted to have shared behavior for some part of it, you might want to have a derive macro that allows you to flag one variant as "empty" or one as "full" and get some methods like unwrap for free.

But I would not shy away from writing new "simple" types like these. They tend to make the code so much more readable that the small amount of needed boilerplate is – IMHO – quite acceptable. It is kinda the same as using custom enums instead of bool all over the place, just one step more abstract :)

@Manishearth
Copy link
Member

Again, they aren't the same enum, the implications of each state are different in each case, unifying them leads to a loss of clarity. I've come across this situation a bunch and it's been exceedingly rare where some generic tristate would have led to more clarity.

Put a different way: we use C-like enums a ton, "why don't we just use integers instead?". Just because things are represented the same doesn't mean they're logically the same

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-lint Area: New lints good first issue These issues are a good way to get started with Clippy T-AST Type: Requires working with the AST
Projects
None yet
Development

No branches or pull requests

8 participants