Suggest adding `return` if the for semi which can coerce to the fn return type by chenyukang · Pull Request #115196 · rust-lang/rust (original) (raw)

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation37 Commits1 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

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

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

r? estebank

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.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 🙂

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

diag.span_suggestion_verbose(

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

…oerce to the fn return type

📌 Commit 25d38c4 has been approved by estebank

It is now in the queue for this repository.

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

bors added a commit to rust-lang-ci/rust that referenced this pull request

Oct 16, 2023

…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

rust-timer added a commit to rust-lang-ci/rust that referenced this pull request

Oct 16, 2023

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

bors-ferrocene bot added a commit to ferrocene/ferrocene that referenced this pull request

Oct 17, 2023

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request

Mar 4, 2025

…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:

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.

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request

Mar 4, 2025

…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.

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.

tgross35 added a commit to tgross35/rust that referenced this pull request

Mar 4, 2025

…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.

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.

jieyouxu added a commit to jieyouxu/rust that referenced this pull request

Mar 4, 2025

…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.

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.

workingjubilee added a commit to workingjubilee/rustc that referenced this pull request

Mar 4, 2025

…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.

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.

workingjubilee added a commit to workingjubilee/rustc that referenced this pull request

Mar 5, 2025

…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.

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.

workingjubilee added a commit to workingjubilee/rustc that referenced this pull request

Mar 5, 2025

…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.

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.

jieyouxu added a commit to jieyouxu/rust that referenced this pull request

Mar 5, 2025

…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.

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.

jieyouxu added a commit to jieyouxu/rust that referenced this pull request

Mar 5, 2025

…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.

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.

jieyouxu added a commit to jieyouxu/rust that referenced this pull request

Mar 5, 2025

…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.

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.

jieyouxu added a commit to jieyouxu/rust that referenced this pull request

Mar 5, 2025

…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.

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.

compiler-errors added a commit to compiler-errors/rust that referenced this pull request

Mar 6, 2025

…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.

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.

Noratrieb added a commit to Noratrieb/rust that referenced this pull request

Mar 6, 2025

…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.

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.

compiler-errors added a commit to compiler-errors/rust that referenced this pull request

Mar 6, 2025

…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.

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.

rust-timer added a commit to rust-lang-ci/rust that referenced this pull request

Mar 7, 2025

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.

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.

Labels

S-waiting-on-bors

Status: Waiting on bors to run and complete tests. Bors will change the label on completion.

T-compiler

Relevant to the compiler team, which will review and decide on the PR/issue.

Suggest adding return if the for semi which can coerce to the fn return type by chenyukang · Pull Request #115196 · rust-lang/rust (original) (raw)

Conversation

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Suggest adding `return` if the for semi which can coerce to the fn return type by chenyukang · Pull Request #115196 · rust-lang/rust (original) (raw)