Skip to content

Implement feature(more_qualified_paths) #19956

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

WaffleLapkin
Copy link
Member

Example:

struct T;
struct S {
    a: u32,
}

trait Trait {
    type Assoc;
}

impl Trait for T {
    type Assoc = S;
}

let <T as Trait>::Assoc { a } = <T as Trait>::Assoc { a: 0 };

@@ -1731,6 +1731,88 @@ impl<'db> InferenceContext<'db> {

self.resolve_variant_on_alias(ty, unresolved, mod_path)
}
TypeNs::TraitId(_trait_id) => {
Copy link
Member Author

Choose a reason for hiding this comment

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

This is mostly copied from the SelfType branch above and this feels very weird. For example the loop always does a single iteration...

Copy link
Contributor

Choose a reason for hiding this comment

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

The loop does not always do a single iteration, in the case Self::AssocType::EnumVariant it does two. However it seems you copied that incorrectly?

Copy link
Member Author

Choose a reason for hiding this comment

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

Rrright. The reason I copied it like that is that I couldn't find a way to get the type (E in <E as Trait>::Assoc::ES) for the ty variable on first iteration.

I found a way to get the type, but you'll probably know a better one ^^'

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm... It seems your logic should indeed be different from Self, and you don't need a loop. Since there is no sense to code like Trait::EnumVariant, you can have straight control flow: lower assoc types (lower_partly_resolved_path(), but it expects no unresolved segments so you may need to adapt it or ignore_last_segment()), then check if you have an addition segment, in which case try to resolve an enum variant from the type (ty.as_adt() then extract an enum out of it).

@WaffleLapkin WaffleLapkin force-pushed the more_qualified_paths branch from 38a3d65 to e2710df Compare June 9, 2025 19:44
@WaffleLapkin
Copy link
Member Author

What I can't so far figure out is how to make this work for enums. Example:

enum E {
    ES { a: u32 },
    ET(u32),
}

impl Trait for E {
    type Assoc = Self;
}

let <E>::ES { a } = (<E>::ES { a: 0 }) else { panic!() };
let <E>::ET(a) = <E>::ET(0) else { panic!() };
let <E as Trait>::Assoc::ES { a } = (<E as Trait>::Assoc::ES { a: 0 }) else { panic!() };
let <E as Trait>::Assoc::ET(a) = <E as Trait>::Assoc::ET(0) else { panic!() };

Out of all these r-a can only resolve ET(0) in expressions (makes sense, this is equivalent to assoc functions), but no other variants...

@ChayimFriedman2
Copy link
Contributor

The reason <E as Trait>::Assoc::ES does not work is because, as I said in a comment, you copied the loop incorrectly.

To resolve <E>::Variant, you need, at the top of the function to check if let Some(type_anchor) = path.type_anchor() (the type anchor is the E). If yes, you need to extract an enum out of it and match the variant. So something like:

        if let Some(type_anchor) = path.type_anchor() {
            let segments = path.segments();
            if segments.len() != 1 {
                return (self.err_ty(), None);
            }
            let ty = ctx.lower_ty(type_anchor);
            if let Some((AdtId::EnumId(id), _)) = ty.as_adt() {
                let enum_data = self.db.enum_variants(id);
                if let Some(variant) = enum_data.variant(segments.first().unwrap().name) {
                    // FIXME: Report error if there are generics on the variant.
                    return (ty, Some(variant.into()));
                }
            } else {
                // FIXME: Report an error.
                return (self.err_ty(), None);
            }
        }

Also, I'll be happy if you could deduplicate the code for handling Self and this.

@WaffleLapkin WaffleLapkin force-pushed the more_qualified_paths branch from e2710df to da7c176 Compare June 10, 2025 15:45
@ChayimFriedman2
Copy link
Contributor

Don't be afraid to ask for help should you need it!

@WaffleLapkin WaffleLapkin force-pushed the more_qualified_paths branch from fab04fe to 1fd0369 Compare June 10, 2025 19:59
@WaffleLapkin WaffleLapkin marked this pull request as ready for review June 10, 2025 20:00
@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 10, 2025
Specifically, this allows the following patterns and expressions which
were not allowed before:
```rust
let <T as Trait>::Assoc { a } = <T as Trait>::Assoc { a: 0 };

let (<E as Trait>::Assoc::ES { a } | <E as Trait>::Assoc::ET(a))
    = <E as Trait>::Assoc::ES { a: 0 };

let (<E>::ES { a } | <E>::ET(a)) = <E>::ES { a: 0 };
```
@WaffleLapkin WaffleLapkin force-pushed the more_qualified_paths branch from 1fd0369 to 64f9379 Compare June 10, 2025 20:03
@WaffleLapkin
Copy link
Member Author

@ChayimFriedman2 thanks for your advise! I think I was able to implement the feature in a way which doesn't look that bad ^^'

Do you know where I could add tests for this?

@ChayimFriedman2
Copy link
Contributor

Do you know where I could add tests for this?

crates/hir-ty/src/tests/traits.rs, you can look at existing tests there.

@Veykril
Copy link
Member

Veykril commented Jun 12, 2025

This probably needs some adjustment for pattern path completions as well. Not necessary for this PR, just a note, can give pointers if you want to add that to this PR though. Seems like we complete the Assoc part in the struct constructor, but not in the deconstructor.

@WaffleLapkin
Copy link
Member Author

@ChayimFriedman2 I'm currently a bit stuck. While adding tests I figured out that <Self>::Assoc { a: 0 } should work, provided it's in an impl of the trait which provides Assoc (this is specific to Self -- for any other type/syntax you need <T as Trait>).

I can check if the type ancor is Self and do something similar to what handles Self::. However, for <Self>::Assoc, path_ctx.resolve_path_in_type_ns() return None, so we early return from resolve_variant. Do you have an idea of what is a proper solution here?

@ChayimFriedman2
Copy link
Contributor

ChayimFriedman2 commented Jun 12, 2025

Use lower_ty_ext() instead of lower_ty() for the type anchor. It provides you, in addition to the Ty, its TypeNs resolution. Check if it is SelfType and extract the ImplId. Then you can extract the trait of that impl with db.impl_trait(), and construct a projection to the associated type in the trait (basically, take the trait substitution from the TraitRef in the impl_trait(), add to it the associated type substitution lowered from the path, that'll be the substitution for the projection). Don't forget to normalize the projection if possible.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants