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

fmease · 2024-11-20T22:42:23Z

compiler/rustc_middle/src/traits/mod.rs

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


Suggested change

"`async` function traits are not yet `dyn`-compatible".into()

"`async` function traits are not yet dyn-compatible".into()

What is the reason for preferring backticks for async and other keywords, but not for dyn? Should I also remove the backticks for async?

There isn't a clear-cut general answer I'd say. As for dyn-compatible specifically, I'd like to think of it as a well-established term — including its spelling — as per rust-lang/lang-team#286 and all PRs under #130852. It would only break consistency to use dyn-compatible now.

Hot-ish take, feel free to ignore

(Personally speaking, I think I'm leaning towards fully avoiding the use of backticks in running text / prose for the sake of legibility unless it surrounds literal source code. Mixing "code" and "prose" to express certain concepts feels rather ad hoc and awkward to me — like, I do that, too, when writing quick GH comments for example but for "official text" I think that's a different story. Note that I've probably broken that "rule" many times myself and I'm not suggesting you should follow it barring dyn-compatibility ^^ as it's quite unrealistic since several terms used in diagnostics are ad hoc & have never been seen before and it's quite convenient to intermix code and text)

rust-lang/lang-team#286 also does not hyphenate. That proposal uses "dyn compatibility" instead of "dyn-compatibility", so to be consistent I suppose I'll use "dyn compatible" rather than "dyn-compatible".

I'll use "dyn compatible" rather than "dyn-compatible".

Ye, that's fine. CC #126554 (comment)

Oh geeze-- yeah, there are lots of uses of "dyn-compatible" throughout the codebase. It would be nice to have a standard here. "dyn compatible" + "dyn-compatibility"? I've added a commit that does a broad search+replace, but we can drop that if you don't think it's justified here.

I think it should be done elsewhere

Sorry, by elsewhere I mean on a different PR. And then probably assign that to @fmease if they want it. This PR now touches 150 files 😅

I'm happy to leave it for another PR. In that case, I'd prefer to keep this PR using the existing terminology that is present throughout the codebase ("dyn-compatible") and a different PR can sort out whether that choice should be revised. I prefer this approach because it keeps the terminology more uniform in the meantime.

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/rustc_trait_selection/src/traits/dyn_compatibility.rs

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.

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

compiler-errors · 2024-11-21T00:34:07Z

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

+    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() {


What does this actually suppress?

Also, I just re-read the whole diff. I see that you just moved this code to generalize the suggestion, right?

If so, then if you don't want to deal w/ the nits in this specific area, I guess that's fine.

I'm happy to make drive-by cleanups-- thanks for the suggestions and for helping me learn!

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

RalfJung · 2024-11-21T06:41:13Z

compiler/rustc_const_eval/src/interpret/call.rs

@@ -614,7 +614,7 @@ impl<'tcx, M: Machine<'tcx>> InterpCx<'tcx, M> {
            // codegen'd / interpreted as virtual calls through the vtable.
            ty::InstanceKind::Virtual(def_id, idx) => {
                let mut args = args.to_vec();
-                // We have to implement all "dyn-compatible receivers". So we have to go search for a
+                // We have to implement all "dyn compatible receivers". So we have to go search for a


Doesn't English grammar say we need a - since this is a compound term used as an adjective? It's like "self-referential data", for instance.

That was my intuition as well-- the initial version of this PR used the hyphenated compound adjective. @fmease linked to #126554 (comment) which seems to indicate a different preference.

My personal preference is for uniform use of "dyn-compatible" and "dyn-compatibility". This seems like something we should have a formal decision (T-lang approval?) around so that different decisions aren't being made at the level of individual contributions.

Travis wrote there:

one would probably say that "a trait is dyn compatible" (but one might "have a dyn-compatible trait")

This is what makes most sense to me. A compound term in English is used with hyphen in adjective form but without hyphen elsewhere. That's how English works, it would be non-grammatical and confusing to insist on the same spelling everywhere.

It seems odd to me that t-lang would decide on a grammar question (the grammar of English, not of Rust ;), but 🤷

Fix rust-lang#132713

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 F-async_closure `#![feature(async_closure)]` label Nov 20, 2024

fmease reviewed Nov 20, 2024

View reviewed changes

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

cramertj force-pushed the async-fn-dyn-err branch from 0fc289a to 0718a49 Compare November 21, 2024 17:56

Add shorter and more direct error for dyn AsyncFn

b73794d

Fix rust-lang#132713

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add shorter and more direct error for dyn AsyncFn #133267

Add shorter and more direct error for dyn AsyncFn #133267

cramertj commented Nov 20, 2024

rustbot commented Nov 20, 2024

rustbot commented Nov 20, 2024

fmease Nov 20, 2024

cramertj Nov 20, 2024

fmease Nov 20, 2024 •

edited

Loading

fmease Nov 20, 2024 •

edited

Loading

cramertj Nov 20, 2024

fmease Nov 21, 2024

cramertj Nov 21, 2024

compiler-errors Nov 21, 2024

compiler-errors Nov 21, 2024

cramertj Nov 21, 2024 •

edited

Loading

This comment has been minimized.

compiler-errors Nov 21, 2024

compiler-errors Nov 21, 2024

compiler-errors Nov 21, 2024

compiler-errors Nov 21, 2024

cramertj Nov 21, 2024

rustbot commented Nov 21, 2024

This comment has been minimized.

RalfJung Nov 21, 2024 •

edited

Loading

cramertj Nov 21, 2024

RalfJung Nov 21, 2024 •

edited

Loading

	"`async` function traits are not yet `dyn`-compatible".into()
	"`async` function traits are not yet dyn-compatible".into()

Add shorter and more direct error for dyn AsyncFn #133267

Are you sure you want to change the base?

Add shorter and more direct error for dyn AsyncFn #133267

Conversation

cramertj commented Nov 20, 2024

rustbot commented Nov 20, 2024

rustbot commented Nov 20, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fmease Nov 20, 2024 • edited Loading

Choose a reason for hiding this comment

fmease Nov 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cramertj Nov 21, 2024 • edited Loading

Choose a reason for hiding this comment

This comment has been minimized.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rustbot commented Nov 21, 2024

This comment has been minimized.

RalfJung Nov 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RalfJung Nov 21, 2024 • edited Loading

Choose a reason for hiding this comment

fmease Nov 20, 2024 •

edited

Loading

fmease Nov 20, 2024 •

edited

Loading

cramertj Nov 21, 2024 •

edited

Loading

RalfJung Nov 21, 2024 •

edited

Loading

RalfJung Nov 21, 2024 •

edited

Loading