Fix a error suggestion of situation when using placeholder _ as return types on function signature.

[surechen]

When return value is one of function's params like following example, we should suggest replace _ with S<'_> but not S<'static>.

fn f1(s: S<'_>) -> _ {
        s
}

fixes #125488

[rustbot]

r? @pnkfelix

rustbot has assigned @pnkfelix.
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

[pnkfelix]

I'm probably missing something obvious... but I'll ask the dumb question anyway:

Is there a reason we cannot just always use the keep_erased_ret_ty here (i.e. always use the ret ty prior to replacing ReErased with ReStatic), rather than doing the work above to compute the value of keep_erased_lifetime and then conditionally choosing whether to ret_ty or keep_erased_ret_ty?

(I'll give a local build a spin and see if I can find the answer to this myself.)

(if we do need to keep this logic, then I think I might have an easier time digesting this if the keep_erased_lifetime computation were factored out into a separate method rather than being defined inline...)

[surechen]

I'm probably missing something obvious... but I'll ask the dumb question anyway:

Is there a reason we cannot just always use the keep_erased_ret_ty here (i.e. always use the ret ty prior to replacing ReErased with ReStatic), rather than doing the work above to compute the value of keep_erased_lifetime and then conditionally choosing whether to ret_ty or keep_erased_ret_ty?

(I'll give a local build a spin and see if I can find the answer to this myself.)

(if we do need to keep this logic, then I think I might have an easier time digesting this if the keep_erased_lifetime computation were factored out into a separate method rather than being defined inline...)

Thank you.
Because the changes here only involve a suggestion from E0121.
I did not try understand the subsequent processing process carefully. If keeping the erased lifetime, it will panic in rustc_borrowck.
It seems unreasonable to continue to transfer backwards lifetime 'static which is not the suggest lifetime '_. I think this is indeed an issue worthy of further understanding.

[pnkfelix]

If keeping the erased lifetime, it will panic in rustc_borrowck.

This seems like the important detail here. I had forgotten about this constraint in rustc_borrowck.

However, I'm not sure that implies that we cannot simplify the logic here. I'm finally following through on making a local build that includes the variation I spoke of above, so hopefully i'll have my answer soon.

[pnkfelix]

So, just so I can be concrete about what I am talking about: Here is a diff on your PR that tried out locally:

Felix's attempt to eliminate the `keep_erased_lifetime` boolean

diff --git a/compiler/rustc_hir_analysis/src/collect.rs b/compiler/rustc_hir_analysis/src/collect.rs
index 19ae890bd1e..c45edba21b1 100644
--- a/compiler/rustc_hir_analysis/src/collect.rs
+++ b/compiler/rustc_hir_analysis/src/collect.rs
@@ -17,7 +17,7 @@
 use rustc_ast::Recovered;
 use rustc_data_structures::captures::Captures;
 use rustc_data_structures::fx::{FxHashSet, FxIndexMap};
-use rustc_data_structures::unord::{UnordMap, UnordSet};
+use rustc_data_structures::unord::UnordMap;
 use rustc_errors::{Applicability, Diag, ErrorGuaranteed, StashKey};
 use rustc_hir as hir;
 use rustc_hir::def::DefKind;
@@ -41,7 +41,6 @@
 use std::cell::Cell;
 use std::iter;
 use std::ops::Bound;
-use std::ops::ControlFlow;

 use crate::check::intrinsic::intrinsic_operation_unsafety;
 use crate::errors;
@@ -1365,7 +1364,7 @@ fn infer_return_ty_for_fn_sig<'tcx>(
     sig: &hir::FnSig<'tcx>,
     generics: &hir::Generics<'_>,
     def_id: LocalDefId,
-    body_id: Option<hir::BodyId>,
+    _body_id: Option<hir::BodyId>,
     icx: &ItemCtxt<'tcx>,
 ) -> ty::PolyFnSig<'tcx> {
     let hir_id = tcx.local_def_id_to_hir_id(def_id);
@@ -1393,68 +1392,6 @@ fn infer_return_ty_for_fn_sig<'tcx>(
             // }
             // -----------------------^--
             //           We should suggest replace `_` with `S<'_>`.
-            let mut keep_erased_lifetime = false;
-            if let Some(body_id) = body_id {
-                struct RetVisitor {
-                    res_hir_ids: UnordSet<hir::HirId>,
-                }
-
-                impl<'v> Visitor<'v> for RetVisitor {
-                    type Result = ControlFlow<()>;
-
-                    fn visit_expr(&mut self, ex: &'v hir::Expr<'v>) -> Self::Result {
-                        if let hir::ExprKind::Path(hir::QPath::Resolved(_, path)) = ex.kind
-                            && let hir::def::Res::Local(hir_id) = path.res
-                            && self.res_hir_ids.contains(&hir_id)
-                        {
-                            ControlFlow::Break(())
-                        } else if let hir::ExprKind::If(_, expr, _) = ex.kind
-                            && let hir::ExprKind::Block(block, _) = expr.kind
-                        {
-                            self.visit_block(block)
-                        } else {
-                            ControlFlow::Continue(())
-                        }
-                    }
-
-                    fn visit_block(&mut self, b: &'v hir::Block<'v>) -> Self::Result {
-                        if let Some(ret) = b.expr {
-                            self.visit_expr(ret)
-                        } else if let Some(ret) = b.stmts.last() {
-                            self.visit_stmt(ret)
-                        } else {
-                            ControlFlow::Continue(())
-                        }
-                    }
-
-                    fn visit_stmt(&mut self, s: &'v hir::Stmt<'v>) -> Self::Result {
-                        if let hir::StmtKind::Semi(expr) = s.kind
-                            && let hir::ExprKind::Ret(Some(ret)) = expr.kind
-                        {
-                            self.visit_expr(ret)
-                        } else {
-                            ControlFlow::Continue(())
-                        }
-                    }
-
-                    fn visit_item(&mut self, _i: &'v hir::Item<'v>) -> Self::Result {
-                        ControlFlow::Continue(())
-                    }
-                }
-
-                let body = tcx.hir().body(body_id);
-                if let hir::ExprKind::Block(b, _) = body.value.kind {
-                    let mut res_hir_ids = UnordSet::new();
-                    for param in body.params {
-                        res_hir_ids.insert(param.pat.hir_id);
-                    }
-                    let mut ret_visitor = RetVisitor { res_hir_ids };
-                    if let ControlFlow::Break(()) = ret_visitor.visit_block(b) {
-                        keep_erased_lifetime = true;
-                    }
-                }
-            }
-
             let mut diag = bad_placeholder(tcx, visitor.0, "return type");
             let ret_ty = fn_sig.output();
             // Don't leak types into signatures unless they're nameable!
@@ -1468,9 +1405,7 @@ fn visit_item(&mut self, _i: &'v hir::Item<'v>) -> Self::Result {
                 diag.span_suggestion(
                     ty.span,
                     "replace with the correct return type",
-                    if keep_erased_lifetime
-                        && let Some(ty) = keep_erased_ret_ty.make_suggestable(tcx, false, None)
-                    {
+                    if let Some(ty) = keep_erased_ret_ty.make_suggestable(tcx, false, None) {
                         ty
                     } else {
                         suggestable_ret_ty

WIth that diff in place, I didn't see any new borrow check failures (I think).

Here's what I did see, in terms of changes to the UI tests:

[ui] tests/ui/suggestions/return-cycle-2.rs
[ui] tests/ui/typeck/typeck_type_placeholder_item.rs

with these associated deltas to the UI output with my patch applied:

---- [ui] tests/ui/suggestions/return-cycle-2.rs stdout ----
diff of stderr:

5          |                                  ^
6          |                                  |
7          |                                  not allowed in type signatures
-          |                                  help: replace with the correct return type: `Token<&'static T>`
+          |                                  help: replace with the correct return type: `Token<&T>`
9
10      error: aborting due to 1 previous error
11

and

---- [ui] tests/ui/typeck/typeck_type_placeholder_item.rs stdout ----
diff of stderr:

158        |                         -^
159        |                         ||
160        |                         |not allowed in type signatures
-          |                         help: replace with the correct return type: `&'static &'static usize`
+          |                         help: replace with the correct return type: `&&usize`
162
163     error[E0121]: the placeholder `_` is not allowed within types on item signatures for return types
164       --> $DIR/typeck_type_placeholder_item.rs:53:52

562        |               ----------------^-
563        |               |               |
564        |               |               not allowed in type signatures
-          |               help: replace with the correct return type: `Option<&'static u8>`
+          |               help: replace with the correct return type: `Option<&u8>`
566
567     error[E0121]: the placeholder `_` is not allowed within types on item signatures for constants
568       --> $DIR/typeck_type_placeholder_item.rs:222:10

So, my question: which is right? What should these two tests be suggesting here?

[pnkfelix]

(Also, in case its not clear: I'm not suggesting you just blindly adopt the diff I posted in the previous comment. It was meant as a sketch while I am trying to understand the significance of the different control flow branches in your PR, and it was not meant as an idealized form of the code.)

[pnkfelix]

(a further note: If I go one step further with the diff I posted above, and attempt to actually use the result of keep_erased_ret_ty.make_suggestable(tcx, false, None) as the new value that we write into recovered_ret_ty, then I do see an ICE during borrowck saying "cannot convert '{erased} to a region vid". So yes, I do entirely believe that borrowck failures are witnessable here. I'm just trying to make sure we don't overindex on that point....)

[pnkfelix]

@rustbot author

See feedback given in: #126017 (comment)

[surechen]

@rustbot author

See feedback given in: #126017 (comment)

Thank you very much for your work on this PR. It helped me a lot to understand this problem.
I tested the error examples in following:

[ui] tests/ui/suggestions/return-cycle-2.rs
[ui] tests/ui/typeck/typeck_type_placeholder_item.rs

exampl1:

use std::marker::PhantomData;
struct Token<T>(PhantomData<T>);
impl<T> Token<T> {
    // Original code:
    fn as_ref(_: i32, _: i32) -> _ {
        Token(PhantomData::<&T>)
    }

    // Correct suggestion:
    fn as_ref_ok_1(_: i32, _: i32) -> Token<&'static T> {
        Token(PhantomData::<&T>)
    }

    // Error suggestion:
    fn as_ref_ok_2(_: i32, _: i32) -> Token<&T> {
        Token(PhantomData::<&T>)
    }
}

exampl2, No matter how modify it, there is always this error: error[E0515]: cannot return reference to function parameter x
I think we don’t need to pay too much attention to the suggestions for this example, because it is not allowed code itself.:

// Original code:
fn test1(x: &usize) -> &_ {
    &x 
}

// Error suggestion:
fn test2(x: &usize) -> &'static &'static usize{
    &x
}

// Correct suggestion:
fn test3(x: &usize) -> &&usize{
    &x
}

// Correct suggestion:
fn test4(x: &usize) -> &'static &usize{
    &x
}

example3:

// Original code
fn value() -> Option<&'static _> {
    Option::<&'static u8>::None
}

// Error suggestion:
fn value_1() -> Option<&u8> {
    Option::<&'static u8>::None
}

// Correct suggestion:
fn value_ok2() -> Option<&'static u8> {
    Option::<&'static u8>::None
}

[surechen]

I could be wrong, but as far as I'm concerned:

1. If we can be sure that the return value has reference and is one of the parameters of the function, then its lifetime should be consistent with the input parameters and cannot be specified as 'static when it is not really 'static ;
2. If the reference in the return value has nothing to do with the parameters, it can only be 'static', because we cannot return a reference to a variable in the function.
   In this example, the return value's lifetime is inferred as 'static, which will also be recognized in subsequent checks:

struct S<'a>(&'a ());
fn f(s: S<'_>) -> _ {
    s
}

So maybe we can make a more accurate suggestion here for the case where the return value is a function's parameter, the way I thought is use a visitor to find this situation, maybe there are some better ways.

Could you please judge whether my idea is correct and give me suggestions for modifications in the next step?
Thank you.

[surechen]

I have adjusted the code, please help review again.
Thank you.

[pnkfelix]

thanks for your investigation of the examples I had pointed out, and for factoring the visitor into a separate subroutine. I think that has improved the readability of your PR.

I didn't mean to imply that there was a "correct" suggestion that would make the test cases start compiling; as you noted, these examples are code that is rightfully rejected by this compiler, and in these cases, I don't think mucking with the type signatures is intended to be a way to make them start compiling.

It was more of a thought experiment as to "which diagnostic is more helpful for our users mental models" -- and as you note, there are contexts where a 'static is the only sensible lifetime to use (and thus we would like to suggest that), and there are other cases where another lifetime within scope is sensible.

I'm doubling checking a detail about the visitor itself; I'll circle back later after I finish with that.

[pnkfelix]

One more note, regarding the first example you explored:

exampl1:

use std::marker::PhantomData;
struct Token<T>(PhantomData<T>);
impl<T> Token<T> {
    // Original code:
    fn as_ref(_: i32, _: i32) -> _ {
        Token(PhantomData::<&T>)
    }

    // Correct suggestion:
    fn as_ref_ok_1(_: i32, _: i32) -> Token<&'static T> {
        Token(PhantomData::<&T>)
    }

    // Error suggestion:
    fn as_ref_ok_2(_: i32, _: i32) -> Token<&T> {
        Token(PhantomData::<&T>)
    }
}

The one gotcha here is that once you have PhantomData, it is really hard to make solid inferences about the user's intent based solely on reasoning available to the Rust compiler.

Blanket rules like "'static is the only thing that can make sense" stop applying, because PhantomData is intended to let people conjure up representative instances of types even when they do not have a value of that type in hand from the compiler's perspective.

In principle, the user might have intended for any lifetime that happens to be in scope to be used there within the PhantomData.

Here's a concrete example (playpen):

use std::marker::PhantomData;
struct Token<T>(PhantomData<T>);

impl<'a, T> Token<&'a T> {
    fn as_ref_with_bad_ret_type(_: i32, _: i32) -> _ {
        Token(PhantomData::<&T>)
    }

    fn as_ref_ok_3(_: i32, _: i32) -> Token<&'a T> {
        Token(PhantomData::<&T>)
    }
}

fn foo<'a>(_: &'a i32) {
    Token::<&'a i32>::as_ref_ok_3(4, 5);
}

fn main() {
    foo(&6);
}

In particular: the compiler is accepting my as_ref_ok_3 above, and it is not returning a 'static lifetime. Instead, it is returning a value with a lifetime that comes from the implementing type itself, even though it has no self parameters and so none of the parameters being fed into as_ref_ok_3 are carrying the relevant lifetime.

(I'm not saying this is a good API in practice. I'm just saying that it is something that one can express today.)

---

Having said that: Chances are anyone who is doing this kind of stuff with PhantomData is already so deeply invested in Rust that we do not need to worry too much about whether we push them too eagerly towards &'static _.

[pnkfelix]

I'm doubling checking a detail about the visitor itself; I'll circle back later after I finish with that.

Okay, so your code is pretty interesting. It works hard to try to figure out whether it can suggest the non-static lifetime.

Two main notes:

Your visitor approach does miss some cases, such as if I return the parameter only through a level of indirection -- in this situation it still suggests S<'static> when only S<'_> would work)

struct S<'a>(&'a ());
fn g(s: S<'_>) -> _ {
    id(s)
}

fn id<T>(t: T) -> T {
    t
}

But I don't think there's a reasonable way for you to fix that via a visitor-based system. The "right" answer would involve getting help from the type inference system, and I don't think we can expect the type inference to infer a correct solution efficiently in all case. (I.e. I wouldn't be surprised if this is an NP-hard problem.)

So lets set that aside for a moment, and assume "its okay to fallback sometimes on suggesting 'static as a candidate lifetime even if it won't end up working in the long run."

---

Here's what i see as a slightly more problematic thing with your PR: I'm concerned you might end up with the compiler reporting distinct errors that, when put together, do not make sense because they come from inconsistent views of one piece of code.

Let me explain a little more carefully:

The compiler, upon encountering the -> _ in the signature, does three things:

1. it notes an error, since it knows the _ is unacceptable in the signature's return type
2. it provides a diagnostic suggestion to the user of a potentially correct type to use
3. it continues the static analysis of the rest of the program, using what it inferred to be the corrected version of the function.

So, consider the following example:

#[allow(dead_code)]
struct S<'a>(&'a mut ());

fn h(s: S<'_>) -> _ {
    s
}

fn main() {
    h(S(&mut ()));
}

with your PR in place, here is the diagnostic I get:

error[E0121]: the placeholder `_` is not allowed within types on item signatures for return types
 --> ../../demo-126017.rs:4:19
  |
4 | fn h(s: S<'_>) -> _ {
  |                   ^
  |                   |
  |                   not allowed in type signatures
  |                   help: replace with the correct return type: `S<'_>`

error[E0716]: temporary value dropped while borrowed
 --> ../../demo-126017.rs:9:14
  |
9 |     h(S(&mut ()));
  |     ---------^^--- temporary value is freed at the end of this statement
  |     |        |
  |     |        creates a temporary value which is freed while still in use
  |     argument requires that borrow lasts for `'static`

error: aborting due to 2 previous errors

If you read those two errors, it is really confusing: "where is the 'static coming from in the second message?

(Note that if one puts in the correction suggested in the first diagnostic, both errors go away.)

My theory as to why this is happening: Under your PR, the compiler encounters erroneous fn signature h, and makes two corrected versions: h_1 is one that returns S<'_> and is reported by your diagnostic, while h_2 is one that returns S<'static> and is installed by the compiler as a the "corrected" h that should be used in the rest of the static analysis (which then leads to the error reported against fn main.

---

Overall, the latter situation seems like it will yield very subtle diagnostic problems that are hard for us (the compiler mainainers) to diagnose.

So I'd prefer we try to fix up your PR to avoid getting into such a situation, if possible.

[pnkfelix]

@surechen I reached out to the T-types team (on zulip) to double-check my hypothesis (about it being impossible to provide 100% good suggestions here), and they confirmed it: We simply do not have the necessary lifetime information to actually provide the "right" diagnostic in all cases here.

So any solution we adopt is going to be based on some heuristic.

And the main questions are about 1. which heuristic is best, 2. how do we ensure the overall user experience is good, and 3. lets not overcomplicate the compiler code if we can help it.

In particular, the T-types team recommended that instead of using your ad-hoc visitor to try to guess at which lifetime to suggest, it would probably be better to abandon the universal "replace ReErased with ReStatic" strategy (since it is not producing good results when there are lifetimes in scope), and instead try a "replace ReErased with ReError" strategy.

The expected outcome of that, we hope, will be that the diagnostics will naturally then start presenting suggestions that use the '_ lifetime (at least, that is what they told me ReError would print as).

the one exception here is that we could use a more precise heuristic like "if there are no region-parameters in scope, then replace ReErased with ReStatic; otherwise, replace ReErased with ReError"

@surechen would you be willing to close this PR and explore making a PR that does the above instead?

[pnkfelix]

@rfcbot author

[surechen]

@surechen I reached out to the T-types team (on zulip) to double-check my hypothesis (about it being impossible to provide 100% good suggestions here), and they confirmed it: We simply do not have the necessary lifetime information to actually provide the "right" diagnostic in all cases here.

So any solution we adopt is going to be based on some heuristic.

And the main questions are about 1. which heuristic is best, 2. how do we ensure the overall user experience is good, and 3. lets not overcomplicate the compiler code if we can help it.

In particular, the T-types team recommended that instead of using your ad-hoc visitor to try to guess at which lifetime to suggest, it would probably be better to abandon the universal "replace ReErased with ReStatic" strategy (since it is not producing good results when there are lifetimes in scope), and instead try a "replace ReErased with ReError" strategy.

The expected outcome of that, we hope, will be that the diagnostics will naturally then start presenting suggestions that use the '_ lifetime (at least, that is what they told me ReError would print as).

the one exception here is that we could use a more precise heuristic like "if there are no region-parameters in scope, then replace ReErased with ReStatic; otherwise, replace ReErased with ReError"

@surechen would you be willing to close this PR and explore making a PR that does the above instead?

Thank you very much.
Your answer is very detailed and clear. I will try it as soon as possible based on your guidance.