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.

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

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.

davidbarsky · 2025-04-09T18:59:14Z

This was blocked on rust-lang/chalk#826, which I think still requires a release.

snprajwal · 2025-05-05T10:47:30Z

Wondering if this also fixes #19439

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).

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.

Here, we lower the method's return type twice: once with RPITITs as assoc types, and once with the RPITITs as opaques, as if it was repeated on the impl.

Cycles can occur in malformed code (this is only a conjecture) or from Chalk bugs (for this I have a test).

Inside the method, we should insert an environment clause `RpititAssoc = Ty` so we know the value of the associated type. This matters mostly for defaulted trait methods, because for them without this calling the same method from the same trait will return the RPITIT and not the opaque type, but it also matters for impl methods, and will matter even more once we start reporting trait errors, as then RTN bounds could cause errors on the methods without this. Unfortunately that means we have to separate `trait_environment()` from `trait_environment_for_body()` (i.e. make the latter no longer a transparent wrapper around the former but a real query), because the RPITIT infra needs to call `trait_environment()` but `trait_environment_for_body()` needs to call it for the `AliasEq` clauses. This means another query, more memory usage etc.. But looks like it's unavoidable.

The binders of the recovery value weren't correct.

ChayimFriedman2 · 2025-05-15T22:58:06Z

Okay, I think this is ready for review.

I sincerely request: this PR is not small, and even less simple, and went through a rebase over a commit that changes a fundamental fact about Chalk - in which order we store generic parameters. I had many bugs, and I am sure there are more. I will really appreciate a throughout review, even more than one. If you feel like you know the ty area of rust-analyzer well enough and agree to do so, please review!

Okay, now to the boring facts:

This PR is a prerequisite to supporting Return Type Notation (and also correctly support Return Position Impl Trait In Trait in general). So it may be surprising that this PR, aimed to improve the correctness of our types, actually regresses unknown types on self (by a bit). This is due to bugs in Chalk that cause cycles, and it will be fixed by the switch to the new trait solver.

Speed and memory usage also regress (although not by much). The main reason is that there are now two queries for trait environment: trait_environment() and trait_environment_for_body() (previously the latter was a transparent wrapper over the former). Unfortunately, there isn't really something we can do to avoid that.

Having to do with how we rebase the impl params onto the trait params.

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 changed the title ~~Implement Return Position Impl Trait In Traits correctly~~ feat: Implement Return Position Impl Trait In Traits correctly Mar 19, 2025

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

ChayimFriedman2 force-pushed the rpitit-assoc branch 2 times, most recently from 0c7e786 to 98fb8c6 Compare May 15, 2025 12:29

ChayimFriedman2 force-pushed the rpitit-assoc branch from 98fb8c6 to 4966781 Compare May 15, 2025 22:46

ChayimFriedman2 marked this pull request as ready for review May 15, 2025 22:47

ChayimFriedman2 added 9 commits May 16, 2025 01:48

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

a609ba6

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

Comply with Chalk expectations

fdc5413

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.

Move RPITIT stuff to a new module

b7692b9

Handle RPITITs in defaulted trait methods

29ea2f0

Here, we lower the method's return type twice: once with RPITITs as assoc types, and once with the RPITITs as opaques, as if it was repeated on the impl.

Handle cycles in RPITIT computation

a8d89ae

Cycles can occur in malformed code (this is only a conjecture) or from Chalk bugs (for this I have a test).

Fix a bug

2b201a1

The binders of the recovery value weren't correct.

ChayimFriedman2 force-pushed the rpitit-assoc branch from 4966781 to 2b201a1 Compare May 15, 2025 22:48

Fix another bug

7544614

Having to do with how we rebase the impl params onto the trait params.

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

davidbarsky commented Apr 9, 2025

snprajwal commented May 5, 2025

ChayimFriedman2 commented May 15, 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

davidbarsky commented Apr 9, 2025

snprajwal commented May 5, 2025

ChayimFriedman2 commented May 15, 2025