Skip to content

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

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.

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
105 changes: 97 additions & 8 deletions compiler/rustc_hir_analysis/src/collect.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
use rustc_data_structures::unord::{UnordMap, UnordSet};
use rustc_errors::{struct_span_code_err, Applicability, Diag, ErrorGuaranteed, StashKey, E0228};
use rustc_hir::def::DefKind;
use rustc_hir::def_id::{DefId, LocalDefId};
Expand All @@ -41,6 +41,7 @@ use rustc_trait_selection::traits::ObligationCtxt;
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;
Expand Down Expand Up @@ -1375,12 +1376,12 @@ fn fn_sig(tcx: TyCtxt<'_>, def_id: LocalDefId) -> ty::EarlyBinder<'_, ty::PolyFn
kind: TraitItemKind::Fn(sig, TraitFn::Provided(_)),
generics,
..
})
| Item(hir::Item { kind: ItemKind::Fn(sig, generics, _), .. }) => {
infer_return_ty_for_fn_sig(tcx, sig, generics, def_id, &icx)
}) => infer_return_ty_for_fn_sig(tcx, sig, generics, def_id, None, &icx),
Item(hir::Item { kind: ItemKind::Fn(sig, generics, body_id), .. }) => {
infer_return_ty_for_fn_sig(tcx, sig, generics, def_id, Some(*body_id), &icx)
}

ImplItem(hir::ImplItem { kind: ImplItemKind::Fn(sig, _), generics, .. }) => {
ImplItem(hir::ImplItem { kind: ImplItemKind::Fn(sig, body_id), generics, .. }) => {
// Do not try to infer the return type for a impl method coming from a trait
if let Item(hir::Item { kind: ItemKind::Impl(i), .. }) = tcx.parent_hir_node(hir_id)
&& i.of_trait.is_some()
Expand All @@ -1394,7 +1395,7 @@ fn fn_sig(tcx: TyCtxt<'_>, def_id: LocalDefId) -> ty::EarlyBinder<'_, ty::PolyFn
None,
)
} else {
infer_return_ty_for_fn_sig(tcx, sig, generics, def_id, &icx)
infer_return_ty_for_fn_sig(tcx, sig, generics, def_id, Some(*body_id), &icx)
}
}

Expand Down Expand Up @@ -1451,13 +1452,15 @@ fn infer_return_ty_for_fn_sig<'tcx>(
sig: &hir::FnSig<'tcx>,
generics: &hir::Generics<'_>,
def_id: LocalDefId,
body_id: Option<hir::BodyId>,
icx: &ItemCtxt<'tcx>,
) -> ty::PolyFnSig<'tcx> {
let hir_id = tcx.local_def_id_to_hir_id(def_id);

match sig.decl.output.get_infer_ret_ty() {
Some(ty) => {
let fn_sig = tcx.typeck(def_id).liberated_fn_sigs()[hir_id];
let keep_erased_ret_ty = fn_sig.output();
// Typeck doesn't expect erased regions to be returned from `type_of`.
let fn_sig = tcx.fold_regions(fn_sig, |r, _| match *r {
ty::ReErased => tcx.lifetimes.re_static,
Expand All @@ -1475,13 +1478,20 @@ fn infer_return_ty_for_fn_sig<'tcx>(
let mut recovered_ret_ty = None;

if let Some(suggestable_ret_ty) = ret_ty.make_suggestable(tcx, false, None) {
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

@pnkfelix pnkfelix Jun 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member

@pnkfelix pnkfelix Jun 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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?

Copy link
Member

@pnkfelix pnkfelix Jun 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

recovered_ret_ty = Some(suggestable_ret_ty);

diag.span_suggestion(
ty.span,
"replace with the correct return type",
suggestable_ret_ty,
if return_value_from_fn_param(tcx, body_id)
&& let Some(ty) = keep_erased_ret_ty.make_suggestable(tcx, false, None)
{
ty
} else {
suggestable_ret_ty
},
Applicability::MachineApplicable,
);
recovered_ret_ty = Some(suggestable_ret_ty);
} else if let Some(sugg) =
suggest_impl_trait(&tcx.infer_ctxt().build(), tcx.param_env(def_id), ret_ty)
{
Expand Down Expand Up @@ -1522,6 +1532,85 @@ fn infer_return_ty_for_fn_sig<'tcx>(
}
}

// When the return value of a Function is one of its params,
// we shouldn't change the `Erased` lifetime to `Static` lifetime.
// For example:
// fn main() {
// fn f1(s: S<'_>) -> _ {
// s
// }
// }
// -----------------------^--
// We should suggest replace `_` with `S<'_>`.
fn return_value_from_fn_param<'tcx>(tcx: TyCtxt<'tcx>, body_id_opt: Option<hir::BodyId>) -> bool {
let body_id = if let Some(id) = body_id_opt {
id
} else {
return false;
};

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 {
match ex.kind {
hir::ExprKind::Path(hir::QPath::Resolved(_, path))
if let hir::def::Res::Local(hir_id) = path.res
&& self.res_hir_ids.contains(&hir_id) =>
{
ControlFlow::Break(())
}
hir::ExprKind::If(_, expr, _) if let hir::ExprKind::Block(block, _) = expr.kind => {
self.visit_block(block)
}
hir::ExprKind::AddrOf(hir::BorrowKind::Ref, _, expr) => self.visit_expr(expr),
_ => 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) {
return true;
}
}
false
}

pub fn suggest_impl_trait<'tcx>(
infcx: &InferCtxt<'tcx>,
param_env: ty::ParamEnv<'tcx>,
Expand Down
33 changes: 33 additions & 0 deletions tests/ui/return/infer-return-ty-for-fn-sig-issue-125488.fixed
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
//@ run-rustfix

#[allow(dead_code)]

fn main() {
struct S<'a>(&'a ());

fn f1(s: S<'_>) -> S<'_> {
//~^ ERROR the placeholder `_` is not allowed
s
}

fn f2(s: S<'_>) -> S<'_> {
//~^ ERROR the placeholder `_` is not allowed
let x = true;
if x {
s
} else {
s
}
}

fn f3(s: S<'_>) -> S<'_> {
//~^ ERROR the placeholder `_` is not allowed
return s;
}

fn f4(s: S<'_>) -> S<'_> {
//~^ ERROR the placeholder `_` is not allowed
let _x = 1;
return s;
}
}
33 changes: 33 additions & 0 deletions tests/ui/return/infer-return-ty-for-fn-sig-issue-125488.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
//@ run-rustfix

#[allow(dead_code)]

fn main() {
struct S<'a>(&'a ());

fn f1(s: S<'_>) -> _ {
//~^ ERROR the placeholder `_` is not allowed
s
}

fn f2(s: S<'_>) -> _ {
//~^ ERROR the placeholder `_` is not allowed
let x = true;
if x {
s
} else {
s
}
}

fn f3(s: S<'_>) -> _ {
//~^ ERROR the placeholder `_` is not allowed
return s;
}

fn f4(s: S<'_>) -> _ {
//~^ ERROR the placeholder `_` is not allowed
let _x = 1;
return s;
}
}
39 changes: 39 additions & 0 deletions tests/ui/return/infer-return-ty-for-fn-sig-issue-125488.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
error[E0121]: the placeholder `_` is not allowed within types on item signatures for return types
--> $DIR/infer-return-ty-for-fn-sig-issue-125488.rs:8:24
|
LL | fn f1(s: S<'_>) -> _ {
| ^
| |
| not allowed in type signatures
| help: replace with the correct return type: `S<'_>`

error[E0121]: the placeholder `_` is not allowed within types on item signatures for return types
--> $DIR/infer-return-ty-for-fn-sig-issue-125488.rs:13:24
|
LL | fn f2(s: S<'_>) -> _ {
| ^
| |
| not allowed in type signatures
| help: replace with the correct return type: `S<'_>`

error[E0121]: the placeholder `_` is not allowed within types on item signatures for return types
--> $DIR/infer-return-ty-for-fn-sig-issue-125488.rs:23:24
|
LL | fn f3(s: S<'_>) -> _ {
| ^
| |
| not allowed in type signatures
| help: replace with the correct return type: `S<'_>`

error[E0121]: the placeholder `_` is not allowed within types on item signatures for return types
--> $DIR/infer-return-ty-for-fn-sig-issue-125488.rs:28:24
|
LL | fn f4(s: S<'_>) -> _ {
| ^
| |
| not allowed in type signatures
| help: replace with the correct return type: `S<'_>`

error: aborting due to 4 previous errors

For more information about this error, try `rustc --explain E0121`.
2 changes: 1 addition & 1 deletion tests/ui/typeck/typeck_type_placeholder_item.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,7 +158,7 @@ LL | fn test11(x: &usize) -> &_ {
| -^
| ||
| |not allowed in type signatures
| help: replace with the correct return type: `&'static &'static usize`
| help: replace with the correct return type: `&&usize`

error[E0121]: the placeholder `_` is not allowed within types on item signatures for return types
--> $DIR/typeck_type_placeholder_item.rs:53:52
Expand Down
Loading