Add shorter and more direct error for dyn AsyncFn #133267

cramertj · 2024-11-20T22:31:25Z

rustbot · 2024-11-20T22:31:34Z

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

rustbot · 2024-11-20T22:31:36Z

HIR ty lowering was modified

cc @fmease

Some changes occurred in diagnostic error codes

cc @GuillaumeGomez

compiler/rustc_middle/src/traits/mod.rs

compiler/rustc_error_codes/src/error_codes/E0802.md

tests/ui/async-await/async-closures/dyn.rs

compiler/rustc_type_ir/src/interner.rs

compiler-errors · 2024-11-21T00:31:00Z

compiler/rustc_trait_selection/src/traits/dyn_compatibility.rs

 ) -> Vec<DynCompatibilityViolation> {
    debug_assert!(tcx.generics_of(trait_def_id).has_self);
+
+    // Check for `AsyncFn` traits first to avoid reporting various other


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

compiler-errors · 2024-11-21T00:31:22Z

compiler/rustc_trait_selection/src/traits/dyn_compatibility.rs

+    // errors about the details of why these traits aren't object-safe.
+    for supertrait in tcx.supertrait_def_ids(trait_def_id) {
+        if tcx.trait_is_async_fn(supertrait) {
+            let fn_trait = tcx.item_name(supertrait);


defer this call to item_name to the error reporting.

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.

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.

compiler-errors · 2024-11-21T00:33:49Z

compiler/rustc_trait_selection/src/error_reporting/traits/mod.rs

+    let hir::Node::Ty(ty) = tcx.hir_node(hir_id) else { return };
+    let hir::TyKind::TraitObject([trait_ref, ..], ..) = ty.kind else { return };
+    let mut hir_id = hir_id;
+    while let hir::Node::Ty(ty) = tcx.parent_hir_node(hir_id) {


I think you can use parent_iter and take_while rather than redoing it here.

Something like this?

- 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_none() { + let parent_ty_hir_id = tcx + .parent_iter(hir_id) + .take_while(|_id, node| if let hir::Node::Ty(_) = node { true } else { false }) + .last() + .unwrap_or(hir_id); + if tcx.parent_hir_node(parent_ty_hir_id).fn_sig().is_none() {

IMO that's harder to read. I also don't have access to parent_iter since it's a method on Map, though I'm probably just missing how to access that value.

You can access Map through tcx.hir().

Also, I'd probably write it a bit shorter like:

if tcx .hir() .parent_iter(hir_id) .take_while(|_, node| matches!(node, hir::Node::Ty(_)))

Or actually, maybe just using skip_while to get the next node.

parent_iter should always be preferred over repeated calls of parent_hir_node/parent_hir_id for performance reasons (see also docs of https://doc.rust-lang.org/nightly/nightly-rustc/rustc_middle/ty/context/struct.TyCtxt.html#method.parent_hir_id)

Or actually, maybe just using skip_while to get the next node.

nit: I don't quite see how skip_while would work here since the goal is to get the last element that matches a predicate. I think take_while + last seems the most straightforward.

Yeah, and we're immediately calling parent_hir_node on it. So what we really want is the first node that isn't a Node::Ty. But whatever, not worth fixing here.

compiler/rustc_trait_selection/src/error_reporting/traits/mod.rs

compiler/rustc_middle/src/ty/mod.rs

rustbot · 2024-11-21T00:57:57Z

Some changes occurred to the CTFE / Miri interpreter

cc @rust-lang/miri

Some changes occurred to the CTFE machinery

cc @rust-lang/wg-const-eval

compiler/rustc_trait_selection/src/traits/dyn_compatibility.rs

compiler/rustc_const_eval/src/interpret/call.rs

Fix rust-lang#132713

compiler-errors · 2024-11-21T23:27:39Z

@bors r+

bors · 2024-11-21T23:27:42Z

📌 Commit 306702e has been approved by compiler-errors

It is now in the queue for this repository.

compiler-errors · 2024-11-21T23:51:59Z

Sorry, but I actually have some thoughts here after some experimentation on a local branch

@bors r-

compiler-errors

Sorry for the churn, but actually, I looked into this a bit further on my own local checkout since I wanted to make a quick follow-up, and I think this still needs some tweaks:

You're not actually using the fn_trait item in the DynCompatibilityViolation anywhere? Not sure if it was intentional, but I also think that's fine -- I don't think it helps with the messaging anyways (and we can always elaborate the trait def id if we want to find it later, rather than storing it into the DynCompatibilityViolation).
I'm gonna re-raise the question that I asked that didn't get an answer, which is why this needs a separate error code? I think that error code is kinda bespoke and unnecessary when we have a general error code E0038 for dyn compatibility violations, especially b/c we're already noting that "the trait cannot be made into an object because async function traits are not yet dyn-compatible".
Also, if you want to be cool and remove the sorting and PartialOrd/Ord implementation from DynCompatibilityViolation, it's actually totally unnecessary for sorting. If you look at the one callsite in the compiler, it sorts the list and then maps that list then sorts it again 😆. If you don't want to fix that, then that's fine too 👍

compiler-errors · 2024-11-21T23:53:19Z

tests/ui/async-await/async-closures/dyn.stderr

+LL | fn takes_async_fn_once_implicit_dyn(_: Box<impl AsyncFnOnce()>) {}
+   |                                            ++++
+
+error[E0038]: the trait `SubAsyncFn` cannot be made into an object


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.

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.

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.

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?

compiler-errors · 2024-11-21T23:53:43Z

compiler/rustc_middle/src/traits/mod.rs

+        // 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,


We don't actually use this anywhere?

This CL makes a number of small changes to dyn compatibility errors: - "object safety" has been renamed to "dyn-compatibility" throughout - "Convert to enum" suggestions are no longer generated when there exists a type-generic impl of the trait or an impl for `dyn OtherTrait` - Several error messages are reorganized for user readability Additionally, the dyn compatibility error creation code has been split out into functions. cc rust-lang#132713 cc rust-lang#133267

cramertj · 2024-11-26T18:16:01Z

Closing this in favor of some more general fixes in #133372

This CL makes a number of small changes to dyn compatibility errors: - "object safety" has been renamed to "dyn-compatibility" throughout - "Convert to enum" suggestions are no longer generated when there exists a type-generic impl of the trait or an impl for `dyn OtherTrait` - Several error messages are reorganized for user readability Additionally, the dyn compatibility error creation code has been split out into functions. cc rust-lang#132713 cc rust-lang#133267

…=fmease Refactor dyn-compatibility error and suggestions This CL makes a number of small changes to dyn compatibility errors: - "object safety" has been renamed to "dyn-compatibility" throughout - "Convert to enum" suggestions are no longer generated when there exists a type-generic impl of the trait or an impl for `dyn OtherTrait` - Several error messages are reorganized for user readability Additionally, the dyn compatibility error creation code has been split out into functions. cc rust-lang#132713 cc rust-lang#133267 r? `@compiler-errors`

rustbot assigned wesleywiser Nov 20, 2024

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 Nov 20, 2024

cramertj added the A-async-closures `async || {}` label Nov 20, 2024

fmease reviewed Nov 20, 2024

View reviewed changes

compiler/rustc_middle/src/traits/mod.rs Outdated Show resolved Hide resolved

GuillaumeGomez reviewed Nov 20, 2024

View reviewed changes

compiler/rustc_error_codes/src/error_codes/E0802.md Outdated Show resolved Hide resolved

cramertj force-pushed the async-fn-dyn-err branch from 1e17c14 to 55d16b9 Compare November 20, 2024 22:51

This comment has been minimized.

Sign in to view

cramertj commented Nov 21, 2024

View reviewed changes

tests/ui/async-await/async-closures/dyn.rs Show resolved Hide resolved

compiler-errors reviewed Nov 21, 2024

View reviewed changes

compiler-errors assigned compiler-errors and unassigned wesleywiser Nov 21, 2024

rustbot added the WG-trait-system-refactor The Rustc Trait System Refactor Initiative (-Znext-solver) label Nov 21, 2024

This comment has been minimized.

Sign in to view

fmease reviewed Nov 21, 2024

View reviewed changes

compiler/rustc_trait_selection/src/traits/dyn_compatibility.rs Outdated Show resolved Hide resolved

compiler/rustc_trait_selection/src/traits/dyn_compatibility.rs Outdated Show resolved Hide resolved

compiler-errors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 21, 2024

RalfJung reviewed Nov 21, 2024

View reviewed changes

compiler/rustc_const_eval/src/interpret/call.rs Outdated Show resolved Hide resolved

cramertj force-pushed the async-fn-dyn-err branch 2 times, most recently from 0718a49 to b73794d Compare November 21, 2024 17:57

This comment has been minimized.

Sign in to view

cramertj force-pushed the async-fn-dyn-err branch from b73794d to 166484c Compare November 21, 2024 19:03

Add shorter and more direct error for dyn AsyncFn

306702e

Fix rust-lang#132713

cramertj force-pushed the async-fn-dyn-err branch from 166484c to 306702e Compare November 21, 2024 19:21

bors removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Nov 21, 2024

bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Nov 21, 2024

bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Nov 21, 2024

compiler-errors requested changes Nov 21, 2024

View reviewed changes

cramertj mentioned this pull request Nov 23, 2024

Refactor dyn-compatibility error and suggestions #133372

Merged

cramertj closed this Nov 26, 2024

Uh oh!

Add shorter and more direct error for dyn AsyncFn #133267

Add shorter and more direct error for dyn AsyncFn #133267

Uh oh!

Conversation

cramertj commented Nov 20, 2024

Uh oh!

rustbot commented Nov 20, 2024

Uh oh!

rustbot commented Nov 20, 2024

Uh oh!

Uh oh!

Uh oh!

This comment has been minimized.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

compiler-errors Nov 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

rustbot commented Nov 21, 2024

Uh oh!

This comment has been minimized.

Uh oh!

Uh oh!

Uh oh!

This comment has been minimized.

compiler-errors commented Nov 21, 2024

Uh oh!

bors commented Nov 21, 2024

Uh oh!

compiler-errors commented Nov 21, 2024

Uh oh!

compiler-errors left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

compiler-errors Nov 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

compiler-errors Nov 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cramertj commented Nov 26, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

compiler-errors Nov 21, 2024 •

edited

Loading

compiler-errors left a comment •

edited

Loading

compiler-errors Nov 21, 2024 •

edited

Loading

compiler-errors Nov 22, 2024 •

edited

Loading