feat: Implement Return Position Impl Trait In Traits correctly #19394

ChayimFriedman2 · 2025-03-18T18:51:28Z

This is a requirement to landing Return Type Notation.

Phew! That is not a lot of code, but it was pretty complicated. I'm still not 100% sure I did everything correctly, so I will appreciate a thorough review.

They don't exist in hir-def, only hir-ty. The idea is that the trait/impl datum query will compute and intern them, then other queries can treat them as (partially) normal associated types. (Lowering RPITIT to synthesized associated types, like rustc does, is required to properly support Return Type Notation).

Veykril · 2025-03-19T06:26:39Z

crates/base-db/src/lib.rs

+    ($id:ident, $loc:ident) => {
+        #[salsa::interned(no_debug, no_lifetime)]
+        pub struct $id {
+            #[return_ref]


Next salsa will hopefully flip this to be the default mode (once someone implements it salsa-rs/salsa#719)

crates/hir-ty/src/tests/traits.rs

crates/hir-ty/src/chalk_db.rs

Instead of lowering them as opaque type, in the trait they should get lowered into a synthesized associated type. Opaques "mostly worked" here, but they break when trying to implement Return Type Notation, which is essentially a way to refer in code to this synthesized associated type. So we need to do the correct thing.

It broke due to the RPITIT changes. And also make it more robust, by relying on semantic instead of textual match.

The previous setup led to a cycle in scenario like the following: ```rust trait Trait { type Assoc; fn foo() -> impl Sized; } impl Trait for () { type Assoc = (); fn foo() -> Self::Assoc; } ``` Because we asked Chalk to normalize the return type of the method, and for that it asked us the datum of the impl, which again causes us to evaluate the RPITITs. Instead, we only put the associated type ID in `impl_datum()`, and we compute it on `associated_ty_value()`. This still causes a cycle because Chalk needlessly evaluates all associated types for an impl, but at least it'll work for the new trait solver, and compiler-errors will try to fix that for Chalk too.

Chalk apparently requires associated type values to contain both the impl and the associated ty binders, completely unlike the docs (which say they should only contain those of the associated type). So fix that.

davidbarsky

not the most thorough review, but i focused mostly on the docs and explanations as to how this works

(note: i don't really understand show this works)

davidbarsky · 2025-03-25T00:07:54Z

crates/hir-ty/src/chalk_db.rs

+            // In this situation, we don't know even that the trait and impl generics match, therefore
+            // the only binders we can give to comply with the trait's binders are the trait's binders.
+            // However, for impl associated types chalk wants only their own generics, excluding
+            // those of the impl (unlike in traits), therefore we filter them here.
+            // Completely unlike the docs, Chalk requires both the impl generics and the associated type
+            // generics in the binder.


Slight rewrite for clarity and emphasis of some key points. Note that I don't really understand much in hir-ty, so my rewrite might not be correct, but one of my goals in rewriting this was to better understand hir-ty!

Suggested change

// In this situation, we don't know even that the trait and impl generics match, therefore

// the only binders we can give to comply with the trait's binders are the trait's binders.

// However, for impl associated types chalk wants only their own generics, excluding

// those of the impl (unlike in traits), therefore we filter them here.

// Completely unlike the docs, Chalk requires both the impl generics and the associated type

// generics in the binder.

// In this situation, we don't know that the trait and impl generics even match. Therefore,

// the only binders we can offer to comply with the trait's binders are the trait's binders.

// This is tautological.

//

// However, for impl-associated types, Chalk wants only the type's *own* generics, excluding

// those of the `impl`. This is unlike the behavior in traits, making it necessary to filter

// out all other generics.

//

// Note that unlike Chalk's documentation, Chalk, in reality, requires both the

// impl generics and the associated type generics to be in the binder.

Two clarifying questions:

What are "impl associated types"?

What does Chalk's documentation actually say that you're doing differently here?

davidbarsky · 2025-03-25T00:10:06Z

crates/hir-ty/src/chalk_ext.rs

+    match from_assoc_type_id(db, id) {
+        AnyTraitAssocType::Normal(type_alias) => match type_alias.lookup(db.upcast()).container {
+            ItemContainerId::TraitId(trait_id) => trait_id,
+            _ => panic!("`AssocTypeId` without parent trait"),


Suggested change

_ => panic!("`AssocTypeId` without parent trait"),

_ => panic!("`AssocTypeId` without parent trait; this is a bug."),

(The "this is a bug" suffix is a thing we do in tracing; I've found it makes people more proactive in reporting bugs)

davidbarsky · 2025-03-25T00:12:31Z

crates/hir-ty/src/chalk_ext.rs

+            // FIXME: This isn't entirely correct, the generics of the RPITIT assoc type may differ from its method
+            // wrt. lifetimes, but we don't handle that currently. See https://rustc-dev-guide.rust-lang.org/return-position-impl-trait-in-trait.html.


Suggested change

// FIXME: This isn't entirely correct, the generics of the RPITIT assoc type may differ from its method

// wrt. lifetimes, but we don't handle that currently. See https://rustc-dev-guide.rust-lang.org/return-position-impl-trait-in-trait.html.

// FIXME: This isn't entirely correct, the generics of the RPITIT assoc type may differ from its method

// with regard to lifetimes, but rust-analyzer doesn't really handle lifetimes in the first place, so whatever.

// For details how on how to handle this properly (e.g., as rustc does), see

// https://rustc-dev-guide.rust-lang.org/return-position-impl-trait-in-trait.html.

davidbarsky · 2025-03-25T00:19:28Z

crates/hir-ty/src/display.rs

+                        AnyTraitAssocType::Normal(it) => it,
+                        AnyTraitAssocType::Rpitit(_) => {
+                            unreachable!(
+                                "Rust does not currently have a way to specify alias equation on RPITIT"


Not sure I'd... use "equation". It's correct, I guess, but might be a bit too impose if a user sees this error message (can they see this error message)?

Suggested change

"Rust does not currently have a way to specify alias equation on RPITIT"

"Rust does not currently have a way to specify aliases on RPITIT"

davidbarsky · 2025-03-25T00:19:43Z

crates/hir-ty/src/display.rs

+                    AnyTraitAssocType::Normal(it) => it,
+                    AnyTraitAssocType::Rpitit(_) => {
+                        unreachable!(
+                            "Rust does not currently have a way to specify alias equation on RPITIT"


Same nit about "equation" as above.

davidbarsky · 2025-03-25T00:47:36Z

crates/hir-ty/src/lower.rs

+    ///     fn foo(&self) -> Option<impl Debug>;
+    /// }
+    /// ```
+    /// The equation will tell us that the hidden associated type has value `Option<impl Debug>` (note: this


Suggested change

/// The equation will tell us that the hidden associated type has value `Option<impl Debug>` (note: this

/// The above process will tell us that the hidden associated type is `Option<impl Debug>` (note: this

davidbarsky · 2025-03-25T00:50:36Z

crates/hir-ty/src/rpitit.rs

+/// An associated type synthesized from a Return Position Impl Trait In Trait
+/// of the trait (not the impls).


Suggested change

/// An associated type synthesized from a Return Position Impl Trait In Trait

/// of the trait (not the impls).

/// An associated type synthesized from a Return Position Impl Trait In Trait.

/// of the *trait*.

///

/// This is not come from the the impls; see [`RpititImplAssocTy`] instead.

davidbarsky · 2025-03-25T00:51:22Z

crates/hir-ty/src/rpitit.rs

+/// An associated type synthesized from a Return Position Impl Trait In Trait
+/// of the impl (not the trait).


Suggested change

/// An associated type synthesized from a Return Position Impl Trait In Trait

/// of the impl (not the trait).

/// An associated type synthesized from a Return Position Impl Trait In Trait

/// of the *impl*.

///

/// This is not come from the the trait; see [`RpititTraitAssocTy`] instead.

davidbarsky · 2025-03-25T00:51:51Z

crates/hir-ty/src/rpitit.rs

+    /// The generics are the generics of the method (with some modifications that we
+    /// don't currently implement, see https://rustc-dev-guide.rust-lang.org/return-position-impl-trait-in-trait.html).


The things we don't implement is the lifetime lowering, right?

davidbarsky · 2025-03-25T00:52:32Z

crates/hir-ty/src/rpitit.rs

+    impl_id: ImplId,
+    trait_method_id: FunctionId,
+) -> Box<[Arc<AssociatedTyValue>]> {
+    tracing::error!("impl_method_rpitit_values()");


I think you want this?

Suggested change

tracing::error!("impl_method_rpitit_values()");

tracing::span!("impl_method_rpitit_values", Level::INFO);

That was a debugging aid, I pushed the PR in a not-ready-for-review state because @compiler-errors wanted to debug the Chalk issue.

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 18, 2025

ChayimFriedman2 marked this pull request as draft March 18, 2025 20:13

Veykril reviewed Mar 19, 2025

View reviewed changes

flodiebold approved these changes Mar 19, 2025

View reviewed changes

crates/hir-ty/src/tests/traits.rs Outdated Show resolved Hide resolved

crates/hir-ty/src/chalk_db.rs Outdated Show resolved Hide resolved

crates/hir-ty/src/chalk_db.rs Outdated Show resolved Hide resolved

ChayimFriedman2 added 2 commits March 19, 2025 23:49

Fix resugaring of fn() -> impl Future to async fn in completions

945a7c0

It broke due to the RPITIT changes. And also make it more robust, by relying on semantic instead of textual match.

ChayimFriedman2 changed the title ~~Implement Return Position Impl Trait In Traits correctly~~ feat: Implement Return Position Impl Trait In Traits correctly Mar 19, 2025

ChayimFriedman2 added 5 commits March 20, 2025 01:32

Comply with Chalk expectations

ee2d5fa

Chalk apparently requires associated type values to contain both the impl and the associated ty binders, completely unlike the docs (which say they should only contain those of the associated type). So fix that.

half work: upgrade chalk

1c199f6

Move RPITIT stuff to a new module

50ef086

half work

1922299

ChayimFriedman2 force-pushed the rpitit-assoc branch from b86adf9 to 1922299 Compare March 20, 2025 17:58

davidbarsky reviewed Mar 25, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Implement Return Position Impl Trait In Traits correctly #19394

feat: Implement Return Position Impl Trait In Traits correctly #19394

ChayimFriedman2 commented Mar 18, 2025

Veykril Mar 19, 2025

davidbarsky left a comment

davidbarsky Mar 25, 2025

davidbarsky Mar 25, 2025

davidbarsky Mar 25, 2025

davidbarsky Mar 25, 2025

davidbarsky Mar 25, 2025

davidbarsky Mar 25, 2025

davidbarsky Mar 25, 2025

davidbarsky Mar 25, 2025

davidbarsky Mar 25, 2025

davidbarsky Mar 25, 2025

ChayimFriedman2 Mar 25, 2025

-            // In this situation, we don't know even that the trait and impl generics match, therefore
-            // the only binders we can give to comply with the trait's binders are the trait's binders.
-            // However, for impl associated types chalk wants only their own generics, excluding
-            // those of the impl (unlike in traits), therefore we filter them here.
-            // Completely unlike the docs, Chalk requires both the impl generics and the associated type
-            // generics in the binder.
+            // In this situation, we don't know that the trait and impl generics even match. Therefore,
+            // the only binders we can offer to comply with the trait's binders are the trait's binders.
+            // This is tautological.
+            //
+            // However, for impl-associated types, Chalk wants only the type's *own* generics, excluding
+            // those of the `impl`. This is unlike the behavior in traits, making it necessary to filter
+            // out all other generics.
+            //
+            // Note that unlike Chalk's documentation, Chalk, in reality, requires both the
+            // impl generics and the associated type generics to be in the binder.

	_ => panic!("`AssocTypeId` without parent trait"),
	_ => panic!("`AssocTypeId` without parent trait; this is a bug."),

		// FIXME: This isn't entirely correct, the generics of the RPITIT assoc type may differ from its method
		// wrt. lifetimes, but we don't handle that currently. See https://rustc-dev-guide.rust-lang.org/return-position-impl-trait-in-trait.html.

	"Rust does not currently have a way to specify alias equation on RPITIT"
	"Rust does not currently have a way to specify aliases on RPITIT"

	/// The equation will tell us that the hidden associated type has value `Option<impl Debug>` (note: this
	/// The above process will tell us that the hidden associated type is `Option<impl Debug>` (note: this

		/// An associated type synthesized from a Return Position Impl Trait In Trait
		/// of the trait (not the impls).

-/// An associated type synthesized from a Return Position Impl Trait In Trait
-/// of the trait (not the impls).
+/// An associated type synthesized from a Return Position Impl Trait In Trait.
+/// of the *trait*.
+///
+/// This is not come from the the impls; see [`RpititImplAssocTy`] instead.

		/// An associated type synthesized from a Return Position Impl Trait In Trait
		/// of the impl (not the trait).

		/// The generics are the generics of the method (with some modifications that we
		/// don't currently implement, see https://rustc-dev-guide.rust-lang.org/return-position-impl-trait-in-trait.html).

	tracing::error!("impl_method_rpitit_values()");
	tracing::span!("impl_method_rpitit_values", Level::INFO);

feat: Implement Return Position Impl Trait In Traits correctly #19394

Are you sure you want to change the base?

feat: Implement Return Position Impl Trait In Traits correctly #19394

Conversation

ChayimFriedman2 commented Mar 18, 2025

Choose a reason for hiding this comment

davidbarsky left a 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

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

Choose a reason for hiding this comment