Remove `Option<!>` return types. #129724

nnethercote · 2024-08-29T04:26:23Z

Several compiler functions have Option<!> for their return type. That's odd. The only valid return value is None, so why is this type used?

Because it lets you write certain patterns slightly more concisely. E.g. if you have these common patterns:

    let Some(a) = f() else { return };
    let Ok(b) = g() else { return };

you can shorten them to these:

    let a = f()?;
    let b = g().ok()?;

Huh.

An Option return type typically designates success/failure. How should I interpret the type signature of a function that always returns (i.e. doesn't panic), does useful work (modifying &mut arguments), and yet only ever fails? This idiom subverts the type system for a cute syntactic trick.

Furthermore, returning Option<!> from a function F makes things syntactically more convenient within F, but makes things worse at F's callsites. The callsites can themselves use ? with F but should not, because they will get an unconditional early return, which is almost certainly not desirable. Instead the return value should be ignored. (Note that some of callsites of process_operand, process_immedate, process_assign actually do use ?, though the early return doesn't matter in these cases because nothing of significance comes after those calls. Ugh.)

When I first saw this pattern I had no idea how to interpret it, and it took me several minutes of close reading to understand everything I've written above. I even started a Zulip thread about it to make sure I understood it properly. "Save a few characters by introducing types so weird that compiler devs have to discuss it on Zulip" feels like a bad trade-off to me. This commit replaces all the Option<!> return values and uses else/return (or something similar) to replace the relevant ? uses. The result is slightly more verbose but much easier to understand.

r? @cjgillot

rustbot · 2024-08-29T04:26:32Z

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

fee1-dead

LGTM, r=me with rollup with my nit addressed.

compiler/rustc_mir_transform/src/jump_threading.rs

Several compiler functions have `Option<!>` for their return type. That's odd. The only valid return value is `None`, so why is this type used? Because it lets you write certain patterns slightly more concisely. E.g. if you have these common patterns: ``` let Some(a) = f() else { return }; let Ok(b) = g() else { return }; ``` you can shorten them to these: ``` let a = f()?; let b = g().ok()?; ``` Huh. An `Option` return type typically designates success/failure. How should I interpret the type signature of a function that always returns (i.e. doesn't panic), does useful work (modifying `&mut` arguments), and yet only ever fails? This idiom subverts the type system for a cute syntactic trick. Furthermore, returning `Option<!>` from a function F makes things syntactically more convenient within F, but makes things worse at F's callsites. The callsites can themselves use `?` with F but should not, because they will get an unconditional early return, which is almost certainly not desirable. Instead the return value should be ignored. (Note that some of callsites of `process_operand`, `process_immedate`, `process_assign` actually do use `?`, though the early return doesn't matter in these cases because nothing of significance comes after those calls. Ugh.) When I first saw this pattern I had no idea how to interpret it, and it took me several minutes of close reading to understand everything I've written above. I even started a Zulip thread about it to make sure I understood it properly. "Save a few characters by introducing types so weird that compiler devs have to discuss it on Zulip" feels like a bad trade-off to me. This commit replaces all the `Option<!>` return values and uses `else`/`return` (or something similar) to replace the relevant `?` uses. The result is slightly more verbose but much easier to understand.

nnethercote · 2024-08-29T22:53:57Z

@bors r=fee1-dead rollup

bors · 2024-08-29T22:53:59Z

📌 Commit fa4f892 has been approved by fee1-dead

It is now in the queue for this repository.

…-dead Remove `Option<!>` return types. Several compiler functions have `Option<!>` for their return type. That's odd. The only valid return value is `None`, so why is this type used? Because it lets you write certain patterns slightly more concisely. E.g. if you have these common patterns: ``` let Some(a) = f() else { return }; let Ok(b) = g() else { return }; ``` you can shorten them to these: ``` let a = f()?; let b = g().ok()?; ``` Huh. An `Option` return type typically designates success/failure. How should I interpret the type signature of a function that always returns (i.e. doesn't panic), does useful work (modifying `&mut` arguments), and yet only ever fails? This idiom subverts the type system for a cute syntactic trick. Furthermore, returning `Option<!>` from a function F makes things syntactically more convenient within F, but makes things worse at F's callsites. The callsites can themselves use `?` with F but should not, because they will get an unconditional early return, which is almost certainly not desirable. Instead the return value should be ignored. (Note that some of callsites of `process_operand`, `process_immedate`, `process_assign` actually do use `?`, though the early return doesn't matter in these cases because nothing of significance comes after those calls. Ugh.) When I first saw this pattern I had no idea how to interpret it, and it took me several minutes of close reading to understand everything I've written above. I even started a Zulip thread about it to make sure I understood it properly. "Save a few characters by introducing types so weird that compiler devs have to discuss it on Zulip" feels like a bad trade-off to me. This commit replaces all the `Option<!>` return values and uses `else`/`return` (or something similar) to replace the relevant `?` uses. The result is slightly more verbose but much easier to understand. r? `@cjgillot`

…kingjubilee Rollup of 10 pull requests Successful merges: - rust-lang#120221 (Don't make statement nonterminals match pattern nonterminals) - rust-lang#127897 (add `aarch64_unknown_nto_qnx700` target - QNX 7.0 support for aarch64le) - rust-lang#129123 (rustdoc-json: Add test for `Self` type) - rust-lang#129642 (Bump backtrace to 0.3.74~ish) - rust-lang#129675 (allow BufReader::peek to be called on unsized types) - rust-lang#129723 (Simplify some extern providers) - rust-lang#129724 (Remove `Option<!>` return types.) - rust-lang#129725 (Stop using `ty::GenericPredicates` for non-predicates_of queries) - rust-lang#129733 (Subtree update of `rust-analyzer`) - rust-lang#129754 (wasi: Fix sleeping for `Duration::MAX`) r? `@ghost` `@rustbot` modify labels: rollup

…-dead Remove `Option<!>` return types. Several compiler functions have `Option<!>` for their return type. That's odd. The only valid return value is `None`, so why is this type used? Because it lets you write certain patterns slightly more concisely. E.g. if you have these common patterns: ``` let Some(a) = f() else { return }; let Ok(b) = g() else { return }; ``` you can shorten them to these: ``` let a = f()?; let b = g().ok()?; ``` Huh. An `Option` return type typically designates success/failure. How should I interpret the type signature of a function that always returns (i.e. doesn't panic), does useful work (modifying `&mut` arguments), and yet only ever fails? This idiom subverts the type system for a cute syntactic trick. Furthermore, returning `Option<!>` from a function F makes things syntactically more convenient within F, but makes things worse at F's callsites. The callsites can themselves use `?` with F but should not, because they will get an unconditional early return, which is almost certainly not desirable. Instead the return value should be ignored. (Note that some of callsites of `process_operand`, `process_immedate`, `process_assign` actually do use `?`, though the early return doesn't matter in these cases because nothing of significance comes after those calls. Ugh.) When I first saw this pattern I had no idea how to interpret it, and it took me several minutes of close reading to understand everything I've written above. I even started a Zulip thread about it to make sure I understood it properly. "Save a few characters by introducing types so weird that compiler devs have to discuss it on Zulip" feels like a bad trade-off to me. This commit replaces all the `Option<!>` return values and uses `else`/`return` (or something similar) to replace the relevant `?` uses. The result is slightly more verbose but much easier to understand. r? ``@cjgillot``

…kingjubilee Rollup of 9 pull requests Successful merges: - rust-lang#120221 (Don't make statement nonterminals match pattern nonterminals) - rust-lang#129123 (rustdoc-json: Add test for `Self` type) - rust-lang#129642 (Bump backtrace to 0.3.74~ish) - rust-lang#129675 (allow BufReader::peek to be called on unsized types) - rust-lang#129723 (Simplify some extern providers) - rust-lang#129724 (Remove `Option<!>` return types.) - rust-lang#129725 (Stop using `ty::GenericPredicates` for non-predicates_of queries) - rust-lang#129733 (Subtree update of `rust-analyzer`) - rust-lang#129751 (interpret/visitor: make memory order iteration slightly more efficient) r? `@ghost` `@rustbot` modify labels: rollup

…-dead Remove `Option<!>` return types. Several compiler functions have `Option<!>` for their return type. That's odd. The only valid return value is `None`, so why is this type used? Because it lets you write certain patterns slightly more concisely. E.g. if you have these common patterns: ``` let Some(a) = f() else { return }; let Ok(b) = g() else { return }; ``` you can shorten them to these: ``` let a = f()?; let b = g().ok()?; ``` Huh. An `Option` return type typically designates success/failure. How should I interpret the type signature of a function that always returns (i.e. doesn't panic), does useful work (modifying `&mut` arguments), and yet only ever fails? This idiom subverts the type system for a cute syntactic trick. Furthermore, returning `Option<!>` from a function F makes things syntactically more convenient within F, but makes things worse at F's callsites. The callsites can themselves use `?` with F but should not, because they will get an unconditional early return, which is almost certainly not desirable. Instead the return value should be ignored. (Note that some of callsites of `process_operand`, `process_immedate`, `process_assign` actually do use `?`, though the early return doesn't matter in these cases because nothing of significance comes after those calls. Ugh.) When I first saw this pattern I had no idea how to interpret it, and it took me several minutes of close reading to understand everything I've written above. I even started a Zulip thread about it to make sure I understood it properly. "Save a few characters by introducing types so weird that compiler devs have to discuss it on Zulip" feels like a bad trade-off to me. This commit replaces all the `Option<!>` return values and uses `else`/`return` (or something similar) to replace the relevant `?` uses. The result is slightly more verbose but much easier to understand. r? ```@cjgillot```

…kingjubilee Rollup of 16 pull requests Successful merges: - rust-lang#129123 (rustdoc-json: Add test for `Self` type) - rust-lang#129675 (allow BufReader::peek to be called on unsized types) - rust-lang#129678 (Deny imports of `rustc_type_ir::inherent` outside of type ir + new trait solver) - rust-lang#129723 (Simplify some extern providers) - rust-lang#129724 (Remove `Option<!>` return types.) - rust-lang#129725 (Stop using `ty::GenericPredicates` for non-predicates_of queries) - rust-lang#129730 (f32 docs: define 'arithmetic' operations) - rust-lang#129749 (llvm-wrapper: adapt for LLVM API changes) - rust-lang#129757 (Add a test for trait solver overflow in MIR inliner cycle detection) - rust-lang#129760 (Make the "detect-old-time" UI test more representative) - rust-lang#129762 (Update the `wasm-component-ld` binary dependency) - rust-lang#129767 (Remove `#[macro_use] extern crate tracing`, round 4) - rust-lang#129774 (Remove `#[macro_use] extern crate tracing` from rustdoc and rustfmt) - rust-lang#129780 (add crashtests for several old unfixed ICEs) - rust-lang#129782 (couple more crash tests) - rust-lang#129791 (mark joboet as on vacation) r? `@ghost` `@rustbot` modify labels: rollup

…-dead Remove `Option<!>` return types. Several compiler functions have `Option<!>` for their return type. That's odd. The only valid return value is `None`, so why is this type used? Because it lets you write certain patterns slightly more concisely. E.g. if you have these common patterns: ``` let Some(a) = f() else { return }; let Ok(b) = g() else { return }; ``` you can shorten them to these: ``` let a = f()?; let b = g().ok()?; ``` Huh. An `Option` return type typically designates success/failure. How should I interpret the type signature of a function that always returns (i.e. doesn't panic), does useful work (modifying `&mut` arguments), and yet only ever fails? This idiom subverts the type system for a cute syntactic trick. Furthermore, returning `Option<!>` from a function F makes things syntactically more convenient within F, but makes things worse at F's callsites. The callsites can themselves use `?` with F but should not, because they will get an unconditional early return, which is almost certainly not desirable. Instead the return value should be ignored. (Note that some of callsites of `process_operand`, `process_immedate`, `process_assign` actually do use `?`, though the early return doesn't matter in these cases because nothing of significance comes after those calls. Ugh.) When I first saw this pattern I had no idea how to interpret it, and it took me several minutes of close reading to understand everything I've written above. I even started a Zulip thread about it to make sure I understood it properly. "Save a few characters by introducing types so weird that compiler devs have to discuss it on Zulip" feels like a bad trade-off to me. This commit replaces all the `Option<!>` return values and uses `else`/`return` (or something similar) to replace the relevant `?` uses. The result is slightly more verbose but much easier to understand. r? ````@cjgillot````

…-dead Remove `Option<!>` return types. Several compiler functions have `Option<!>` for their return type. That's odd. The only valid return value is `None`, so why is this type used? Because it lets you write certain patterns slightly more concisely. E.g. if you have these common patterns: ``` let Some(a) = f() else { return }; let Ok(b) = g() else { return }; ``` you can shorten them to these: ``` let a = f()?; let b = g().ok()?; ``` Huh. An `Option` return type typically designates success/failure. How should I interpret the type signature of a function that always returns (i.e. doesn't panic), does useful work (modifying `&mut` arguments), and yet only ever fails? This idiom subverts the type system for a cute syntactic trick. Furthermore, returning `Option<!>` from a function F makes things syntactically more convenient within F, but makes things worse at F's callsites. The callsites can themselves use `?` with F but should not, because they will get an unconditional early return, which is almost certainly not desirable. Instead the return value should be ignored. (Note that some of callsites of `process_operand`, `process_immedate`, `process_assign` actually do use `?`, though the early return doesn't matter in these cases because nothing of significance comes after those calls. Ugh.) When I first saw this pattern I had no idea how to interpret it, and it took me several minutes of close reading to understand everything I've written above. I even started a Zulip thread about it to make sure I understood it properly. "Save a few characters by introducing types so weird that compiler devs have to discuss it on Zulip" feels like a bad trade-off to me. This commit replaces all the `Option<!>` return values and uses `else`/`return` (or something similar) to replace the relevant `?` uses. The result is slightly more verbose but much easier to understand. r? `````@cjgillot`````

…iaskrgr Rollup of 15 pull requests Successful merges: - rust-lang#120221 (Don't make statement nonterminals match pattern nonterminals) - rust-lang#126183 (Separate core search logic with search ui) - rust-lang#129123 (rustdoc-json: Add test for `Self` type) - rust-lang#129366 (linker: Synchronize native library search in rustc and linker) - rust-lang#129527 (Don't use `TyKind` in a lint) - rust-lang#129534 (Deny `wasm_c_abi` lint to nudge the last 25%) - rust-lang#129640 (Re-enable android tests/benches in alloc/core) - rust-lang#129642 (Bump backtrace to 0.3.74~ish) - rust-lang#129675 (allow BufReader::peek to be called on unsized types) - rust-lang#129723 (Simplify some extern providers) - rust-lang#129724 (Remove `Option<!>` return types.) - rust-lang#129725 (Stop using `ty::GenericPredicates` for non-predicates_of queries) - rust-lang#129731 (Allow running `./x.py test compiler`) - rust-lang#129751 (interpret/visitor: make memory order iteration slightly more efficient) - rust-lang#129754 (wasi: Fix sleeping for `Duration::MAX`) r? `@ghost` `@rustbot` modify labels: rollup

Rollup merge of rust-lang#129724 - nnethercote:rm-Option-bang, r=fee1-dead Remove `Option<!>` return types. Several compiler functions have `Option<!>` for their return type. That's odd. The only valid return value is `None`, so why is this type used? Because it lets you write certain patterns slightly more concisely. E.g. if you have these common patterns: ``` let Some(a) = f() else { return }; let Ok(b) = g() else { return }; ``` you can shorten them to these: ``` let a = f()?; let b = g().ok()?; ``` Huh. An `Option` return type typically designates success/failure. How should I interpret the type signature of a function that always returns (i.e. doesn't panic), does useful work (modifying `&mut` arguments), and yet only ever fails? This idiom subverts the type system for a cute syntactic trick. Furthermore, returning `Option<!>` from a function F makes things syntactically more convenient within F, but makes things worse at F's callsites. The callsites can themselves use `?` with F but should not, because they will get an unconditional early return, which is almost certainly not desirable. Instead the return value should be ignored. (Note that some of callsites of `process_operand`, `process_immedate`, `process_assign` actually do use `?`, though the early return doesn't matter in these cases because nothing of significance comes after those calls. Ugh.) When I first saw this pattern I had no idea how to interpret it, and it took me several minutes of close reading to understand everything I've written above. I even started a Zulip thread about it to make sure I understood it properly. "Save a few characters by introducing types so weird that compiler devs have to discuss it on Zulip" feels like a bad trade-off to me. This commit replaces all the `Option<!>` return values and uses `else`/`return` (or something similar) to replace the relevant `?` uses. The result is slightly more verbose but much easier to understand. r? ``````@cjgillot``````

rustbot assigned cjgillot Aug 29, 2024

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. labels Aug 29, 2024

fee1-dead requested changes Aug 29, 2024

View reviewed changes

compiler/rustc_mir_transform/src/jump_threading.rs Outdated Show resolved Hide resolved

fee1-dead assigned fee1-dead and unassigned cjgillot Aug 29, 2024

fee1-dead 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 Aug 29, 2024

nnethercote force-pushed the rm-Option-bang branch from b36666b to fa4f892 Compare August 29, 2024 22:53

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 29, 2024

workingjubilee mentioned this pull request Aug 30, 2024

Rollup of 10 pull requests #129770

Closed

workingjubilee mentioned this pull request Aug 30, 2024

Rollup of 9 pull requests #129772

Closed

workingjubilee mentioned this pull request Aug 31, 2024

Rollup of 16 pull requests #129802

Closed

workingjubilee mentioned this pull request Aug 31, 2024

Rollup of 10 pull requests #129805

Closed

matthiaskrgr mentioned this pull request Aug 31, 2024

Rollup of 17 pull requests #129807

Closed

matthiaskrgr mentioned this pull request Aug 31, 2024

Rollup of 15 pull requests #129809

Merged

bors merged commit 4418552 into rust-lang:master Aug 31, 2024
6 checks passed

rustbot added this to the 1.82.0 milestone Aug 31, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove `Option<!>` return types. #129724

Remove `Option<!>` return types. #129724

nnethercote commented Aug 29, 2024

rustbot commented Aug 29, 2024

fee1-dead left a comment

nnethercote commented Aug 29, 2024

bors commented Aug 29, 2024

Remove Option<!> return types. #129724

Remove Option<!> return types. #129724

Conversation

nnethercote commented Aug 29, 2024

rustbot commented Aug 29, 2024

fee1-dead left a comment

Choose a reason for hiding this comment

nnethercote commented Aug 29, 2024

bors commented Aug 29, 2024

Remove `Option<!>` return types. #129724

Remove `Option<!>` return types. #129724