Suggest adding `return` if the for semi which can coerce to the fn return type #115196

chenyukang · 2023-08-25T01:54:40Z

Fixes #86094
r? @estebank

compiler/rustc_hir_typeck/src/fn_ctxt/checks.rs

compiler-errors · 2023-08-30T02:02:29Z

gonna pass this over to esteban for advice on the wording, impl looks fine enough (a bit hacky, but oh well)

r? estebank

rustbot · 2023-08-30T02:02:31Z

Could not assign reviewer from: estebank.
User(s) estebank are either the PR author, already assigned, or on vacation, and there are no other candidates.
Use r? to specify someone else to assign.

bors · 2023-09-22T17:13:21Z

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

wesleywiser · 2023-10-05T14:41:05Z

tests/ui/inference/issue-86094-suggest-add-return-to-coerce-ret-ty.stderr

+   |
+LL |         Err::<T, MyError>(MyError);
+   |            ++++++++++++++
+help: you might have meant to return this to infer its type parameters


I wonder if it would make sense to just ask did you mean to return this? At least in the examples here, forgetting to write return is probably a bigger deal to the user than needing to qualify the type.

Just a thought 🙂

bors · 2023-10-06T21:36:01Z

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

chenyukang · 2023-10-07T03:34:56Z

compiler/rustc_hir_typeck/src/fn_ctxt/_impl.rs

            self.err_ctxt().report_fulfillment_errors(errors);
+            self.collect_unused_stmts_for_coerce_return_ty(errors_causecode);


@estebank
I fixed the conflicts, please have a review.

this changed because of this commit:
91b9ffe

I need to clone span and cause code from errors for collect_unused_stmts_for_coerce_return_ty.

Any better solution?

Ah, I see what's going on, report_fulfillment_errors now takes ownership, a change I made purely because we never needed access to the errors after reporting before this change.

Would it be feasible to instead modify the error cause code to carry an Option<Span> for the place where the return would be suggested? That way we don't need to leverage the "steal" mechanism nor clone the errors only iterate over them. We do something similar for errors coming from type parameters or from function arguments.

Do you mean to add a field Option<span> to ExprBindingObligation, then use ObligationCause's map_code API to update the trigger span?

I tried this solution and seems involve a wider change, and end with that we can not get the diag here if we don't use the "steal" mechanism:

rust/compiler/rustc_hir_typeck/src/fn_ctxt/checks.rs

Line 1889 in 25d38c4

diag.span_suggestion_verbose(

estebank

I'm ok with the code. I'd prefer to avoid the errors_causecode cloning, but if my proposed alternative to modify the errors in-place proves to be infeasible, we can go with that (after all, we are already in the unhappy path, this won't impact compile times).

compiler/rustc_hir_typeck/src/fn_ctxt/checks.rs

estebank · 2023-10-09T16:30:06Z

compiler/rustc_hir_typeck/src/fn_ctxt/_impl.rs

            self.err_ctxt().report_fulfillment_errors(errors);
+            self.collect_unused_stmts_for_coerce_return_ty(errors_causecode);


Ah, I see what's going on, report_fulfillment_errors now takes ownership, a change I made purely because we never needed access to the errors after reporting before this change.

Would it be feasible to instead modify the error cause code to carry an Option<Span> for the place where the return would be suggested? That way we don't need to leverage the "steal" mechanism nor clone the errors only iterate over them. We do something similar for errors coming from type parameters or from function arguments.

…oerce to the fn return type

estebank · 2023-10-15T22:10:34Z

@bors r+

bors · 2023-10-15T22:10:36Z

📌 Commit 25d38c4 has been approved by estebank

It is now in the queue for this repository.

…iaskrgr Rollup of 3 pull requests Successful merges: - rust-lang#115196 (Suggest adding `return` if the for semi which can coerce to the fn return type) - rust-lang#115955 (Stabilize `{IpAddr, Ipv6Addr}::to_canonical`) - rust-lang#116776 (Enable `review-requested` feature for rustbot) r? `@ghost` `@rustbot` modify labels: rollup

Rollup merge of rust-lang#115196 - chenyukang:yukang-fix-86094, r=estebank Suggest adding `return` if the for semi which can coerce to the fn return type Fixes rust-lang#86094 r? `@estebank`

…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 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 estebank Aug 25, 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. labels Aug 25, 2023

chenyukang force-pushed the yukang-fix-86094 branch 2 times, most recently from ed3e9f6 to af377c6 Compare August 25, 2023 02:02

compiler-errors reviewed Aug 25, 2023

View reviewed changes

compiler/rustc_hir_typeck/src/fn_ctxt/checks.rs Outdated Show resolved Hide resolved

compiler-errors reviewed Aug 25, 2023

View reviewed changes

compiler/rustc_hir_typeck/src/fn_ctxt/checks.rs Outdated Show resolved Hide resolved

compiler-errors reviewed Aug 25, 2023

View reviewed changes

chenyukang force-pushed the yukang-fix-86094 branch from af377c6 to c381f8f Compare August 25, 2023 06:01

compiler-errors reviewed Aug 25, 2023

View reviewed changes

chenyukang force-pushed the yukang-fix-86094 branch from c381f8f to a7bf3aa Compare August 25, 2023 06:16

compiler-errors self-assigned this Aug 25, 2023

chenyukang force-pushed the yukang-fix-86094 branch 2 times, most recently from 2fbc863 to f1cf816 Compare August 25, 2023 23:53

compiler-errors reviewed Aug 30, 2023

View reviewed changes

compiler/rustc_hir_typeck/src/fn_ctxt/checks.rs Outdated Show resolved Hide resolved

compiler-errors removed their assignment Aug 30, 2023

chenyukang force-pushed the yukang-fix-86094 branch from f1cf816 to f0c6f00 Compare September 3, 2023 01:22

chenyukang force-pushed the yukang-fix-86094 branch from f0c6f00 to 8f0665f Compare September 23, 2023 05:12

wesleywiser reviewed Oct 5, 2023

View reviewed changes

chenyukang force-pushed the yukang-fix-86094 branch from 8f0665f to a204ada Compare October 7, 2023 03:30

chenyukang commented Oct 7, 2023

View reviewed changes

estebank reviewed Oct 9, 2023

View reviewed changes

Suggest adding return if the type of unused semi return value can c…

25d38c4

…oerce to the fn return type

chenyukang force-pushed the yukang-fix-86094 branch from a204ada to 25d38c4 Compare October 15, 2023 14:57

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 Oct 15, 2023

matthiaskrgr mentioned this pull request Oct 16, 2023

Rollup of 3 pull requests #116782

Merged

bors merged commit 1de910f into rust-lang:master Oct 16, 2023

rustbot added this to the 1.75.0 milestone Oct 16, 2023

Veykril mentioned this pull request Oct 17, 2023

Pull upstream master 2023 10 17 ferrocene/ferrocene#54

Merged

compiler-errors mentioned this pull request Feb 20, 2025

Remove MaybeForgetReturn suggestion #137303

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Suggest adding `return` if the for semi which can coerce to the fn return type #115196

Suggest adding `return` if the for semi which can coerce to the fn return type #115196

chenyukang commented Aug 25, 2023

compiler-errors commented Aug 30, 2023

rustbot commented Aug 30, 2023

bors commented Sep 22, 2023

wesleywiser Oct 5, 2023

bors commented Oct 6, 2023

chenyukang Oct 7, 2023

estebank Oct 9, 2023

chenyukang Oct 15, 2023

estebank left a comment

estebank Oct 9, 2023

estebank commented Oct 15, 2023

bors commented Oct 15, 2023

		self.err_ctxt().report_fulfillment_errors(errors);
		self.collect_unused_stmts_for_coerce_return_ty(errors_causecode);

Suggest adding return if the for semi which can coerce to the fn return type #115196

Suggest adding return if the for semi which can coerce to the fn return type #115196

Conversation

chenyukang commented Aug 25, 2023

compiler-errors commented Aug 30, 2023

rustbot commented Aug 30, 2023

bors commented Sep 22, 2023

wesleywiser Oct 5, 2023

Choose a reason for hiding this comment

bors commented Oct 6, 2023

chenyukang Oct 7, 2023

Choose a reason for hiding this comment

estebank Oct 9, 2023

Choose a reason for hiding this comment

chenyukang Oct 15, 2023

Choose a reason for hiding this comment

estebank left a comment

Choose a reason for hiding this comment

estebank Oct 9, 2023

Choose a reason for hiding this comment

estebank commented Oct 15, 2023

bors commented Oct 15, 2023

Suggest adding `return` if the for semi which can coerce to the fn return type #115196

Suggest adding `return` if the for semi which can coerce to the fn return type #115196