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

Selective AsRef/AsMut impl #298

Merged
merged 59 commits into from
Sep 15, 2023

Conversation

MegaBluejay
Copy link
Contributor

@MegaBluejay MegaBluejay commented Aug 22, 2023

Resolves #285

Synopsis

The AsRef/AsMut derives are less powerful than From and Into, because there's no way to specify types into which forwarded implementations should convert.

Solution

Keep using the forward attribute argument for blanket forwarded impls, and use a list of types for selective ones.

Checklist

  • Documentation is updated (if required)
  • Tests are added/updated (if required)
  • CHANGELOG entry is added (if required)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These are the same kind of errors From and Into produce.
Unfortunately, as far as I can tell, there isn't any way of customizing these messages, since we can't check whether a type is in scope during macro expansion.

/// Blanket impl, fully forwarding to the field type
Forward(forward::Attribute),
/// Forward implementation, but only impl for specified types
Types(Punctuated<Type, Token![,]>),
Copy link
Contributor Author

@MegaBluejay MegaBluejay Aug 22, 2023

Choose a reason for hiding this comment

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

I don't think it makes sense to extract the type list attribute into utils like forward/skip, even though
the same arguments are used in From and Into, because parsing Punctuated is quite concise and understandable already

@MegaBluejay MegaBluejay marked this pull request as ready for review August 22, 2023 16:09
Copy link
Collaborator

@tyranron tyranron left a comment

Choose a reason for hiding this comment

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

@MegaBluejay

I don't think it makes sense to extract the type list attribute into utils like forward/skip, even though
the same arguments are used in From and Into, because parsing Punctuated is quite concise and understandable already

I don't really like this, there are still a lot of unnecessary duplication. But for keeping things simpler: refactoring this is beyond this PR's topic, and would be done in a separate one. So, for merging this PR, it's OK.


Regarding the PR itself, it lacks implementation of one essential feature. See the comments below.

I think it was missed due to the fact, that you first wrote some code, and only latter added the tests, covering what you had implemented with your code. This led to the huge implementation-bias when you were writing the tests, as you thought only about what you had implemented.
That's why I propose and advise you to take the opposite, more TDDish, approach in the future, when implementing new features: first, you think of all feature combinations and edge cases, and fill them up as tests cases, so you have some high-level overview of what should be done, and cannot forget them, and only then you write the implementation covering those tests cases.


// Asserts that the macro expansion doesn't generate `AsRef` impl for unmentioned type, by
// producing trait implementations conflict error during compilation, if it does.
impl AsRef<bool> for Types {
Copy link
Collaborator

Choose a reason for hiding this comment

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

@MegaBluejay that's a very strange assertion. Nothing mentions bool in the type signature, so why we check for it? It seems much more reasonable to check that impl AsRef<Foo> is absent, because it is not specified explicitly (we have similar behavior for derive(From)).

Copy link
Contributor Author

@MegaBluejay MegaBluejay Aug 28, 2023

Choose a reason for hiding this comment

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

The field type Foo implements AsRef<bool>, and
the assertion is there to make sure no blanket impl is generated.
I'll reword the comment to make what's going on more clear

#[derive(AsRef)]
struct Types {
#[as_ref(str)]
first: String,
Copy link
Collaborator

@tyranron tyranron Aug 28, 2023

Choose a reason for hiding this comment

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

@MegaBluejay we're clearly lacking here test cases like:

        #[derive(AsRef)]
        struct Types {
            #[as_ref(str, String)]
            first: String,

For From it's trivial, because the trait provides the blanket impl<T> From<T> for T {, but for AsRef/AsMut there are none such blankets, yet still we should support this case naturally, even considering something like that:

type Wtf = String;

#[derive(AsRef)]
#[as_ref(str, Wtf)]
struct Foo(String);

Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess, for supporting this, we would need some type-level machinery providing autoref-based specialization in the macro expansion, so we could specialize in our transparent wrapper the impl<T> AsRef<T> for T case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tyranron
I don't think this syntax can work on stable.

Autoref-based specialization gets most of the way there, but can't handle generics.

For instance, without something to differentiate these 2 cases, there's no way of generating correct impl bounds for both

// where T: TypeEquals<Other = T>
#[derive(AsRef)]
#[as_ref(T)]
struct Foo<T>(T);

// where T: AsRef<i32>
#[derive(AsRef)]
#[as_ref(i32)]
struct Bar<T>(T);

As an alternative, we could allow multiple attributes

#[derive(AsRef)]
#[as_ref]
#[as_ref(str)]
struct Foo(String);

Or use a special keyword for the field type

#[derive(AsRef)]
#[as_ref(str, direct)]
struct Foo(String);

Either one would remove the need for specialization.

Copy link
Collaborator

@tyranron tyranron Aug 29, 2023

Choose a reason for hiding this comment

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

@MegaBluejay

Autoref-based specialization gets most of the way there, but can't handle generics.

But do we need to?

#[derive(AsRef)]
#[as_ref(T)]
struct Foo<T>(T);

This seems to be trivial, as we cannot alias a type parameter. So we can just check whether it's contained in the type generics and treat this situation as #[as_ref].

Copy link
Contributor Author

Choose a reason for hiding this comment

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

One case I don't think we could handle this way is when the field type is itself generic and uses a type parameter

#[derive(AsRef)]
#[as_ref(i32)]
struct Foo<T>(T);

#[derive(AsRef)]
#[as_ref(Foo<T>, T)]
struct Bar<T>(Foo<T>);

&Bar<T> -> &T should be forwarded, even though there's a type parameter involved.

I'll look into this some more and figure out if there's a good way of working around this

Copy link
Collaborator

Choose a reason for hiding this comment

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

@MegaBluejay

First, I don't understand all the squats around Cell. Seems totally redundant and misleading, so I've removed it. It's not required for type-level reasoning (coherence) at all.

Second, your example seems to work just fine with autoref-based specialization, even when a type alias is inclined: playground.

Third, we may omit using autoref-based specialization in the cases where the specified type is named exactly the same as the one of the field:

#[derive(AsRef)]
#[as_ref(Vec<T>)]
struct Stack<T>(Vec<T>);

// expands to just:
impl<T> AsRef<Vec<T>> for Stack<T> {
    fn as_ref(&self) -> &Vec<T> {
        &self.0
    }
}

We need only involve autoref-based specialization machinery when some "outer" types are used.

Given all of these, it seems that we're covering all the possible cases here. If not, prove me wrong with an example that doesn't compile on playground, please.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@MegaBluejay ah yes, of course, we cannot conditionally generate that bound:

where
    Foo<T>: AsRef<T>,

We either can generate it for both impls, which breaks the first impl, or doesn't generate it at all, which breaks the second impl.

Let me think over it a little bit... it seems that the problem is not fundamental, and we can overcome it by bounding with Conv<_>: ExtractRef somehow, which will automatically imply the necessary conditions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cell is required for the AsMut version, because Conv needs to contain a mutable reference.

As for bounding with Conv, I don't think it's possible, since code using autoref-specialization isn't actually valid for all its cases at once, it just compiles differently in different contexts, which can't be expressed in types

Copy link
Collaborator

Choose a reason for hiding this comment

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

@MegaBluejay

Cell is required for the AsMut version, because Conv needs to contain a mutable reference.

Yes, variance gives there some hard times for mutable references. Let's keep separate machinery traits for AsRef and AsMut versions, so we don't bother AsRef version with runtime impact of a Cell. Regarding AsMut, for now, use Cell in machinery, but give some more time poking around to make it working without Cell. There should be a way, imho.

As for bounding with Conv, I don't think it's possible, since code using autoref-specialization isn't actually valid for all its cases at once, it just compiles differently in different contexts, which can't be expressed in types

True. We cannot express "OR" bounds, only "AND", so bringing all the bounds, we disjunct both cases, rather than conjunct them.

But we can use some trickery here, inspecting the type names. The whole algorithm would look like something like this:

  1. We inspect the provided type to match by name the field type, if it does, we just use trivial #[as_ref] logic without specialization. It will easily handle all the cases of impl<T> AsRef<T> for T
    #[derive(AsRef)]
    #[as_ref(Vec<T>)]
    struct Stack<T>(Vec<T>);
    
    // expands to just:
    impl<T> AsRef<Vec<T>> for Stack<T> {
        fn as_ref(&self) -> &Vec<T> {
            &self.0
        }
    }
    except the cases where type is renamed
    type V<T> = Vec<T>;
    
    #[derive(AsRef)]
    #[as_ref(V<T>)]
    struct Stack<T>(Vec<T>);
  2. If the provided type doesn't match the field type, we use the specialization wrapper.
  3. If the provided type contains one of type parameters of the declared type (we can recursively inspect it), we add the FieldType: AsRef<ProvidedType<T>> bound, which enables the second specialization impl to work in generic contexts. Even more, such as we choose here the second specialization impl only, we may not use specialization totally here, falling into the assumption, that if the types named differently - they are different types.

This way, the only situations that don't work would be when we convert directly to the field type, which involves generics, and is named differently in field and attribute:

type V<T> = Vec<T>;

#[derive(AsRef)]
#[as_ref(V<T>)]
struct Stack<T>(Vec<T>);

I would say that this is a very rare situation. Why would one name differently the same type in the same place? Moreover, we should state explicitly in the docs, that this concrete edge case won't work as expected.

Note, that we still have specialization working okay when generics are not involved, allowing to use renamed types.


And another direction that is worth a shot, is another kind of specialization: runtime same-type specialization, which inspects a type to be the same in runtime and transmutes, but TypeId and any machinery is limited to 'static lifetime only, unfortunately, which would likely limit kinds of types we can place the macro onto very-very unpractically much.

Copy link
Contributor Author

@MegaBluejay MegaBluejay Aug 30, 2023

Choose a reason for hiding this comment

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

I wrote out some more impls manually following this algorithm

We'd need to omit specialization in a wider number of cases, both when the provided type contains type parameters, and when the field type does.

For instance, this case

#[derive(AsRef)]
#[as_ref(i32)]
struct Foo<T>(T);

Specialization can't work here either, this falls into the same situation you described

Other than that everything seems to work.

impl/doc/as_ref.md Outdated Show resolved Hide resolved
@tyranron tyranron added this to the 1.0.0 milestone Aug 28, 2023
tyranron
tyranron previously approved these changes Sep 5, 2023
Copy link
Collaborator

@tyranron tyranron left a comment

Choose a reason for hiding this comment

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

@MegaBluejay good job, thanks! 🍻


let impl_kind = 'ver: {
if is_blanket {
break 'ver Forwarded;
Copy link
Collaborator

Choose a reason for hiding this comment

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

@MegaBluejay just use if else, Luke! 🙈

@tyranron
Copy link
Collaborator

tyranron commented Sep 5, 2023

GitHub ignores my commits 🥲

Copy link
Collaborator

@tyranron tyranron left a comment

Choose a reason for hiding this comment

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

@JelteF this one is relatively big and introduces a new feature. Though, I think it's polished quite well for now. Would you like to take a look?

@tyranron
Copy link
Collaborator

tyranron commented Sep 7, 2023

@JelteF ping

@JelteF
Copy link
Owner

JelteF commented Sep 7, 2023

I'll take a look this weekend. I'd like to double check if I like the the API interface

@tyranron
Copy link
Collaborator

@JelteF ping

@JelteF JelteF merged commit bbda6eb into JelteF:master Sep 15, 2023
16 checks passed
@tyranron tyranron mentioned this pull request Sep 18, 2023
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

AsRef restricted impls
3 participants