Remove `MaybeForgetReturn` suggestion #137303

compiler-errors · 2025-02-20T03:54:56Z

#115196 implemented a suggestion to add a missing return when there is an ambiguity error, when that ambiguity error could be constrained by the return type of the function.

I initially reviewed it and thought it could be useful; however, looking back at that code now, I feel like it's a bit too much of a hack to be worth keeping around in typeck, especially given how rare it's expected to fire in practice. This is especially true because it depends on StashKey::MaybeForgetReturn, which is only stashed when we have Sized obligation ambiguity errors. Let's remove it for now.

I'd like to note that it's basically impossible to get this suggestion to apply in its current state except for what I'd consider somewhat artificial examples, involving no generic trait bounds. For example, it's not triggered for:

struct W<T>(T);

fn bar<T: Default>() -> W<T> { todo!() }

fn foo() -> W<i32> {
    if true {
        bar();
    }
    W(0)
}

Nor is it triggered for:

fn foo() -> i32 {
    if true {
        Default::default();
    }
    0
}

It's basically only triggered iff there's only one ambiguity error on the type, which is Sized.

Generally, suggesting something that affects control flow is a pretty dramatic suggestion; therefore, both the accuracy and precision of this diagnostic should be pretty high.

One other, somewhat unrelated observation is that this might be using stashed diagnostics incorrectly (or at least unnecessarily). Stashed diagnostics are used when error detection is fragmented over several major stages of the compiler, like a parse or resolver error which later can be recovered in typeck. However, this one is a bit different since it is fully handled within typeck -- perhaps that suggests that if this were to be reimplemented, it wouldn't need to be so complicated of an implementation.

rustbot · 2025-02-20T03:55:05Z

r? @cjgillot

rustbot has assigned @cjgillot.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

bors · 2025-02-22T00:02:36Z

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

cjgillot · 2025-02-25T00:20:09Z

cc @chenyukang and @estebank as you wrote and reviewed the original PR.

compiler-errors · 2025-03-04T00:05:50Z

It's been a few weeks with no input here. @cjgillot, do you plan to approve this or should I re-roll?

cjgillot · 2025-03-04T02:13:25Z

No comment from original pr author, so:
@bors r+ rollup

bors · 2025-03-04T02:13:28Z

📌 Commit 9323ba5 has been approved by cjgillot

It is now in the queue for this repository.

…gillot Remove `MaybeForgetReturn` suggestion rust-lang#115196 implemented a suggestion to add a missing `return` when there is an ambiguity error, when that ambiguity error could be constrained by the return type of the function. I initially reviewed it and thought it could be useful; however, looking back at that code now, I feel like it's a bit too much of a hack to be worth keeping around in typeck, especially given how rare it's expected to fire in practice. This is especially true because it depends on `StashKey::MaybeForgetReturn`, which is only stashed when we have *Sized* obligation ambiguity errors. Let's remove it for now. I'd like to note that it's basically impossible to get this suggestion to apply in its current state except for what I'd consider somewhat artificial examples, involving no generic trait bounds. For example, it's not triggered for: ```rust struct W<T>(T); fn bar<T: Default>() -> W<T> { todo!() } fn foo() -> W<i32> { if true { bar(); } W(0) } ``` Nor is it triggered for: ``` fn foo() -> i32 { if true { Default::default(); } 0 } ``` It's basically only triggered iff there's only one ambiguity error on the type, which is `Sized`. Generally, suggesting something that affects control flow is a pretty dramatic suggestion; therefore, both the accuracy and precision of this diagnostic should be pretty high. One other, somewhat unrelated observation is that this might be using stashed diagnostics incorrectly (or at least unnecessarily). Stashed diagnostics are used when error detection is fragmented over several major stages of the compiler, like a parse or resolver error which later can be recovered in typeck. However, this one is a bit different since it is fully handled within typeck -- perhaps that suggests that if this were to be reimplemented, it wouldn't need to be so complicated of an implementation.

…iaskrgr Rollup of 12 pull requests Successful merges: - rust-lang#136975 (Look for `python3` first on MacOS, not `py`) - rust-lang#137240 (Slightly reformat `std::fs::remove_dir_all` error docs) - rust-lang#137303 (Remove `MaybeForgetReturn` suggestion) - rust-lang#137463 ([illumos] attempt to use posix_spawn to spawn processes) - rust-lang#137722 (`librustdoc`: 2024 edition! 🎊) - rust-lang#137758 (fix usage of ty decl macro fragments in attributes) - rust-lang#137772 (Fix char count in `Display` for `ByteStr`) - rust-lang#137805 (adjust Layout debug printing to match the internal field name) - rust-lang#137808 (Do not require that unsafe fields lack drop glue) - rust-lang#137913 (Allow struct field default values to reference struct's generics) - rust-lang#137963 (Add ``dyn`` keyword to `E0373` examples) - rust-lang#137975 (Remove unused `PpMode::needs_hir`) r? `@ghost` `@rustbot` modify labels: rollup

…gillot Remove `MaybeForgetReturn` suggestion rust-lang#115196 implemented a suggestion to add a missing `return` when there is an ambiguity error, when that ambiguity error could be constrained by the return type of the function. I initially reviewed it and thought it could be useful; however, looking back at that code now, I feel like it's a bit too much of a hack to be worth keeping around in typeck, especially given how rare it's expected to fire in practice. This is especially true because it depends on `StashKey::MaybeForgetReturn`, which is only stashed when we have *Sized* obligation ambiguity errors. Let's remove it for now. I'd like to note that it's basically impossible to get this suggestion to apply in its current state except for what I'd consider somewhat artificial examples, involving no generic trait bounds. For example, it's not triggered for: ```rust struct W<T>(T); fn bar<T: Default>() -> W<T> { todo!() } fn foo() -> W<i32> { if true { bar(); } W(0) } ``` Nor is it triggered for: ``` fn foo() -> i32 { if true { Default::default(); } 0 } ``` It's basically only triggered iff there's only one ambiguity error on the type, which is `Sized`. Generally, suggesting something that affects control flow is a pretty dramatic suggestion; therefore, both the accuracy and precision of this diagnostic should be pretty high. One other, somewhat unrelated observation is that this might be using stashed diagnostics incorrectly (or at least unnecessarily). Stashed diagnostics are used when error detection is fragmented over several major stages of the compiler, like a parse or resolver error which later can be recovered in typeck. However, this one is a bit different since it is fully handled within typeck -- perhaps that suggests that if this were to be reimplemented, it wouldn't need to be so complicated of an implementation.

Rollup of 10 pull requests Successful merges: - rust-lang#134063 (dec2flt: Clean up float parsing modules) - rust-lang#136581 (Retire the legacy `Makefile`-based `run-make` test infra) - rust-lang#136975 (Look for `python3` first on MacOS, not `py`) - rust-lang#137147 (Add exclude to config.toml) - rust-lang#137240 (Slightly reformat `std::fs::remove_dir_all` error docs) - rust-lang#137303 (Remove `MaybeForgetReturn` suggestion) - rust-lang#137327 (Undeprecate env::home_dir) - rust-lang#137463 ([illumos] attempt to use posix_spawn to spawn processes) - rust-lang#137534 ([rustdoc] hide item that is not marked as doc(inline) and whose src is doc(hidden)) - rust-lang#137829 (Stabilize [T]::split_off... methods) r? `@ghost` `@rustbot` modify labels: rollup

…gillot Remove `MaybeForgetReturn` suggestion rust-lang#115196 implemented a suggestion to add a missing `return` when there is an ambiguity error, when that ambiguity error could be constrained by the return type of the function. I initially reviewed it and thought it could be useful; however, looking back at that code now, I feel like it's a bit too much of a hack to be worth keeping around in typeck, especially given how rare it's expected to fire in practice. This is especially true because it depends on `StashKey::MaybeForgetReturn`, which is only stashed when we have *Sized* obligation ambiguity errors. Let's remove it for now. I'd like to note that it's basically impossible to get this suggestion to apply in its current state except for what I'd consider somewhat artificial examples, involving no generic trait bounds. For example, it's not triggered for: ```rust struct W<T>(T); fn bar<T: Default>() -> W<T> { todo!() } fn foo() -> W<i32> { if true { bar(); } W(0) } ``` Nor is it triggered for: ``` fn foo() -> i32 { if true { Default::default(); } 0 } ``` It's basically only triggered iff there's only one ambiguity error on the type, which is `Sized`. Generally, suggesting something that affects control flow is a pretty dramatic suggestion; therefore, both the accuracy and precision of this diagnostic should be pretty high. One other, somewhat unrelated observation is that this might be using stashed diagnostics incorrectly (or at least unnecessarily). Stashed diagnostics are used when error detection is fragmented over several major stages of the compiler, like a parse or resolver error which later can be recovered in typeck. However, this one is a bit different since it is fully handled within typeck -- perhaps that suggests that if this were to be reimplemented, it wouldn't need to be so complicated of an implementation.

Rollup of 10 pull requests Successful merges: - rust-lang#136581 (Retire the legacy `Makefile`-based `run-make` test infra) - rust-lang#136865 (Perform deeper compiletest path normalization for `$TEST_BUILD_DIR` to account for compare-mode/debugger cases, and normalize long type file filename hashes) - rust-lang#136975 (Look for `python3` first on MacOS, not `py`) - rust-lang#137240 (Slightly reformat `std::fs::remove_dir_all` error docs) - rust-lang#137303 (Remove `MaybeForgetReturn` suggestion) - rust-lang#137634 (Update `compiler-builtins` to 0.1.149) - rust-lang#137679 (Various coretests improvements) - rust-lang#137685 (self-contained linker: conservatively default to `-znostart-stop-gc`) - rust-lang#137850 (Stabilize `box_uninit_write`) - rust-lang#137947 (Do not install rustup on Rust for Linux job) r? `@ghost` `@rustbot` modify labels: rollup

…gillot Remove `MaybeForgetReturn` suggestion rust-lang#115196 implemented a suggestion to add a missing `return` when there is an ambiguity error, when that ambiguity error could be constrained by the return type of the function. I initially reviewed it and thought it could be useful; however, looking back at that code now, I feel like it's a bit too much of a hack to be worth keeping around in typeck, especially given how rare it's expected to fire in practice. This is especially true because it depends on `StashKey::MaybeForgetReturn`, which is only stashed when we have *Sized* obligation ambiguity errors. Let's remove it for now. I'd like to note that it's basically impossible to get this suggestion to apply in its current state except for what I'd consider somewhat artificial examples, involving no generic trait bounds. For example, it's not triggered for: ```rust struct W<T>(T); fn bar<T: Default>() -> W<T> { todo!() } fn foo() -> W<i32> { if true { bar(); } W(0) } ``` Nor is it triggered for: ``` fn foo() -> i32 { if true { Default::default(); } 0 } ``` It's basically only triggered iff there's only one ambiguity error on the type, which is `Sized`. Generally, suggesting something that affects control flow is a pretty dramatic suggestion; therefore, both the accuracy and precision of this diagnostic should be pretty high. One other, somewhat unrelated observation is that this might be using stashed diagnostics incorrectly (or at least unnecessarily). Stashed diagnostics are used when error detection is fragmented over several major stages of the compiler, like a parse or resolver error which later can be recovered in typeck. However, this one is a bit different since it is fully handled within typeck -- perhaps that suggests that if this were to be reimplemented, it wouldn't need to be so complicated of an implementation.

…kingjubilee Rollup of 33 pull requests Successful merges: - rust-lang#136581 (Retire the legacy `Makefile`-based `run-make` test infra) - rust-lang#136764 (Make `ptr_cast_add_auto_to_object` lint into hard error) - rust-lang#136798 (Added documentation for flushing per rust-lang#74348) - rust-lang#136865 (Perform deeper compiletest path normalization for `$TEST_BUILD_DIR` to account for compare-mode/debugger cases, and normalize long type file filename hashes) - rust-lang#136975 (Look for `python3` first on MacOS, not `py`) - rust-lang#137240 (Slightly reformat `std::fs::remove_dir_all` error docs) - rust-lang#137303 (Remove `MaybeForgetReturn` suggestion) - rust-lang#137327 (Undeprecate env::home_dir) - rust-lang#137502 (Don't include global asm in `mir_keys`, fix error body synthesis) - rust-lang#137534 ([rustdoc] hide item that is not marked as doc(inline) and whose src is doc(hidden)) - rust-lang#137565 (Try to point of macro expansion from resolver and method errors if it involves macro var) - rust-lang#137643 (Add DWARF test case for non-C-like `repr128` enums) - rust-lang#137758 (fix usage of ty decl macro fragments in attributes) - rust-lang#137764 (Ensure that negative auto impls are always applicable) - rust-lang#137772 (Fix char count in `Display` for `ByteStr`) - rust-lang#137798 (ci: use ubuntu 24 on arm large runner) - rust-lang#137805 (adjust Layout debug printing to match the internal field name) - rust-lang#137808 (Do not require that unsafe fields lack drop glue) - rust-lang#137820 (Clarify why InhabitedPredicate::instantiate_opt exists) - rust-lang#137825 (Provide more context on resolve error caused from incorrect RTN) - rust-lang#137829 (Stabilize [T]::split_off... methods) - rust-lang#137834 (rustc_fluent_macro: use CARGO_CRATE_NAME instead of CARGO_PKG_NAME) - rust-lang#137850 (Stabilize `box_uninit_write`) - rust-lang#137912 (Do not recover missing lifetime with random in-scope lifetime) - rust-lang#137913 (Allow struct field default values to reference struct's generics) - rust-lang#137923 (Simplify `<Postorder as Iterator>::size_hint`) - rust-lang#137949 (Update MSVC INSTALL.md instructions to recommend VS 2022 + recent Windows 10/11 SDK) - rust-lang#137963 (Add ``dyn`` keyword to `E0373` examples) - rust-lang#137975 (Remove unused `PpMode::needs_hir`) - rust-lang#137986 (Fix some typos) - rust-lang#137991 (Add `avr-none` to SUMMARY.md and platform-support.md) - rust-lang#137993 (Remove obsolete comment from DeduceReadOnly) - rust-lang#137996 (Revert "compiler/rustc_data_structures/src/sync/worker_local.rs: delete "unsafe impl Sync"") r? `@ghost` `@rustbot` modify labels: rollup

…gillot Remove `MaybeForgetReturn` suggestion rust-lang#115196 implemented a suggestion to add a missing `return` when there is an ambiguity error, when that ambiguity error could be constrained by the return type of the function. I initially reviewed it and thought it could be useful; however, looking back at that code now, I feel like it's a bit too much of a hack to be worth keeping around in typeck, especially given how rare it's expected to fire in practice. This is especially true because it depends on `StashKey::MaybeForgetReturn`, which is only stashed when we have *Sized* obligation ambiguity errors. Let's remove it for now. I'd like to note that it's basically impossible to get this suggestion to apply in its current state except for what I'd consider somewhat artificial examples, involving no generic trait bounds. For example, it's not triggered for: ```rust struct W<T>(T); fn bar<T: Default>() -> W<T> { todo!() } fn foo() -> W<i32> { if true { bar(); } W(0) } ``` Nor is it triggered for: ``` fn foo() -> i32 { if true { Default::default(); } 0 } ``` It's basically only triggered iff there's only one ambiguity error on the type, which is `Sized`. Generally, suggesting something that affects control flow is a pretty dramatic suggestion; therefore, both the accuracy and precision of this diagnostic should be pretty high. One other, somewhat unrelated observation is that this might be using stashed diagnostics incorrectly (or at least unnecessarily). Stashed diagnostics are used when error detection is fragmented over several major stages of the compiler, like a parse or resolver error which later can be recovered in typeck. However, this one is a bit different since it is fully handled within typeck -- perhaps that suggests that if this were to be reimplemented, it wouldn't need to be so complicated of an implementation.

…kingjubilee Rollup of 20 pull requests Successful merges: - rust-lang#136581 (Retire the legacy `Makefile`-based `run-make` test infra) - rust-lang#136764 (Make `ptr_cast_add_auto_to_object` lint into hard error) - rust-lang#136798 (Added documentation for flushing per rust-lang#74348) - rust-lang#136865 (Perform deeper compiletest path normalization for `$TEST_BUILD_DIR` to account for compare-mode/debugger cases, and normalize long type file filename hashes) - rust-lang#136975 (Look for `python3` first on MacOS, not `py`) - rust-lang#137240 (Slightly reformat `std::fs::remove_dir_all` error docs) - rust-lang#137303 (Remove `MaybeForgetReturn` suggestion) - rust-lang#137327 (Undeprecate env::home_dir) - rust-lang#137502 (Don't include global asm in `mir_keys`, fix error body synthesis) - rust-lang#137534 ([rustdoc] hide item that is not marked as doc(inline) and whose src is doc(hidden)) - rust-lang#137565 (Try to point of macro expansion from resolver and method errors if it involves macro var) - rust-lang#137643 (Add DWARF test case for non-C-like `repr128` enums) - rust-lang#137758 (fix usage of ty decl macro fragments in attributes) - rust-lang#137764 (Ensure that negative auto impls are always applicable) - rust-lang#137772 (Fix char count in `Display` for `ByteStr`) - rust-lang#137798 (ci: use ubuntu 24 on arm large runner) - rust-lang#137805 (adjust Layout debug printing to match the internal field name) - rust-lang#137808 (Do not require that unsafe fields lack drop glue) - rust-lang#137820 (Clarify why InhabitedPredicate::instantiate_opt exists) - rust-lang#137825 (Provide more context on resolve error caused from incorrect RTN) r? `@ghost` `@rustbot` modify labels: rollup

…gillot Remove `MaybeForgetReturn` suggestion rust-lang#115196 implemented a suggestion to add a missing `return` when there is an ambiguity error, when that ambiguity error could be constrained by the return type of the function. I initially reviewed it and thought it could be useful; however, looking back at that code now, I feel like it's a bit too much of a hack to be worth keeping around in typeck, especially given how rare it's expected to fire in practice. This is especially true because it depends on `StashKey::MaybeForgetReturn`, which is only stashed when we have *Sized* obligation ambiguity errors. Let's remove it for now. I'd like to note that it's basically impossible to get this suggestion to apply in its current state except for what I'd consider somewhat artificial examples, involving no generic trait bounds. For example, it's not triggered for: ```rust struct W<T>(T); fn bar<T: Default>() -> W<T> { todo!() } fn foo() -> W<i32> { if true { bar(); } W(0) } ``` Nor is it triggered for: ``` fn foo() -> i32 { if true { Default::default(); } 0 } ``` It's basically only triggered iff there's only one ambiguity error on the type, which is `Sized`. Generally, suggesting something that affects control flow is a pretty dramatic suggestion; therefore, both the accuracy and precision of this diagnostic should be pretty high. One other, somewhat unrelated observation is that this might be using stashed diagnostics incorrectly (or at least unnecessarily). Stashed diagnostics are used when error detection is fragmented over several major stages of the compiler, like a parse or resolver error which later can be recovered in typeck. However, this one is a bit different since it is fully handled within typeck -- perhaps that suggests that if this were to be reimplemented, it wouldn't need to be so complicated of an implementation.

…kingjubilee Rollup of 25 pull requests Successful merges: - rust-lang#134063 (dec2flt: Clean up float parsing modules) - rust-lang#136581 (Retire the legacy `Makefile`-based `run-make` test infra) - rust-lang#136662 (Count char width at most once in `Formatter::pad`) - rust-lang#136798 (Added documentation for flushing per rust-lang#74348) - rust-lang#137240 (Slightly reformat `std::fs::remove_dir_all` error docs) - rust-lang#137303 (Remove `MaybeForgetReturn` suggestion) - rust-lang#137327 (Undeprecate env::home_dir) - rust-lang#137463 ([illumos] attempt to use posix_spawn to spawn processes) - rust-lang#137477 (uefi: Add Service Binding Protocol abstraction) - rust-lang#137565 (Try to point of macro expansion from resolver and method errors if it involves macro var) - rust-lang#137569 (Stabilize `string_extend_from_within`) - rust-lang#137612 (Update bootstrap to edition 2024) - rust-lang#137633 (Only use implied bounds hack if bevy, and use deeply normalize in implied bounds hack) - rust-lang#137643 (Add DWARF test case for non-C-like `repr128` enums) - rust-lang#137679 (Various coretests improvements) - rust-lang#137723 (Make `rust.description` more general-purpose and pass `CFG_VER_DESCRIPTION`) - rust-lang#137758 (fix usage of ty decl macro fragments in attributes) - rust-lang#137764 (Ensure that negative auto impls are always applicable) - rust-lang#137772 (Fix char count in `Display` for `ByteStr`) - rust-lang#137798 (ci: use ubuntu 24 on arm large runner) - rust-lang#137805 (adjust Layout debug printing to match the internal field name) - rust-lang#137808 (Do not require that unsafe fields lack drop glue) - rust-lang#137820 (Clarify why InhabitedPredicate::instantiate_opt exists) - rust-lang#137825 (Provide more context on resolve error caused from incorrect RTN) - rust-lang#138028 (compiler: add `ExternAbi::is_rustic_abi`) r? `@ghost` `@rustbot` modify labels: rollup

…gillot Remove `MaybeForgetReturn` suggestion rust-lang#115196 implemented a suggestion to add a missing `return` when there is an ambiguity error, when that ambiguity error could be constrained by the return type of the function. I initially reviewed it and thought it could be useful; however, looking back at that code now, I feel like it's a bit too much of a hack to be worth keeping around in typeck, especially given how rare it's expected to fire in practice. This is especially true because it depends on `StashKey::MaybeForgetReturn`, which is only stashed when we have *Sized* obligation ambiguity errors. Let's remove it for now. I'd like to note that it's basically impossible to get this suggestion to apply in its current state except for what I'd consider somewhat artificial examples, involving no generic trait bounds. For example, it's not triggered for: ```rust struct W<T>(T); fn bar<T: Default>() -> W<T> { todo!() } fn foo() -> W<i32> { if true { bar(); } W(0) } ``` Nor is it triggered for: ``` fn foo() -> i32 { if true { Default::default(); } 0 } ``` It's basically only triggered iff there's only one ambiguity error on the type, which is `Sized`. Generally, suggesting something that affects control flow is a pretty dramatic suggestion; therefore, both the accuracy and precision of this diagnostic should be pretty high. One other, somewhat unrelated observation is that this might be using stashed diagnostics incorrectly (or at least unnecessarily). Stashed diagnostics are used when error detection is fragmented over several major stages of the compiler, like a parse or resolver error which later can be recovered in typeck. However, this one is a bit different since it is fully handled within typeck -- perhaps that suggests that if this were to be reimplemented, it wouldn't need to be so complicated of an implementation.

Rollup of 23 pull requests Successful merges: - rust-lang#137303 (Remove `MaybeForgetReturn` suggestion) - rust-lang#137327 (Undeprecate env::home_dir) - rust-lang#137358 (Match Ergonomics 2024: add context and examples to the unstable book) - rust-lang#137534 ([rustdoc] hide item that is not marked as doc(inline) and whose src is doc(hidden)) - rust-lang#137565 (Try to point of macro expansion from resolver and method errors if it involves macro var) - rust-lang#137637 (Check dyn flavor before registering upcast goal on wide pointer cast in MIR typeck) - rust-lang#137643 (Add DWARF test case for non-C-like `repr128` enums) - rust-lang#137744 (Re-add `Clone`-derive on `Thir`) - rust-lang#137758 (fix usage of ty decl macro fragments in attributes) - rust-lang#137764 (Ensure that negative auto impls are always applicable) - rust-lang#137772 (Fix char count in `Display` for `ByteStr`) - rust-lang#137798 (ci: use ubuntu 24 on arm large runner) - rust-lang#137802 (miri native-call support: all previously exposed provenance is accessible to the callee) - rust-lang#137805 (adjust Layout debug printing to match the internal field name) - rust-lang#137808 (Do not require that unsafe fields lack drop glue) - rust-lang#137820 (Clarify why InhabitedPredicate::instantiate_opt exists) - rust-lang#137825 (Provide more context on resolve error caused from incorrect RTN) - rust-lang#137910 (Improve error message for `AsyncFn` trait failure for RPIT) - rust-lang#137920 (interpret/provenance_map: consistently use range_is_empty) - rust-lang#138038 (Update `compiler-builtins` to 0.1.151) - rust-lang#138041 (bootstrap and compiletest: Use `size_of_val` from the prelude instead of imported) - rust-lang#138046 (trim channel value in `get_closest_merge_commit`) - rust-lang#138053 (Increase the max. custom try jobs requested to `20`) r? `@ghost` `@rustbot` modify labels: rollup

…gillot Remove `MaybeForgetReturn` suggestion rust-lang#115196 implemented a suggestion to add a missing `return` when there is an ambiguity error, when that ambiguity error could be constrained by the return type of the function. I initially reviewed it and thought it could be useful; however, looking back at that code now, I feel like it's a bit too much of a hack to be worth keeping around in typeck, especially given how rare it's expected to fire in practice. This is especially true because it depends on `StashKey::MaybeForgetReturn`, which is only stashed when we have *Sized* obligation ambiguity errors. Let's remove it for now. I'd like to note that it's basically impossible to get this suggestion to apply in its current state except for what I'd consider somewhat artificial examples, involving no generic trait bounds. For example, it's not triggered for: ```rust struct W<T>(T); fn bar<T: Default>() -> W<T> { todo!() } fn foo() -> W<i32> { if true { bar(); } W(0) } ``` Nor is it triggered for: ``` fn foo() -> i32 { if true { Default::default(); } 0 } ``` It's basically only triggered iff there's only one ambiguity error on the type, which is `Sized`. Generally, suggesting something that affects control flow is a pretty dramatic suggestion; therefore, both the accuracy and precision of this diagnostic should be pretty high. One other, somewhat unrelated observation is that this might be using stashed diagnostics incorrectly (or at least unnecessarily). Stashed diagnostics are used when error detection is fragmented over several major stages of the compiler, like a parse or resolver error which later can be recovered in typeck. However, this one is a bit different since it is fully handled within typeck -- perhaps that suggests that if this were to be reimplemented, it wouldn't need to be so complicated of an implementation.

Rollup of 29 pull requests Successful merges: - rust-lang#135733 (Implement `&pin const self` and `&pin mut self` sugars) - rust-lang#137107 (Override default `Write` methods for cursor-like types) - rust-lang#137303 (Remove `MaybeForgetReturn` suggestion) - rust-lang#137327 (Undeprecate env::home_dir) - rust-lang#137358 (Match Ergonomics 2024: add context and examples to the unstable book) - rust-lang#137534 ([rustdoc] hide item that is not marked as doc(inline) and whose src is doc(hidden)) - rust-lang#137565 (Try to point of macro expansion from resolver and method errors if it involves macro var) - rust-lang#137612 (Update bootstrap to edition 2024) - rust-lang#137637 (Check dyn flavor before registering upcast goal on wide pointer cast in MIR typeck) - rust-lang#137643 (Add DWARF test case for non-C-like `repr128` enums) - rust-lang#137685 (self-contained linker: conservatively default to `-znostart-stop-gc` on x64 linux) - rust-lang#137744 (Re-add `Clone`-derive on `Thir`) - rust-lang#137758 (fix usage of ty decl macro fragments in attributes) - rust-lang#137764 (Ensure that negative auto impls are always applicable) - rust-lang#137772 (Fix char count in `Display` for `ByteStr`) - rust-lang#137798 (ci: use ubuntu 24 on arm large runner) - rust-lang#137802 (miri native-call support: all previously exposed provenance is accessible to the callee) - rust-lang#137805 (adjust Layout debug printing to match the internal field name) - rust-lang#137808 (Do not require that unsafe fields lack drop glue) - rust-lang#137820 (Clarify why InhabitedPredicate::instantiate_opt exists) - rust-lang#137825 (Provide more context on resolve error caused from incorrect RTN) - rust-lang#137827 (Add timestamp to unstable feature usage metrics) - rust-lang#137832 (Fix crash in BufReader::peek()) - rust-lang#137910 (Improve error message for `AsyncFn` trait failure for RPIT) - rust-lang#137920 (interpret/provenance_map: consistently use range_is_empty) - rust-lang#138038 (Update `compiler-builtins` to 0.1.151) - rust-lang#138046 (trim channel value in `get_closest_merge_commit`) - rust-lang#138052 (strip `-Wlinker-messages` wrappers from `rust-lld` rmake test) - rust-lang#138053 (Increase the max. custom try jobs requested to `20`) r? `@ghost` `@rustbot` modify labels: rollup

…gillot Remove `MaybeForgetReturn` suggestion rust-lang#115196 implemented a suggestion to add a missing `return` when there is an ambiguity error, when that ambiguity error could be constrained by the return type of the function. I initially reviewed it and thought it could be useful; however, looking back at that code now, I feel like it's a bit too much of a hack to be worth keeping around in typeck, especially given how rare it's expected to fire in practice. This is especially true because it depends on `StashKey::MaybeForgetReturn`, which is only stashed when we have *Sized* obligation ambiguity errors. Let's remove it for now. I'd like to note that it's basically impossible to get this suggestion to apply in its current state except for what I'd consider somewhat artificial examples, involving no generic trait bounds. For example, it's not triggered for: ```rust struct W<T>(T); fn bar<T: Default>() -> W<T> { todo!() } fn foo() -> W<i32> { if true { bar(); } W(0) } ``` Nor is it triggered for: ``` fn foo() -> i32 { if true { Default::default(); } 0 } ``` It's basically only triggered iff there's only one ambiguity error on the type, which is `Sized`. Generally, suggesting something that affects control flow is a pretty dramatic suggestion; therefore, both the accuracy and precision of this diagnostic should be pretty high. One other, somewhat unrelated observation is that this might be using stashed diagnostics incorrectly (or at least unnecessarily). Stashed diagnostics are used when error detection is fragmented over several major stages of the compiler, like a parse or resolver error which later can be recovered in typeck. However, this one is a bit different since it is fully handled within typeck -- perhaps that suggests that if this were to be reimplemented, it wouldn't need to be so complicated of an implementation.

…compiler-errors Rollup of 20 pull requests Successful merges: - rust-lang#137303 (Remove `MaybeForgetReturn` suggestion) - rust-lang#137327 (Undeprecate env::home_dir) - rust-lang#137358 (Match Ergonomics 2024: add context and examples to the unstable book) - rust-lang#137502 (Don't include global asm in `mir_keys`, fix error body synthesis) - rust-lang#137534 ([rustdoc] hide item that is not marked as doc(inline) and whose src is doc(hidden)) - rust-lang#137565 (Try to point of macro expansion from resolver and method errors if it involves macro var) - rust-lang#137637 (Check dyn flavor before registering upcast goal on wide pointer cast in MIR typeck) - rust-lang#137643 (Add DWARF test case for non-C-like `repr128` enums) - rust-lang#137744 (Re-add `Clone`-derive on `Thir`) - rust-lang#137758 (fix usage of ty decl macro fragments in attributes) - rust-lang#137764 (Ensure that negative auto impls are always applicable) - rust-lang#137772 (Fix char count in `Display` for `ByteStr`) - rust-lang#137798 (ci: use ubuntu 24 on arm large runner) - rust-lang#137802 (miri native-call support: all previously exposed provenance is accessible to the callee) - rust-lang#137805 (adjust Layout debug printing to match the internal field name) - rust-lang#137808 (Do not require that unsafe fields lack drop glue) - rust-lang#137820 (Clarify why InhabitedPredicate::instantiate_opt exists) - rust-lang#137825 (Provide more context on resolve error caused from incorrect RTN) - rust-lang#137868 (Add minimal platform support documentation for powerpc-unknown-linux-gnuspe) - rust-lang#137910 (Improve error message for `AsyncFn` trait failure for RPIT) r? `@ghost` `@rustbot` modify labels: rollup

…mpiler-errors Rollup of 20 pull requests Successful merges: - rust-lang#137303 (Remove `MaybeForgetReturn` suggestion) - rust-lang#137327 (Undeprecate env::home_dir) - rust-lang#137358 (Match Ergonomics 2024: add context and examples to the unstable book) - rust-lang#137502 (Don't include global asm in `mir_keys`, fix error body synthesis) - rust-lang#137534 ([rustdoc] hide item that is not marked as doc(inline) and whose src is doc(hidden)) - rust-lang#137565 (Try to point of macro expansion from resolver and method errors if it involves macro var) - rust-lang#137637 (Check dyn flavor before registering upcast goal on wide pointer cast in MIR typeck) - rust-lang#137643 (Add DWARF test case for non-C-like `repr128` enums) - rust-lang#137744 (Re-add `Clone`-derive on `Thir`) - rust-lang#137758 (fix usage of ty decl macro fragments in attributes) - rust-lang#137764 (Ensure that negative auto impls are always applicable) - rust-lang#137772 (Fix char count in `Display` for `ByteStr`) - rust-lang#137798 (ci: use ubuntu 24 on arm large runner) - rust-lang#137802 (miri native-call support: all previously exposed provenance is accessible to the callee) - rust-lang#137805 (adjust Layout debug printing to match the internal field name) - rust-lang#137808 (Do not require that unsafe fields lack drop glue) - rust-lang#137820 (Clarify why InhabitedPredicate::instantiate_opt exists) - rust-lang#137825 (Provide more context on resolve error caused from incorrect RTN) - rust-lang#137868 (Add minimal platform support documentation for powerpc-unknown-linux-gnuspe) - rust-lang#137910 (Improve error message for `AsyncFn` trait failure for RPIT) r? `@ghost` `@rustbot` modify labels: rollup

…mpiler-errors Rollup of 25 pull requests Successful merges: - rust-lang#135733 (Implement `&pin const self` and `&pin mut self` sugars) - rust-lang#135895 (Document workings of successors more clearly) - rust-lang#136922 (Pattern types: Avoid having to handle an Option for range ends in the type system or the HIR) - rust-lang#137303 (Remove `MaybeForgetReturn` suggestion) - rust-lang#137327 (Undeprecate env::home_dir) - rust-lang#137358 (Match Ergonomics 2024: add context and examples to the unstable book) - rust-lang#137534 ([rustdoc] hide item that is not marked as doc(inline) and whose src is doc(hidden)) - rust-lang#137565 (Try to point of macro expansion from resolver and method errors if it involves macro var) - rust-lang#137637 (Check dyn flavor before registering upcast goal on wide pointer cast in MIR typeck) - rust-lang#137643 (Add DWARF test case for non-C-like `repr128` enums) - rust-lang#137744 (Re-add `Clone`-derive on `Thir`) - rust-lang#137758 (fix usage of ty decl macro fragments in attributes) - rust-lang#137764 (Ensure that negative auto impls are always applicable) - rust-lang#137772 (Fix char count in `Display` for `ByteStr`) - rust-lang#137798 (ci: use ubuntu 24 on arm large runner) - rust-lang#137802 (miri native-call support: all previously exposed provenance is accessible to the callee) - rust-lang#137805 (adjust Layout debug printing to match the internal field name) - rust-lang#137808 (Do not require that unsafe fields lack drop glue) - rust-lang#137820 (Clarify why InhabitedPredicate::instantiate_opt exists) - rust-lang#137825 (Provide more context on resolve error caused from incorrect RTN) - rust-lang#137834 (rustc_fluent_macro: use CARGO_CRATE_NAME instead of CARGO_PKG_NAME) - rust-lang#137868 (Add minimal platform support documentation for powerpc-unknown-linux-gnuspe) - rust-lang#137910 (Improve error message for `AsyncFn` trait failure for RPIT) - rust-lang#137920 (interpret/provenance_map: consistently use range_is_empty) - rust-lang#138038 (Update `compiler-builtins` to 0.1.151) r? `@ghost` `@rustbot` modify labels: rollup

Rollup merge of rust-lang#137303 - compiler-errors:maybe-forgor, r=cjgillot Remove `MaybeForgetReturn` suggestion rust-lang#115196 implemented a suggestion to add a missing `return` when there is an ambiguity error, when that ambiguity error could be constrained by the return type of the function. I initially reviewed it and thought it could be useful; however, looking back at that code now, I feel like it's a bit too much of a hack to be worth keeping around in typeck, especially given how rare it's expected to fire in practice. This is especially true because it depends on `StashKey::MaybeForgetReturn`, which is only stashed when we have *Sized* obligation ambiguity errors. Let's remove it for now. I'd like to note that it's basically impossible to get this suggestion to apply in its current state except for what I'd consider somewhat artificial examples, involving no generic trait bounds. For example, it's not triggered for: ```rust struct W<T>(T); fn bar<T: Default>() -> W<T> { todo!() } fn foo() -> W<i32> { if true { bar(); } W(0) } ``` Nor is it triggered for: ``` fn foo() -> i32 { if true { Default::default(); } 0 } ``` It's basically only triggered iff there's only one ambiguity error on the type, which is `Sized`. Generally, suggesting something that affects control flow is a pretty dramatic suggestion; therefore, both the accuracy and precision of this diagnostic should be pretty high. One other, somewhat unrelated observation is that this might be using stashed diagnostics incorrectly (or at least unnecessarily). Stashed diagnostics are used when error detection is fragmented over several major stages of the compiler, like a parse or resolver error which later can be recovered in typeck. However, this one is a bit different since it is fully handled within typeck -- perhaps that suggests that if this were to be reimplemented, it wouldn't need to be so complicated of an implementation.

rustbot assigned cjgillot Feb 20, 2025

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 Feb 20, 2025

Remove MaybeForgetReturn suggestion

9323ba5

compiler-errors force-pushed the maybe-forgor branch from 5b5681e to 9323ba5 Compare February 22, 2025 00:04

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-review Status: Awaiting review from the assignee but also interested parties. labels Mar 4, 2025

matthiaskrgr mentioned this pull request Mar 4, 2025

Rollup of 13 pull requests #137980

Closed

matthiaskrgr mentioned this pull request Mar 4, 2025

Rollup of 12 pull requests #137982

Closed

tgross35 mentioned this pull request Mar 4, 2025

Rollup of 10 pull requests #137988

Closed

jieyouxu mentioned this pull request Mar 4, 2025

Rollup of 10 pull requests #138007

Closed

workingjubilee mentioned this pull request Mar 4, 2025

Rollup of 33 pull requests #138020

Closed

workingjubilee mentioned this pull request Mar 5, 2025

Rollup of 20 pull requests #138030

Closed

workingjubilee mentioned this pull request Mar 5, 2025

Rollup of 25 pull requests #138044

Closed

jieyouxu mentioned this pull request Mar 5, 2025

Rollup of 23 pull requests #138059

Closed

jieyouxu mentioned this pull request Mar 5, 2025

Rollup of 29 pull requests #138071

Closed

jieyouxu mentioned this pull request Mar 5, 2025

Rollup of 15 pull requests #138072

Closed

compiler-errors mentioned this pull request Mar 6, 2025

Rollup of 20 pull requests #138079

Closed

Noratrieb mentioned this pull request Mar 6, 2025

Rollup of 42 pull requests #138086

Closed

compiler-errors mentioned this pull request Mar 6, 2025

Rollup of 25 pull requests #138114

Merged

bors merged commit ca9f615 into rust-lang:master Mar 7, 2025
6 checks passed

rustbot added this to the 1.87.0 milestone Mar 7, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove `MaybeForgetReturn` suggestion #137303

Remove `MaybeForgetReturn` suggestion #137303

compiler-errors commented Feb 20, 2025 •

edited

Loading

rustbot commented Feb 20, 2025

bors commented Feb 22, 2025

cjgillot commented Feb 25, 2025

compiler-errors commented Mar 4, 2025

cjgillot commented Mar 4, 2025

bors commented Mar 4, 2025

Remove MaybeForgetReturn suggestion #137303

Remove MaybeForgetReturn suggestion #137303

Conversation

compiler-errors commented Feb 20, 2025 • edited Loading

rustbot commented Feb 20, 2025

bors commented Feb 22, 2025

cjgillot commented Feb 25, 2025

compiler-errors commented Mar 4, 2025

cjgillot commented Mar 4, 2025

bors commented Mar 4, 2025

Remove `MaybeForgetReturn` suggestion #137303

Remove `MaybeForgetReturn` suggestion #137303

compiler-errors commented Feb 20, 2025 •

edited

Loading