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

Add shorter and more direct error for dyn AsyncFn #133267

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
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
37 changes: 37 additions & 0 deletions compiler/rustc_error_codes/src/error_codes/E0802.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
The `Async{Fn, FnMut, FnOnce}` traits are not yet dyn-compatible.

Erroneous code example:

```compile_fail,E0802,edition2018
#![feature(async_closure)]
use core::ops::AsyncFn;

async fn call_async_fn_twice(some_fn: &dyn AsyncFn()) {
some_fn().await;
some_fn().await;
}
```

One workaround to this issue is to use `impl Async...` instead:

```edition2018
#![feature(async_closure)]
use core::ops::AsyncFn;

async fn call_async_fn_twice(some_fn: &impl AsyncFn()) {
some_fn().await;
some_fn().await;
}
```

This error indicates that you attempted to use `dyn AsyncFn`,
`dyn AsyncFnMut`, or `dyn AsyncFnOnce`.

This is not yet possible because the `Async...` traits internally return
a concrete `Future` associated type. For dynamic callsites, it is impossible
to know the size of the returned `Future` object since different
`Async...` implementations may return differently-sized `Future` objects.

This is analogous to the more general issue of creating a `dyn` type without
specifying associated types, e.g. `dyn Iterator` as opposed to
`dyn Iterator<Item = SomeItemType>`.
1 change: 1 addition & 0 deletions compiler/rustc_error_codes/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -541,6 +541,7 @@ E0798: 0798,
E0799: 0799,
E0800: 0800,
E0801: 0801,
E0802: 0802,
);
)
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -103,17 +103,12 @@ impl<'tcx> dyn HirTyLowerer<'tcx> + '_ {
// most importantly, that the supertraits don't contain `Self`,
// to avoid ICEs.
for item in &regular_traits {
let violations =
hir_ty_lowering_dyn_compatibility_violations(tcx, item.trait_ref().def_id());
let item_def_id = item.trait_ref().def_id();
let violations = hir_ty_lowering_dyn_compatibility_violations(tcx, item_def_id);
if !violations.is_empty() {
let reported = report_dyn_incompatibility(
tcx,
span,
Some(hir_id),
item.trait_ref().def_id(),
&violations,
)
.emit();
let reported =
report_dyn_incompatibility(tcx, span, Some(hir_id), item_def_id, &violations)
.emit();
return Ty::new_error(tcx, reported);
}
}
Expand Down
1 change: 1 addition & 0 deletions compiler/rustc_hir_analysis/src/hir_ty_lowering/errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -733,6 +733,7 @@ impl<'tcx> dyn HirTyLowerer<'tcx> + '_ {
(span, def_ids.into_iter().map(|did| tcx.associated_item(did)).collect())
})
.collect();

let mut names: FxIndexMap<String, Vec<Symbol>> = Default::default();
let mut names_len = 0;

Expand Down
22 changes: 21 additions & 1 deletion compiler/rustc_middle/src/traits/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -712,6 +712,22 @@ pub enum DynCompatibilityViolation {

/// GAT
GAT(Symbol, Span),

/// Async{Fn, FnMut, FnOnce}
///
/// `fn_trait` is the name of the `AsyncFn...` trait,
AsyncFnTrait {
/// The `AsyncFn...` trait referenced.
///
/// This is useful for better error reporting in cases where the
/// `dyn`-incompatible trait inherits from `Async...`.
//
// FIXME(cramertj): I'd love for this to be a DefId for proper comparison
// in the error reporting stage, but sadly this isn't possible because
// DefIds cannot be stored at this stage. Is there a better way to handle
// catching the supertrait case than string comparison?
fn_trait: Symbol,
Copy link
Member

Choose a reason for hiding this comment

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

We don't actually use this anywhere?

},
}

impl DynCompatibilityViolation {
Expand Down Expand Up @@ -779,14 +795,18 @@ impl DynCompatibilityViolation {
DynCompatibilityViolation::GAT(name, _) => {
format!("it contains the generic associated type `{name}`").into()
}
DynCompatibilityViolation::AsyncFnTrait { .. } => {
"`async` function traits are not yet dyn-compatible".into()
}
}
}

pub fn solution(&self) -> DynCompatibilityViolationSolution {
match self {
DynCompatibilityViolation::SizedSelf(_)
| DynCompatibilityViolation::SupertraitSelf(_)
| DynCompatibilityViolation::SupertraitNonLifetimeBinder(..) => {
| DynCompatibilityViolation::SupertraitNonLifetimeBinder(..)
| DynCompatibilityViolation::AsyncFnTrait { .. } => {
DynCompatibilityViolationSolution::None
}
DynCompatibilityViolation::Method(
Expand Down
85 changes: 66 additions & 19 deletions compiler/rustc_trait_selection/src/error_reporting/traits/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ pub mod suggestions;
use std::{fmt, iter};

use rustc_data_structures::fx::{FxIndexMap, FxIndexSet};
use rustc_errors::{Applicability, Diag, E0038, E0276, MultiSpan, struct_span_code_err};
use rustc_errors::{Applicability, Diag, E0038, E0276, E0802, MultiSpan, struct_span_code_err};
use rustc_hir::def_id::{DefId, LocalDefId};
use rustc_hir::intravisit::Visitor;
use rustc_hir::{self as hir, LangItem};
Expand Down Expand Up @@ -428,10 +428,46 @@ pub fn report_dyn_incompatibility<'tcx>(
violations: &[DynCompatibilityViolation],
) -> Diag<'tcx> {
let trait_str = tcx.def_path_str(trait_def_id);

// Avoid errors diving into the details of the `AsyncFn` traits if this is
// a straightforward "`AsyncFn` is not yet dyn-compatible" error.
if let Some(async_fn_trait_kind) = tcx.async_fn_trait_kind_from_def_id(trait_def_id) {
let async_fn_trait_name = match async_fn_trait_kind {
ty::ClosureKind::Fn => "AsyncFn",
ty::ClosureKind::FnMut => "AsyncFnMut",
ty::ClosureKind::FnOnce => "AsyncFnOnce",
};

let mut err = struct_span_code_err!(
tcx.dcx(),
span,
E0802,
"the trait `{}` is not yet dyn-compatible",
async_fn_trait_name
);
// Note: this check is quite imprecise.
// Comparing the DefIds or similar would be better, but we can't store
// DefIds in `DynCompatibilityViolation`.
if async_fn_trait_name == trait_str {
err.span_label(span, format!("`{async_fn_trait_name}` is not yet dyn compatible"));
} else {
let trait_str = tcx.def_path_str(trait_def_id);
err.span_label(
span,
format!(
"`{trait_str}` inherits from `{async_fn_trait_name}` which is not yet dyn-compatible'"
),
);
}
attempt_dyn_to_impl_suggestion(tcx, hir_id, &mut err);
return err;
}

let trait_span = tcx.hir().get_if_local(trait_def_id).and_then(|node| match node {
hir::Node::Item(item) => Some(item.ident.span),
_ => None,
});

let mut err = struct_span_code_err!(
tcx.dcx(),
span,
Expand All @@ -441,24 +477,8 @@ pub fn report_dyn_incompatibility<'tcx>(
);
err.span_label(span, format!("`{trait_str}` cannot be made into an object"));

if let Some(hir_id) = hir_id
&& let hir::Node::Ty(ty) = tcx.hir_node(hir_id)
&& let hir::TyKind::TraitObject([trait_ref, ..], ..) = ty.kind
{
let mut hir_id = hir_id;
while let hir::Node::Ty(ty) = tcx.parent_hir_node(hir_id) {
hir_id = ty.hir_id;
}
if tcx.parent_hir_node(hir_id).fn_sig().is_some() {
// Do not suggest `impl Trait` when dealing with things like super-traits.
err.span_suggestion_verbose(
ty.span.until(trait_ref.span),
"consider using an opaque type instead",
"impl ",
Applicability::MaybeIncorrect,
);
}
}
attempt_dyn_to_impl_suggestion(tcx, hir_id, &mut err);

let mut reported_violations = FxIndexSet::default();
let mut multi_span = vec![];
let mut messages = vec![];
Expand Down Expand Up @@ -583,3 +603,30 @@ pub fn report_dyn_incompatibility<'tcx>(

err
}

fn attempt_dyn_to_impl_suggestion(tcx: TyCtxt<'_>, hir_id: Option<hir::HirId>, err: &mut Diag<'_>) {
let Some(hir_id) = hir_id else { return };
let hir::Node::Ty(ty) = tcx.hir_node(hir_id) else { return };
let hir::TyKind::TraitObject([trait_ref, ..], ..) = ty.kind else { return };

// Only suggest converting `dyn` to `impl` if we're in a function signature.
// Get the top-most parent element which is a type.
let parent_ty_hir_id = tcx
.hir()
.parent_iter(hir_id)
.take_while(|(_id, node)| matches!(node, hir::Node::Ty(_)))
.last()
.map(|(id, _node)| id)
.unwrap_or(hir_id);
if tcx.parent_hir_node(parent_ty_hir_id).fn_sig().is_none() {
// Do not suggest `impl Trait` when dealing with things like super-traits.
return;
}

err.span_suggestion_verbose(
ty.span.until(trait_ref.span),
"consider using an opaque type instead",
"impl ",
Applicability::MaybeIncorrect,
);
}
21 changes: 21 additions & 0 deletions compiler/rustc_trait_selection/src/traits/dyn_compatibility.rs
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,16 @@ pub fn hir_ty_lowering_dyn_compatibility_violations(
trait_def_id: DefId,
) -> Vec<DynCompatibilityViolation> {
debug_assert!(tcx.generics_of(trait_def_id).has_self);

// Check for `AsyncFn` traits first to avoid reporting various other
Copy link
Member

Choose a reason for hiding this comment

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

This isn't necessary, is it? I would suppose removing this doesn't actually affect anything.

// errors about the details of why these traits aren't dyn-compatible.
for supertrait in tcx.supertrait_def_ids(trait_def_id) {
if tcx.async_fn_trait_kind_from_def_id(supertrait).is_some() {
let fn_trait = tcx.item_name(supertrait);
return vec![DynCompatibilityViolation::AsyncFnTrait { fn_trait }];
}
}

tcx.supertrait_def_ids(trait_def_id)
.map(|def_id| predicates_reference_self(tcx, def_id, true))
.filter(|spans| !spans.is_empty())
Expand All @@ -53,6 +63,17 @@ fn dyn_compatibility_violations(
debug_assert!(tcx.generics_of(trait_def_id).has_self);
debug!("dyn_compatibility_violations: {:?}", trait_def_id);

// Check for `AsyncFn` traits first to avoid reporting various other
// errors about the details of why these traits aren't dyn-compatible.
for supertrait in tcx.supertrait_def_ids(trait_def_id) {
if tcx.async_fn_trait_kind_from_def_id(supertrait).is_some() {
let fn_trait = tcx.item_name(supertrait);
Copy link
Member

Choose a reason for hiding this comment

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

defer this call to item_name to the error reporting.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, lol, you can't. If you want, you could probably remove the PartialOrd implementation from DynCompatibilityViolation, and change the one(?) sort call in error reporting to stop doing that.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I did try that initially! Hence the comments about my annoyance that I couldn't put a DefId in the DynCompatibilityViolation. I don't think I want to extend this PR to removing the PartialOrd impl + sorting, as I anticipate that'll affect a number of other error outputs as well.

return core::slice::from_ref(
tcx.arena.alloc(DynCompatibilityViolation::AsyncFnTrait { fn_trait }),
);
}
}

tcx.arena.alloc_from_iter(
tcx.supertrait_def_ids(trait_def_id)
.flat_map(|def_id| dyn_compatibility_violations_for_trait(tcx, def_id)),
Expand Down
39 changes: 39 additions & 0 deletions tests/ui/async-await/async-closures/dyn.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
// Test the diagnostic output (error descriptions) resulting from `dyn AsyncFn{,Mut,Once}`.
//@ edition:2018
cramertj marked this conversation as resolved.
Show resolved Hide resolved

#![feature(async_closure)]

use core::ops::{AsyncFn, AsyncFnMut, AsyncFnOnce};

// --- Explicit `dyn` ---

fn takes_async_fn(_: &dyn AsyncFn()) {}
//~^ ERROR the trait `AsyncFn` is not yet dyn-compatible

fn takes_async_fn_mut(_: &mut dyn AsyncFnMut()) {}
//~^ ERROR the trait `AsyncFnMut` is not yet dyn-compatible

fn takes_async_fn_once(_: Box<dyn AsyncFnOnce()>) {}
//~^ ERROR the trait `AsyncFnOnce` is not yet dyn-compatible

// --- Non-explicit `dyn` ---

#[allow(bare_trait_objects)]
fn takes_async_fn_implicit_dyn(_: &AsyncFn()) {}
//~^ ERROR the trait `AsyncFn` is not yet dyn-compatible

#[allow(bare_trait_objects)]
fn takes_async_fn_mut_implicit_dyn(_: &mut AsyncFnMut()) {}
//~^ ERROR the trait `AsyncFnMut` is not yet dyn-compatible

#[allow(bare_trait_objects)]
fn takes_async_fn_once_implicit_dyn(_: Box<AsyncFnOnce()>) {}
//~^ ERROR the trait `AsyncFnOnce` is not yet dyn-compatible

// --- Supertrait ---

trait SubAsyncFn: AsyncFn() {}
fn takes_sub_async_fn(_: &dyn SubAsyncFn) {}
//~^ ERROR the trait `SubAsyncFn` cannot be made into an object

fn main() {}
83 changes: 83 additions & 0 deletions tests/ui/async-await/async-closures/dyn.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,83 @@
error[E0802]: the trait `AsyncFn` is not yet dyn-compatible
--> $DIR/dyn.rs:10:23
|
LL | fn takes_async_fn(_: &dyn AsyncFn()) {}
| ^^^^^^^^^^^^^ `AsyncFn` is not yet dyn compatible
|
help: consider using an opaque type instead
|
LL | fn takes_async_fn(_: &impl AsyncFn()) {}
| ~~~~

error[E0802]: the trait `AsyncFnMut` is not yet dyn-compatible
--> $DIR/dyn.rs:13:31
|
LL | fn takes_async_fn_mut(_: &mut dyn AsyncFnMut()) {}
| ^^^^^^^^^^^^^^^^ `AsyncFnMut` is not yet dyn compatible
|
help: consider using an opaque type instead
|
LL | fn takes_async_fn_mut(_: &mut impl AsyncFnMut()) {}
| ~~~~

error[E0802]: the trait `AsyncFnOnce` is not yet dyn-compatible
--> $DIR/dyn.rs:16:31
|
LL | fn takes_async_fn_once(_: Box<dyn AsyncFnOnce()>) {}
| ^^^^^^^^^^^^^^^^^ `AsyncFnOnce` is not yet dyn compatible
|
help: consider using an opaque type instead
|
LL | fn takes_async_fn_once(_: Box<impl AsyncFnOnce()>) {}
| ~~~~

error[E0802]: the trait `AsyncFn` is not yet dyn-compatible
--> $DIR/dyn.rs:22:36
|
LL | fn takes_async_fn_implicit_dyn(_: &AsyncFn()) {}
| ^^^^^^^^^ `AsyncFn` is not yet dyn compatible
|
help: consider using an opaque type instead
|
LL | fn takes_async_fn_implicit_dyn(_: &impl AsyncFn()) {}
| ++++

error[E0802]: the trait `AsyncFnMut` is not yet dyn-compatible
--> $DIR/dyn.rs:26:44
|
LL | fn takes_async_fn_mut_implicit_dyn(_: &mut AsyncFnMut()) {}
| ^^^^^^^^^^^^ `AsyncFnMut` is not yet dyn compatible
|
help: consider using an opaque type instead
|
LL | fn takes_async_fn_mut_implicit_dyn(_: &mut impl AsyncFnMut()) {}
| ++++

error[E0802]: the trait `AsyncFnOnce` is not yet dyn-compatible
--> $DIR/dyn.rs:30:44
|
LL | fn takes_async_fn_once_implicit_dyn(_: Box<AsyncFnOnce()>) {}
| ^^^^^^^^^^^^^ `AsyncFnOnce` is not yet dyn compatible
|
help: consider using an opaque type instead
|
LL | fn takes_async_fn_once_implicit_dyn(_: Box<impl AsyncFnOnce()>) {}
| ++++

error[E0038]: the trait `SubAsyncFn` cannot be made into an object
Copy link
Member

@compiler-errors compiler-errors Nov 21, 2024

Choose a reason for hiding this comment

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

We already have an error code for object safety, why add another one? Also, I am a bit unsatisfied with the inconsistency in the error messaging between the error message above and this one. If we want to have a separate error message, it should probably read somewhat parallel to the E0038 error message.

I don't believe users really need an in-depth explanation of the implementation details for why AsyncFn* is not dyn-compatible, and especially b/c it's something that I expect for us to fix in the medium-term future with my introduction of general AFIDT support.

Copy link
Member Author

Choose a reason for hiding this comment

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

I didn't use the language "cannot be made into an object" in the new error message because my understanding was that "object-safety" is the deprecated term, and "dyn-compatibility" is the new one. I also didn't change both to use "dyn-compatibility" because that would have affected significantly more code, though I can send a separate PR for that if it is desired.

Copy link
Member

Choose a reason for hiding this comment

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

I think just making this say the same thing as what E0038 says by default is fine, and if you'd like to reword the general error message in a separate PR it should be done separately.

Copy link
Member

@compiler-errors compiler-errors Nov 22, 2024

Choose a reason for hiding this comment

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

IMO the net effect of this PR should be to deduplicate the many different dyn compatibility violations, and to add a note explaining that AsyncFn* is not object safe.

Adding a totally different error message and a new error code seem unnecessary on top of that I feel?

--> $DIR/dyn.rs:36:27
|
LL | fn takes_sub_async_fn(_: &dyn SubAsyncFn) {}
| ^^^^^^^^^^^^^^ `SubAsyncFn` cannot be made into an object
|
= note: the trait cannot be made into an object because `async` function traits are not yet dyn-compatible
= note: for a trait to be "dyn-compatible" it needs to allow building a vtable to allow the call to be resolvable dynamically; for more information visit <https://doc.rust-lang.org/reference/items/traits.html#object-safety>
help: consider using an opaque type instead
|
LL | fn takes_sub_async_fn(_: &impl SubAsyncFn) {}
| ~~~~

error: aborting due to 7 previous errors

Some errors have detailed explanations: E0038, E0802.
For more information about an error, try `rustc --explain E0038`.
Loading
Loading