Add & use `is_none_or` `Option` extension in the compiler by WaffleLapkin · Pull Request #111873 · rust-lang/rust

WaffleLapkin · 2023-05-23T14:29:59Z

This adds an extension to Option which lives in the rustc_data_structures and provides a is_none_or method, similarly to the Option::is_some_and that already exists.

IMHO is_none_or makes the code a lot more readable than .map_or(true, ...) which it replaces here.

rustbot · 2023-05-23T14:30:10Z

r? @cjgillot

(rustbot has picked a reviewer for you, use r? to override)

rustbot · 2023-05-23T14:30:14Z

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

Some changes occurred to the core trait solver

cc @rust-lang/initiative-trait-system-refactor

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

lcnr · 2023-05-23T15:28:20Z

why do you add this to rustc_data_structures instead of core itself? it feels like if the method is good enough to be used in the compiler, we should properly add it as an inherent method.

WaffleLapkin · 2023-05-23T15:32:48Z

@lcnr rust-lang/libs-team#212 (comment)

rustbot · 2023-05-24T14:56:05Z

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

lcnr · 2023-05-24T15:17:15Z

I don't think this is worth it with the requirement to add an extension trait. I am open to throw this to the libs team with the added rationale of this PR, but as is this PR doesn't feel great to me.

…mpiler, r=petrochenkov Use `Option::is_some_and` and `Result::is_ok_and` in the compiler `.is_some_and(..)`/`.is_ok_and(..)` replace `.map_or(false, ..)` and `.map(..).unwrap_or(false)`, making the code more readable. This PR is a sibling of rust-lang#111873 (comment)

bors · 2023-05-26T09:55:25Z

☔ The latest upstream changes (presumably #111918) made this pull request unmergeable. Please resolve the merge conflicts.

m-ou-se · 2023-06-05T09:59:37Z

            .session
            .source_map()
            .span_to_snippet(span)
-            .map(|snippet| snippet.starts_with("#["))
-            .unwrap_or(true);
+            .map_or(true, |snippet| snippet.starts_with("#["));
        if !is_macro_callsite {


Since this condition is only used inverted, it might be easier to read to just use is_some_and with the inverted condition:

if self .session .source_map() .span_to_snippet(span) .is_some_and(|snippet| !snippet.starts_with("#[")) {

Then it clearly reads as "if the snippet is available and doesn't start with #[".

Good catch, thanks!

(as a side note span_to_snippet returns a result, so this would have to be a is_ok_and, but yeah)

m-ou-se · 2023-06-05T11:59:57Z

                    let lifetime_only_in_expanded_code =
-                        deletion_span.map(|sp| sp.in_derive_expansion()).unwrap_or(true);
+                        deletion_span.is_none_or(|sp| sp.in_derive_expansion());
                    if !lifetime_only_in_expanded_code {


Here's another case where the condition is inverted, such that is_some_and might be easier to read:

if deletion_span.is_some_and(|sp| !sp.in_derive_expansion()) {

"If the deletion span is not in a derive expansion, then .."

m-ou-se · 2023-06-05T12:08:51Z

        }
-        attr.ident().map_or(true, |ident| {
+        attr.ident().is_none_or(|ident| {
            ident.name == sym::cfg_attr || !rustc_feature::is_builtin_attr_name(ident.name)
        })


maybe_needs_tokens only ever seems to be used as !maybe_needs_tokens, so maybe this function should be inverted:

// name to be bikeshed pub fn is_complete(attrs: &[ast::Attribute]) -> bool { attrs.iter().all(|attr| { attr.is_doc_comment() || attr.ident().is_some_and(|ident| { ident.name != sym::cfg_attr && rustc_feature::is_builtin_attr_name(ident.name) }) }) }

"The attributes are complete if all tokens are either a doc comment or a builtin attribute other than cfg_attr."

m-ou-se · 2023-06-05T12:10:53Z

+            if self
+                .prev_expn_span
+                .is_none_or(|prev_expn_span| self.curr().expn_span.ctxt() != prev_expn_span.ctxt())
+            {


Suggested change

if self

.prev_expn_span

.is_none_or(|prev_expn_span| self.curr().expn_span.ctxt() != prev_expn_span.ctxt())

{

if !self

.prev_expn_span

.is_some_and(|prev_expn_span| self.curr().expn_span.ctxt() == prev_expn_span.ctxt())

{

m-ou-se · 2023-06-05T12:11:28Z

            // have an expression (to be injected into an existing `BasicBlock` represented by this
            // `BasicCoverageBlock`).
-            if !self.counter_kind.as_ref().map_or(true, |c| c.is_expression()) {
+            if !self.counter_kind.as_ref().is_none_or(|c| c.is_expression()) {


Suggested change

if !self.counter_kind.as_ref().is_none_or(|c| c.is_expression()) {

if self.counter_kind.as_ref().is_some_and(|c| !c.is_expression()) {

m-ou-se · 2023-06-05T12:19:32Z

            // allow individual lints to opt-out from being reported.
            let not_future_incompatible =
-                future_incompatible.map(|f| f.reason.edition().is_some()).unwrap_or(true);
+                future_incompatible.is_none_or(|f| f.reason.edition().is_some());


The not_ prefix of the name here also hints at an inverted condition that might be easier to read:

let incompatible = future_incompatible.is_some_and(|f| f.reason.edition().is_none());

Dylan-DPC · 2023-08-14T06:34:21Z

@WaffleLapkin any updates on this?

WaffleLapkin · 2023-08-17T17:55:52Z

@Dylan-DPC this is waiting on me figuring out if the method is actually useful. Mara showed that it can be often replaced by is_some_and making the code clearer, I just haven't had time to recheck it.

RalfJung · 2023-08-17T17:59:03Z

I recently wanted something like (x : Option<i32>).is_none_or(|x| *x == 0), though in the end it turned out I needed something else (but that's not is_none_or's fault^^).

IMO it is the obvious dual to is_some_and.

WaffleLapkin · 2023-08-17T18:07:31Z

though in the end it turned out I needed something else

This seems like a repeating pattern: is_none_or seems like a good choice for the problem, but then, when you zoom out, it turns out that there is another solution.

IMO it is the obvious dual to is_some_and.

Yes! That's why I was/am advocating for it =)

RalfJung · 2023-08-27T15:15:10Z

#115111 is an example that would benefit from is_none_or.

Grepping for map_or(false in src/bootstrap finds a whole bunch of hits, many of which would also be clearer with is_none_or I think. (I find map_or to never be a good choice, it's so much harder to mentally parse than an explicit match.)

CryZe · 2023-10-20T08:56:52Z

I have some code that vaguely looks like this:

iter.filter(|element| !element.date.is_some_and(|date| date != today))

where I want to iterate over all the elements that are from today or don't have a date. Having to negate a couple of times to get the De Morgan equivalent doesn't read well at all.

iter.filter(|element| element.date.is_none_or(|date| date == today))

reads so much better.

It's a pretty well known rule to avoid double negatives in writing because it makes it harder to understand. So I'm honestly baffled by the push back here.

WaffleLapkin · 2023-10-25T20:56:09Z

I do want to return to this and thoughtfully re-check if is_none_or is actually useful. But ATM I don't have capacity and this has too many merge conflicts, so, closing.

RalfJung · 2023-11-12T15:14:51Z

I just had another usecase, where I ended up writing

                    // If the newly promised alignment is bigger than the native alignment of this
                    // allocation, and bigger than the previously promised alignment, then set it.
                    if align > alloc_align
                        && !alloc_extra
                            .symbolic_alignment
                            .get()
                            .is_some_and(|(_, old_align)| align <= old_align)

but having to use de Morgan and !is_some_and here is awkward; this would be much more clear:

                    // If the newly promised alignment is bigger than the native alignment of this
                    // allocation, and bigger than the previously promised alignment, then set it.
                    if align > alloc_align
                        && alloc_extra
                            .symbolic_alignment
                            .get()
                            .is_none_or(|(_, old_align)| align > old_align)

WaffleLapkin · 2024-03-02T16:14:23Z

Another data point: rust analyzer has a surprising amount of .map_or(true, ...) (which is equivalent to is_none_or):

: ~/rust-analyzer (master); rg -i "map_or\(true"
crates/ide/src/signature_help.rs
614:        rest_pat.map_or(true, |it| token.text_range().start() < it.syntax().text_range().end());

crates/ide/src/inlay_hints/discriminant.rs
33:    if data_carrying && def.repr(sema.db).map_or(true, |r| r.int.is_none()) {

crates/ide/src/rename.rs
238:                        //     .map_or(true, |it| !ast::ExternCrate::can_cast(it.kind()))

crates/hir-def/src/dyn_map/keys.rs
71:        map.map.get::<FxHashMap<AstPtr<AST>, ID>>().map_or(true, |it| it.is_empty())

crates/hir-def/src/dyn_map.rs
72:        map.map.get::<FxHashMap<K, V>>().map_or(true, |it| it.is_empty())

crates/hir-def/src/nameres/collector.rs
392:            .map_or(true, |cfg| self.cfg_options.check(&cfg) != Some(false));

crates/rust-analyzer/src/cli/rustc_tests.rs
289:            if p.extension().map_or(true, |x| x != "rs") {

crates/syntax/src/ast/make.rs
779:        let needs_comma = arm.expr().map_or(true, |it| !it.is_block_like());

crates/syntax/src/ast/expr_ext.rs
380:                .map_or(true, |it| it == *self.syntax()),

crates/syntax/src/tests/sourcegen_ast.rs
713:            if trailing_sep.map_or(true, |it| comma == &**it) && n == node

crates/ide-db/src/imports/import_assets.rs
713:                if qualifier.first_qualifier().map_or(true, |it| sema.resolve_path(&it).is_none()) {

crates/ide-db/src/imports/insert_use.rs
490:        .filter(|child| child.as_token().map_or(true, |t| t.kind() != SyntaxKind::WHITESPACE))

crates/ide-completion/src/context/analysis.rs
993:                .map_or(true, |arrow| it.text_range().start() < arrow.text_range().start()),

crates/ide-assists/src/handlers/extract_function.rs
1070:                    .map_or(true, |it| it.text_range().contains_range(src.syntax().text_range()));

crates/ide-assists/src/handlers/add_missing_match_arms.rs
240:                let is_empty_expr = arm.expr().map_or(true, |e| match e {

crates/hir-ty/src/diagnostics/decl_check/case_conv.rs
52:    name.chars().next().map_or(true, |c| !c.is_lowercase())

crates/hir-ty/src/method_resolution.rs
1416:            check_that!(name.map_or(true, |n| db.const_data(c).name.as_ref() == Some(n)));
1454:            check_that!(name.map_or(true, |n| n == &data.name));
1483:            check_that!(name.map_or(true, |n| db.const_data(c).name.as_ref() == Some(n)));
1504:    check_that!(name.map_or(true, |n| n == &data.name));

crates/hir-ty/src/lower.rs
2108:            if bound.index_if_innermost().map_or(true, is_allowed) {
2115:            if bound.index_if_innermost().map_or(true, is_allowed) {

crates/hir-ty/src/mir/eval.rs
1897:        if size.checked_add(self.heap.len()).map_or(true, |x| x > self.memory_limit) {
: ~/rust-analyzer (master); git log -1 --oneline
4ef6a49b44 (HEAD -> master, upstream/master, upstream/auto) Auto merge of #16702 - Veykril:intra-doc-links-generic, r=Veykril

And to me it looks like in a lot of them .is_none_or would be much better (than both the current version and !.is_some_and).

…=petrochenkov Use `Option::is_some_and` and `Result::is_ok_and` in the compiler `.is_some_and(..)`/`.is_ok_and(..)` replace `.map_or(false, ..)` and `.map(..).unwrap_or(false)`, making the code more readable. This PR is a sibling of rust-lang/rust#111873 (comment)

rustbot assigned cjgillot May 23, 2023

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. WG-trait-system-refactor The Rustc Trait System Refactor Initiative (-Znext-solver) labels May 23, 2023

WaffleLapkin removed the WG-trait-system-refactor The Rustc Trait System Refactor Initiative (-Znext-solver) label May 23, 2023

WaffleLapkin added 3 commits May 23, 2023 15:33

Reorder stuff in rustc_data_structures/lib.rs

8eae198

Simplify useless map_or

7592c13

Add OptionExt with is_none_or to rustc_data_structures

ddc72fe

rust-cloud-vms Bot force-pushed the is_none_or_in_the_compiler branch from d7468b2 to 1c8ca38 Compare May 23, 2023 15:38

rustbot added the WG-trait-system-refactor The Rustc Trait System Refactor Initiative (-Znext-solver) label May 23, 2023

WaffleLapkin removed the WG-trait-system-refactor The Rustc Trait System Refactor Initiative (-Znext-solver) label May 23, 2023

This comment has been minimized.

Sign in to view

Use OptionExt::is_none_or in the compiler

3bd865c

rust-cloud-vms Bot force-pushed the is_none_or_in_the_compiler branch from 1c8ca38 to 3bd865c Compare May 23, 2023 15:51

rustbot added the WG-trait-system-refactor The Rustc Trait System Refactor Initiative (-Znext-solver) label May 23, 2023

WaffleLapkin removed the WG-trait-system-refactor The Rustc Trait System Refactor Initiative (-Znext-solver) label May 23, 2023

Use is_none_or in less obvious spots

2fb4550

rustbot added the WG-trait-system-refactor The Rustc Trait System Refactor Initiative (-Znext-solver) label May 24, 2023

WaffleLapkin mentioned this pull request May 24, 2023

Use Option::is_some_and and Result::is_ok_and in the compiler #111912

Merged

m-ou-se mentioned this pull request Jun 5, 2023

Option::is_none_or rust-lang/libs-team#212

Closed

m-ou-se reviewed Jun 5, 2023

View reviewed changes

WaffleLapkin added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 8, 2023

WaffleLapkin closed this Oct 25, 2023

WaffleLapkin deleted the is_none_or_in_the_compiler branch February 6, 2025 22:12

	if !self.counter_kind.as_ref().is_none_or(\|c\| c.is_expression()) {
	if self.counter_kind.as_ref().is_some_and(\|c\| !c.is_expression()) {

Uh oh!

Conversation

WaffleLapkin commented May 23, 2023

Uh oh!

rustbot commented May 23, 2023

Uh oh!

rustbot commented May 23, 2023

Uh oh!

lcnr commented May 23, 2023

Uh oh!

WaffleLapkin commented May 23, 2023

Uh oh!

This comment has been minimized.

rustbot commented May 24, 2023

Uh oh!

lcnr commented May 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bors commented May 26, 2023

Uh oh!

m-ou-se Jun 5, 2023

Choose a reason for hiding this comment

Uh oh!

WaffleLapkin Jun 5, 2023

Choose a reason for hiding this comment

Uh oh!

m-ou-se Jun 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

m-ou-se Jun 5, 2023

Choose a reason for hiding this comment

Uh oh!

m-ou-se Jun 5, 2023

Choose a reason for hiding this comment

Uh oh!

m-ou-se Jun 5, 2023

Choose a reason for hiding this comment

Uh oh!

m-ou-se Jun 5, 2023

Choose a reason for hiding this comment

Uh oh!

Dylan-DPC commented Aug 14, 2023

Uh oh!

WaffleLapkin commented Aug 17, 2023

Uh oh!

RalfJung commented Aug 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

WaffleLapkin commented Aug 17, 2023

Uh oh!

RalfJung commented Aug 27, 2023

Uh oh!

CryZe commented Oct 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

WaffleLapkin commented Oct 25, 2023

Uh oh!

RalfJung commented Nov 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

WaffleLapkin commented Mar 2, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

lcnr commented May 24, 2023 •

edited

Loading

m-ou-se Jun 5, 2023 •

edited

Loading

RalfJung commented Aug 17, 2023 •

edited

Loading

CryZe commented Oct 20, 2023 •

edited

Loading

RalfJung commented Nov 12, 2023 •

edited

Loading