search

rust-lang / libs-team

The home of the library team
Apache License 2.0
115 stars 18 forks source link

`Option::is_none_or` #212

Closed WaffleLapkin closed 3 months ago

WaffleLapkin commented 1 year ago

Proposal

Problem statement

It seems like there is a common use-case to check if an option is None or some condition holds for its value.

Motivation, use-cases

Similarly to how we have Option::is_some_and which is basically .map_or(false, ...), sometimes it is desirable to have the opposite "default" value, i.e. sometimes a better name for .map_or(true, ...) is wanted. See for example comments on the Option::is_some_and tracking issue:

See also 45 occurrences of .map_or(true, ...) in the rustc itself:

list

``` :~/rust-lib (rust-lib); rg ".map_or\(true" ./compiler --stats -q | rg "\d+ matches" 45 matches :~/rust-lib (rust-lib); rg ".map_or\(true" ./compiler ./compiler/rustc_middle/src/values.rs 180: let check_params = def_id.as_local().map_or(true, |def_id| { ./compiler/rustc_middle/src/ty/instance.rs 236: return ty.ty_adt_def().map_or(true, |adt_def| { ./compiler/rustc_session/src/parse.rs 58: self.spans.borrow().get(&feature).map_or(true, |spans| spans.is_empty()) ./compiler/rustc_passes/src/dead.rs 682: .map_or(true, |layout| layout.is_zst()) ./compiler/rustc_hir_analysis/src/coherence/inherent_impls_overlap.rs 152: .map_or(true, |overlap| { ./compiler/rustc_codegen_cranelift/build_system/build_sysroot.rs 72: if file.extension().map_or(true, |ext| ext.to_str().unwrap() != "o") { ./compiler/rustc_hir_analysis/src/check_unused.rs 79: tcx.extern_mod_stmt_cnum(def_id).map_or(true, |cnum| { ./compiler/rustc_attr/src/builtin.rs 1092: if sess.opts.pretty.map_or(true, |pp| pp.needs_analysis()) { ./compiler/rustc_resolve/src/macros.rs 840: if kind != NonMacroAttrKind::Tool && binding.map_or(true, |b| b.is_import()) { ./compiler/rustc_resolve/src/imports.rs 249: || max_vis.get().map_or(true, |max_vis| vis.is_at_least(max_vis, self)) ./compiler/rustc_parse/src/parser/attr.rs 428: attr.ident().map_or(true, |ident| { ./compiler/rustc_resolve/src/lib.rs 1066: if def_id.map_or(true, |def_id| def_id.is_local()) { ./compiler/rustc_resolve/src/diagnostics.rs 285: self.extern_prelude.get(&ident).map_or(true, |entry| entry.introduced_by_item); 512: if filter_fn(res) && ctxt.map_or(true, |ctxt| ctxt == key.ident.span.ctxt()) { ./compiler/rustc_trait_selection/src/traits/error_reporting/suggestions.rs 1463: .map_or(true, |def_id| self.tcx.object_safety_violations(def_id).is_empty()) 1499: && last_ty.map_or(true, |last_ty| { ./compiler/rustc_infer/src/infer/mod.rs 1474: value.as_ref().map_or(true, |value| !value.needs_infer()), ./compiler/rustc_mir_dataflow/src/framework/graphviz.rs 416: assert!(befores.as_ref().map_or(true, ExactSizeIterator::is_empty)); ./compiler/rustc_mir_dataflow/src/framework/direction.rs 290: if dead_unwinds.map_or(true, |dead| !dead.contains(bb)) { 505: if dead_unwinds.map_or(true, |dead| !dead.contains(bb)) { 537: if dead_unwinds.map_or(true, |dead| !dead.contains(bb)) { 563: if dead_unwinds.map_or(true, |dead| !dead.contains(bb)) { ./compiler/rustc_hir_typeck/src/_match.rs 155: && prior_arm.map_or(true, |(_, t, _)| self.can_coerce(t, ret_ty)) ./compiler/rustc_hir_typeck/src/lib.rs 490: trait_did.map_or(true, |trait_did| { ./compiler/rustc_hir_typeck/src/expr.rs 1458: if count.try_eval_usize(tcx, self.param_env).map_or(true, |len| len > 1) { ./compiler/rustc_hir_typeck/src/coercion.rs 947: .map_or(true, |u| u.is_empty()) => ./compiler/rustc_hir_typeck/src/fn_ctxt/_impl.rs 992: .map_or(true, |ty| expected.peel_refs() != ty.peel_refs()) ./compiler/rustc_hir_typeck/src/generator_interior/mod.rs 396: || ty.map_or(true, |ty| { ./compiler/rustc_hir_typeck/src/fn_ctxt/checks.rs 2190: .filter(|(idx, _)| expected_idx.map_or(true, |expected_idx| expected_idx == *idx)) ./compiler/rustc_hir/src/def.rs 729: self.ns().map_or(true, |actual_ns| actual_ns == ns) ./compiler/rustc_index/src/interval.rs 234: current.map_or(true, |x| x < self.domain as u32) ./compiler/rustc_expand/src/config.rs 466: parse_cfg(&meta_item, &self.sess).map_or(true, |meta_item| { 474: if !self.features.map_or(true, |features| features.stmt_expr_attributes) { ./compiler/rustc_const_eval/src/interpret/util.rs 44: let is_used = unused_params.contains(index).map_or(true, |unused| !unused); ./compiler/rustc_const_eval/src/interpret/intern.rs 117: let frozen = ty.map_or(true, |ty| ty.is_freeze(*ecx.tcx, ecx.param_env)); ./compiler/rustc_const_eval/src/interpret/memory.rs 431: if offset.checked_add(size, &self.tcx).map_or(true, |end| end > alloc_size) { ./compiler/rustc_const_eval/src/interpret/projection.rs 295: if from.checked_add(to).map_or(true, |to| to > len) { ./compiler/rustc_mir_transform/src/const_prop_lint.rs 722: .map_or(true, |layout| layout.is_zst()) ./compiler/rustc_mir_transform/src/coverage/graph.rs 387: if !self.counter_kind.as_ref().map_or(true, |c| c.is_expression()) { ./compiler/rustc_mir_transform/src/const_prop.rs 1142: .map_or(true, |layout| layout.is_zst()) ./compiler/rustc_mir_transform/src/coverage/spans.rs 484: if self.prev_expn_span.map_or(true, |prev_expn_span| { ./compiler/rustc_mir_build/src/check_unsafety.rs 172: ref kind if ExprCategory::of(kind).map_or(true, |cat| cat == ExprCategory::Place) => { ./compiler/rustc_borrowck/src/type_check/mod.rs 1892: if len.try_eval_usize(tcx, self.param_env).map_or(true, |len| len > 1) { ./compiler/rustc_lint/src/expect.rs 29: && tool_filter.map_or(true, |filter| expectation.lint_tool == Some(filter)) ./compiler/rustc_ast_passes/src/feature_gate.rs 100: if self.sess.opts.pretty.map_or(true, |ppm| ppm.needs_hir()) { ```

Solution sketches

impl<T> Option<T> {
    pub fn is_none_or(self, f: impl FnOnce(T) -> bool) -> bool {
        match self {
            None => true,
            Some(x) => f(x),
        }
    }
}

Links and related work

There is an open PR implementing this, that was untouched for 8 moths: https://github.com/rust-lang/rust/pull/100602

What happens now?

This issue is part of the libs-api team API change proposal process. Once this issue is filed the libs-api team will review open proposals in its weekly meeting. You should receive feedback within a week or two.

m-ou-se commented 1 year ago

Note that this proposed opt.is_none_or(..) can already be written as opt.is_none() || opt.is_some_and(..).

m-ou-se commented 1 year ago

This was discussed in the libs meetup today. We generally have a relatively high bar for methods on types like Option and Result, and we don't think this passes that bar considering the alternatives available.

WaffleLapkin commented 1 year ago

Note that this proposed opt.is_none_or(..) can already be written as opt.is_none() || opt.is_some_and(..).

Note that this only works if opt is a variable. For something like f().is_none_or(..) your suggestion won't work.

.map_or(true, ..) is an option, but it (same as your suggestion) is (imho) much less readable.

CryZe commented 1 year ago

With is_some_and stabilized, I started replacing all the occurrences of map_or(false, in my codebase. It turns out there's about just as many cases of map_or(true, to the point where it feels really weird that there's no is_none_or, so I would like this to be reconsidered.

m-ou-se commented 1 year ago

I had the opposite experience. Once I started using is_some_and I realized I didn't miss is_none_or at all, because all those cases were easily handled by !is_some_and.

m-ou-se commented 1 year ago

If we were to have a Option::is_none_or method, would you also expect a Result::is_err_or (that runs a closure taking the Ok value) and Result::is_ok_or (that takes a closure taking the Err value) and a Result::is_ok_or_err_and (that takes both a closure for the Ok value and one for the Err value)?

m-ou-se commented 1 year ago

I had the opposite experience. Once I started using is_some_and I realized I didn't miss is_none_or at all, because all those cases were easily handled by !is_some_and.

As an example of that:

I quickly looked at https://github.com/rust-lang/rust/pull/111873 for use cases, and the very first file in that change is:

        let is_macro_callsite = self
            .session
            .source_map()
            .span_to_snippet(span)
-           .map(|snippet| snippet.starts_with("#["))
-           .unwrap_or(true);
+           .map_or(true, |snippet| snippet.starts_with("#["));

Which seems like an argument for is_none_or, but then the next line is:

        if !is_macro_callsite {

Which makes me think that it'd actually be easier to follow to use is_some_and and remove the !, such that it basically says: "if the snippet is available and doesn't start with #[", which I'd find easier to understand.

WaffleLapkin commented 1 year ago

Result::is_err_or and Result::is_ok_or seem potentially interesting, I think I saw a few cases in the compiler where they can be used.

The naming is a problem though, is_err_or looks too much like is_error, similarly to Option::err_or which was not accepted a while back: https://github.com/rust-lang/rust/pull/73040#pullrequestreview-442096330.

For Result::is_ok_or_err_and I haven't yet seen any use-cases yet. It is pretty uncommon, although not unheard of, to have Result<T, T>, so usefulness of this method (especially with such a long name...) is questionable. Additionally it can be expressed via matches!(x, Ok(v) | Err(v) if ...) which is not too bad — it's a bit longer, but perfectly readable.

So, my opinions:

WaffleLapkin commented 1 year ago

@m-ou-se yeah, this is a bad example, I agree 😅

Ig a better example would be something like

pub fn is_ungated(&self, feature: Symbol) -> bool {
-    self.spans.borrow().get(&feature).map_or(true, |spans| spans.is_empty())
+    self.spans.borrow().get(&feature).is_none_or(|spans| spans.is_empty())
}

As using is_some_and would require double-negation:

    !self.spans.borrow().get(&feature).is_some_and(|spans| !spans.is_empty())

Similarly I think !(...).is_some_and() is not nice with long chains, where you have to remember negation from the start.

the8472 commented 1 year ago

Imo the Option/Result/Iterator/Stream/TryStream methods are a clusterfuck. In some cases it's flat_map or and_then or then. If "these are needed for consistency/to fill in quadrants in a feature matrix" is the argument then they should actually be consistent so understanding carries from one to the other.

In the current situation where each gets a bespoke name I end up not remembering the names and having to look them up quite often or relying on RA to offer the right thing. So my general stance is to not add any more unless there's a significantly stronger argument than consistency.

WaffleLapkin commented 1 year ago

@the8472 my main argument in this case is readability, all other options seem significantly less readable than is_none_or when it can be applied directly (i.e. negating its result is not readable, but otherwise it is better). I also get your annoyance with flat_map/and_then/then, it's very sad...

m-ou-se commented 1 year ago

Ig a better example would be something like

pub fn is_ungated(&self, feature: Symbol) -> bool {
-    self.spans.borrow().get(&feature).map_or(true, |spans| spans.is_empty())
+    self.spans.borrow().get(&feature).is_none_or(|spans| spans.is_empty())
}

That example also has a negation of the condition. It's hidden in the name: ungated. ;)

I think this would be easier to read:

pub fn is_gated(&self, feature: Symbol) -> bool {
    self.spans.borrow().get(&feature).is_some_and(|spans| !spans.is_empty())
}

"It's gated if there is a non-empty entry for the feature." (is_some_and(!))

versus

"It's ungated if there is no entry for the feature or if the entry is empty." (is_none_or)

or

"It's ungated if there is no non-empty entry for the feature." (!is_some_and(!))

WaffleLapkin commented 1 year ago

Rrright. I'll check/fix the PR more closely and see if there are still valid use cases.

RalfJung commented 9 months ago

What is the bar for reopening this ACP? Here are some recent examples people posted where is_none_or would have made the code more clear:

Generally this is not surprising; is_none_or is the de-Morgan dual to is_some_and after all. We have both && and ||, and we have both and_then and or_else; it is inconsistent with the rest of our API to have only one of the pair of duals for is_some_and and is_none_or.

marcelchampagne commented 6 months ago

I recognize the importance of maintaining high standards for methods on Option and Result. However, I also believe is_none_or would be a valuable addition and the most consistent with the existing APIs. Specifically, I think @RalfJung's argument above is a strong one and it seems like there are a significant number of people who also share a similar sentiment, indicated by the numerous 👍 reactions it has received over the past four months.

Would it be possible to reconsider this proposal?

RalfJung commented 5 months ago

Another usecase in the compiler:

tys.last().iter().all(|ty| is_very_trivially_sized(**ty))

This is iterating over an Option just to avoid map_or(false, ...), which IMO is quite hard to follow. With is_none_or this could nicely avoid iterators and become

tys.last().is_none_or(|ty| is_very_trivially_sized(**ty))

I can of course do

!tys.last().is_some_and(|ty| !is_very_trivially_sized(**ty))

but that is extremely hard to reason about.

Having is_some_and but not is_none_or is like having any but not all.

Would it be possible to reconsider this proposal?

I think that requires someone to file a new ACP. Do you want to do that?

Amanieu commented 3 months ago

I've also found this to be quite useful in my own code, and found that !is_some_and with a reversed condition is particularly unreadable. I will therefore re-open this ACP and have it re-considered in our next libs-api meeting.

Amanieu commented 3 months ago

We discussed this in the libs-api meeting again today and we're happy to accept it. This is primarily based on the experience reports which show that only relying on is_some_and is insufficient because inverting the conditions can make the code less readable.

With that said, we would also like people to provide feedback on https://github.com/rust-lang/rfcs/pull/3573 which is a language feature which provides similar functionality.