Skip to content
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

Make Self clause explicit in trait item declarations #514

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

Nadrieril
Copy link
Member

For #180 I want trait methods to be completely non-magical functions. #512 made huge progress towards this, but I had forgotten about Self clauses.

Within a trait declaration (including its associated items), we can use TraitRefKind::SelfId to refer to the ambiant Self: ThisTrait clause. This is used e.g. to recursively call other trait methods, or access the value of the Self::Type associated types. Trait implementations don't need this, because they always know the correct method/type to use directly.

In order for methods to be normal functions, I needed to remove uses of this implicit clause. This PR instead makes the clause explicit in the FunDecl that corresponds to each method declaration. Note that this does not affect method signatures: the clauses needed to call a method don't change. The special clause is passed when the method is bound in the TraitDecl (which we can express thanks to #512).

@Nadrieril Nadrieril force-pushed the explicit-self-in-methods branch from 19dc60a to ea90198 Compare January 3, 2025 14:56
@sonmarcho
Copy link
Member

I think this goes in the right direction, but there are some details I don't fully get. In particular I'm not sure I understand: "The special clause is passed when the method is bound in the TraitDecl": could you give an example?

@Nadrieril
Copy link
Member Author

Nadrieril commented Jan 6, 2025

Here's an example inspired from the tests diff:

trait test_crate::Trait<Self, B>
{
    parent_clause0 : [@TraitClause0]: core::marker::Sized<B>
    parent_clause1 : [@TraitClause1]: core::marker::Sized<Self::Item>
    type Item
    fn method<C, [@TraitClause0]: core::marker::Sized<C>> = test_crate::Trait::method<Self, B, C>[Self, @TraitClause0_0]
    //                                                                                            ^^^^ `TraitRefKind::SelfId`
    //                                                                                ^^^^ normal type param named `Self`
    // before this PR:
    // fn method<C, [@TraitClause0]: core::marker::Sized<C>> = test_crate::Trait::method<Self, B, C>[@TraitClause0_0]
}

fn test_crate::Trait::method<Self, B, C>() -> @TraitClause0::Item // was `-> Self::Item` before
where
    [@TraitClause0]: test_crate::Trait<Self, B>, // added by this PR
    [@TraitClause1]: core::marker::Sized<C>,

As you can see, since #512 in a trait declaration/implementation a method is not just a FunDeclId it's a map from the method arguments to the arguments to pass to the function item. What this PR does is that the function item for a method declaration now has an extra clause argument. The implicit Self clause is no longer available in function items, all references to Self now use the new clause. (Note that the implicit Self clause was already not available in trait impls or trait method implementation items).

@sonmarcho
Copy link
Member

Ok, thanks! So we agree that now all trait method declarations are mutually recursive with the trait itself? Doesn't that cause issues on the Aeneas' side?

@Nadrieril
Copy link
Member Author

So we agree that now all trait method declarations are mutually recursive with the trait itself?

Yep

Doesn't that cause issues on the Aeneas' side?

I don't know yet, this requires more binder-related fixes on the aeneas side first. If it does cause issues I hope that'll be only for provided methods (which I'll be fixing next), we'll see.

@Nadrieril Nadrieril force-pushed the explicit-self-in-methods branch 2 times, most recently from 928747f to 7f8bece Compare January 7, 2025 11:19
@Nadrieril Nadrieril changed the title Make Self clause explicit in trait method declarations Make Self clause explicit in trait item declarations Jan 7, 2025
@Nadrieril Nadrieril force-pushed the explicit-self-in-methods branch from dcde15c to 0c3cbb3 Compare January 7, 2025 15:02
@sonmarcho
Copy link
Member

sonmarcho commented Jan 8, 2025

But we agree that implementations of trait methods don't change (in particular their signature - I don't fully understand this sentence: "Note that this does not affect method signatures: the clauses needed to call a method don't change.")?

@Nadrieril
Copy link
Member Author

Nadrieril commented Jan 8, 2025

But we agree that implementations of trait methods don't change

Absolutely

I don't fully understand this sentence: "Note that this does not affect method signatures: the clauses needed to call a method don't change."

I lack good terminology here. The important property about the two-binding-levels-for-methods I introduced recently is that the Binder<FunDeclRef> for a particular method in a TraitDecl binds exactly the same generics as the Binder<FunDeclRef>for that same method in a TraitImpl for that trait. These are exactly the generics that must be supplied by the GenericArgs of a FnPtr::Trait pointing to that method.

The note I was trying to make is that these generics are unchanged by this PR. The thing that is changed by this PR is the generics of the FunDecls that correspond to method declarations. Which is the only sensible thing to do anyway, maybe this note added more confusion than it removed ^^.

Both for consistency fir the corresponding method generics and in case
the default const uses values from `Self`.
It was causing type inference problems in visitors.
@Nadrieril Nadrieril force-pushed the explicit-self-in-methods branch from 7d70f18 to c7243f2 Compare January 9, 2025 10:16
@sonmarcho
Copy link
Member

But we agree that implementations of trait methods don't change

Absolutely

I don't fully understand this sentence: "Note that this does not affect method signatures: the clauses needed to call a method don't change."

I lack good terminology here. The important property about the two-binding-levels-for-methods I introduced recently is that the Binder<FunDeclRef> for a particular method in a TraitDecl binds exactly the same generics as the Binder<FunDeclRef>for that same method in a TraitImpl for that trait. These are exactly the generics that must be supplied by the GenericArgs of a FnPtr::Trait pointing to that method.

The note I was trying to make is that these generics are unchanged by this PR. The thing that is changed by this PR is the generics of the FunDecls that correspond to method declarations. Which is the only sensible thing to do anyway, maybe this note added more confusion than it removed ^^.

This is clear and makes sense to me, thanks :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants