Skip to content
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 and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix suggestions when returning a bare trait from an async fn. #131882

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions Cargo.lock
Original file line number Diff line number Diff line change
Expand Up @@ -3365,6 +3365,7 @@ dependencies = [
"rustc_fluent_macro",
"rustc_graphviz",
"rustc_hir",
"rustc_hir_analysis",
"rustc_index",
"rustc_infer",
"rustc_lexer",
Expand Down
1 change: 1 addition & 0 deletions compiler/rustc_borrowck/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ rustc_errors = { path = "../rustc_errors" }
rustc_fluent_macro = { path = "../rustc_fluent_macro" }
rustc_graphviz = { path = "../rustc_graphviz" }
rustc_hir = { path = "../rustc_hir" }
rustc_hir_analysis = { path = "../rustc_hir_analysis" }
rustc_index = { path = "../rustc_index" }
rustc_infer = { path = "../rustc_infer" }
rustc_lexer = { path = "../rustc_lexer" }
Expand Down
44 changes: 4 additions & 40 deletions compiler/rustc_borrowck/src/diagnostics/region_name.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,10 +8,11 @@ use rustc_data_structures::fx::IndexEntry;
use rustc_errors::Diag;
use rustc_hir as hir;
use rustc_hir::def::{DefKind, Res};
use rustc_hir_analysis::hir_ty_lowering::HirTyLowerer;
use rustc_middle::bug;
use rustc_middle::ty::print::RegionHighlightMode;
use rustc_middle::ty::{self, GenericArgKind, GenericArgsRef, RegionVid, Ty};
use rustc_middle::{bug, span_bug};
use rustc_span::symbol::{Symbol, kw, sym};
use rustc_span::symbol::{Symbol, kw};
use rustc_span::{DUMMY_SP, Span};
use rustc_trait_selection::error_reporting::InferCtxtErrorExt;
use tracing::{debug, instrument};
Expand Down Expand Up @@ -722,7 +723,7 @@ impl<'tcx> MirBorrowckCtxt<'_, '_, 'tcx> {
.output;
span = output.span();
if let hir::FnRetTy::Return(ret) = output {
hir_ty = Some(self.get_future_inner_return_ty(ret));
hir_ty = <dyn HirTyLowerer<'_>>::get_future_inner_return_ty(ret);
}
" of async function"
}
Expand Down Expand Up @@ -816,43 +817,6 @@ impl<'tcx> MirBorrowckCtxt<'_, '_, 'tcx> {
})
}

/// From the [`hir::Ty`] of an async function's lowered return type,
/// retrieve the `hir::Ty` representing the type the user originally wrote.
///
/// e.g. given the function:
///
/// ```
/// async fn foo() -> i32 { 2 }
/// ```
///
/// this function, given the lowered return type of `foo`, an [`OpaqueDef`] that implements `Future<Output=i32>`,
/// returns the `i32`.
///
/// [`OpaqueDef`]: hir::TyKind::OpaqueDef
fn get_future_inner_return_ty(&self, hir_ty: &'tcx hir::Ty<'tcx>) -> &'tcx hir::Ty<'tcx> {
let hir::TyKind::OpaqueDef(opaque_ty, _) = hir_ty.kind else {
span_bug!(
hir_ty.span,
"lowered return type of async fn is not OpaqueDef: {:?}",
hir_ty
);
};
if let hir::OpaqueTy { bounds: [hir::GenericBound::Trait(trait_ref)], .. } = opaque_ty
&& let Some(segment) = trait_ref.trait_ref.path.segments.last()
&& let Some(args) = segment.args
&& let [constraint] = args.constraints
&& constraint.ident.name == sym::Output
&& let Some(ty) = constraint.ty()
{
ty
} else {
span_bug!(
hir_ty.span,
"bounds from lowered return type of async fn did not match expected format: {opaque_ty:?}",
);
}
}

#[instrument(level = "trace", skip(self))]
fn give_name_if_anonymous_region_appears_in_yield_ty(
&self,
Expand Down
58 changes: 53 additions & 5 deletions compiler/rustc_hir_analysis/src/hir_ty_lowering/lint.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ use rustc_hir as hir;
use rustc_hir::def::{DefKind, Res};
use rustc_lint_defs::Applicability;
use rustc_lint_defs::builtin::BARE_TRAIT_OBJECTS;
use rustc_span::Span;
use rustc_span::{Span, sym};
use rustc_trait_selection::error_reporting::traits::suggestions::NextTypeParamName;

use super::HirTyLowerer;
Expand Down Expand Up @@ -181,14 +181,14 @@ impl<'tcx> dyn HirTyLowerer<'tcx> + '_ {
/// Make sure that we are in the condition to suggest `impl Trait`.
fn maybe_suggest_impl_trait(&self, self_ty: &hir::Ty<'_>, diag: &mut Diag<'_>) -> bool {
let tcx = self.tcx();
let parent_id = tcx.hir().get_parent_item(self_ty.hir_id).def_id;
let parent_node = tcx.hir_node_by_def_id(tcx.hir().get_parent_item(self_ty.hir_id).def_id);
// FIXME: If `type_alias_impl_trait` is enabled, also look for `Trait0<Ty = Trait1>`
// and suggest `Trait0<Ty = impl Trait1>`.
// Functions are found in three different contexts.
// 1. Independent functions
// 2. Functions inside trait blocks
// 3. Functions inside impl blocks
let (sig, generics) = match tcx.hir_node_by_def_id(parent_id) {
let (sig, generics) = match parent_node {
hir::Node::Item(hir::Item { kind: hir::ItemKind::Fn(sig, generics, _), .. }) => {
(sig, generics)
}
Expand Down Expand Up @@ -223,10 +223,17 @@ impl<'tcx> dyn HirTyLowerer<'tcx> + '_ {
tcx.parent_hir_node(self_ty.hir_id),
hir::Node::Ty(hir::Ty { kind: hir::TyKind::Ref(..), .. })
);
let is_non_trait_object = |ty: &'tcx hir::Ty<'_>| {
Copy link
Member

Choose a reason for hiding this comment

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

This feels unfortunately somewhat ad-hoc, since any place we want to inspect the hir::TyKind, like for any diagnostics, we'll need to call the get_future_inner_return_ty.

if sig.header.is_async() {
Self::get_future_inner_return_ty(ty).map_or(false, |ty| ty.hir_id == self_ty.hir_id)
} else {
ty.peel_refs().hir_id == self_ty.hir_id
}
};

// Suggestions for function return type.
if let hir::FnRetTy::Return(ty) = sig.decl.output
&& ty.peel_refs().hir_id == self_ty.hir_id
&& is_non_trait_object(ty)
{
let pre = if !is_dyn_compatible {
format!("`{trait_name}` is dyn-incompatible, ")
Expand Down Expand Up @@ -311,10 +318,21 @@ impl<'tcx> dyn HirTyLowerer<'tcx> + '_ {
}

fn maybe_suggest_assoc_ty_bound(&self, self_ty: &hir::Ty<'_>, diag: &mut Diag<'_>) {
let mut parents = self.tcx().hir().parent_iter(self_ty.hir_id);
let mut parents = self.tcx().hir().parent_iter(self_ty.hir_id).peekable();
let is_async_fn = if let Some((_, parent)) = parents.peek()
&& let Some(sig) = parent.fn_sig()
&& sig.header.is_async()
&& let hir::FnRetTy::Return(ty) = sig.decl.output
&& Self::get_future_inner_return_ty(ty).is_some()
{
true
} else {
false
};

if let Some((_, hir::Node::AssocItemConstraint(constraint))) = parents.next()
&& let Some(obj_ty) = constraint.ty()
&& !is_async_fn
{
if let Some((_, hir::Node::TraitRef(..))) = parents.next()
&& let Some((_, hir::Node::Ty(ty))) = parents.next()
Expand Down Expand Up @@ -343,4 +361,34 @@ impl<'tcx> dyn HirTyLowerer<'tcx> + '_ {
);
}
}

/// From the [`hir::Ty`] of an async function's lowered return type,
/// retrieve the `hir::Ty` representing the type the user originally wrote.
///
/// e.g. given the function:
///
/// ```
/// async fn foo() -> i32 { 2 }
/// ```
///
/// this function, given the lowered return type of `foo`, an [`OpaqueDef`] that implements `Future<Output=i32>`,
/// returns the `i32`.
///
/// [`OpaqueDef`]: hir::TyKind::OpaqueDef
pub fn get_future_inner_return_ty<'a>(hir_ty: &'a hir::Ty<'a>) -> Option<&'a hir::Ty<'a>> {
Copy link
Member

Choose a reason for hiding this comment

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

Just make this an inherent method please. There's no use of &self. Also rename the 'a to 'tcx, as it is more consistent.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This function/method was moved from Mir to Hir. I thought compiler stages should only have access to previous stages, not following ones, right? I hope this is ok.

I saw that in the definition of get_future_inner_return_ty in Mir, self was not used so I made it a non-inherent (static?) method to provide access to the one place in Mir where it is used.

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, typo, I mean make it a free function. There's no reason to put this into an impl :)

let hir::TyKind::OpaqueDef(opaque_ty, _) = hir_ty.kind else {
return None;
};
if let hir::OpaqueTy { bounds: [hir::GenericBound::Trait(trait_ref)], .. } = opaque_ty
&& let Some(segment) = trait_ref.trait_ref.path.segments.last()
&& let Some(args) = segment.args
&& let [constraint] = args.constraints
&& constraint.ident.name == sym::Output
&& let Some(ty) = constraint.ty()
{
Some(ty)
} else {
None
}
}
}
8 changes: 8 additions & 0 deletions tests/ui/traits/suggest-impl-on-bare-trait-return.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
//@ edition:2021
trait Trait {}

async fn fun() -> Trait { //~ ERROR expected a type, found a trait
todo!()
}

fn main() {}
18 changes: 18 additions & 0 deletions tests/ui/traits/suggest-impl-on-bare-trait-return.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
error[E0782]: expected a type, found a trait
--> $DIR/suggest-impl-on-bare-trait-return.rs:4:13
|
LL | fn fun() -> Trait {
| ^^^^^
|
help: use `impl Trait` to return an opaque type, as long as you return a single underlying type
|
LL | fn fun() -> impl Trait {
| ++++
help: alternatively, you can return an owned trait object
|
LL | fn fun() -> Box<dyn Trait> {
| +++++++ +

error: aborting due to 1 previous error

For more information about this error, try `rustc --explain E0782`.
Loading