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

Support adding bounds only to fields #10

Closed
nrxus opened this issue Mar 17, 2018 · 5 comments
Closed

Support adding bounds only to fields #10

nrxus opened this issue Mar 17, 2018 · 5 comments

Comments

@nrxus
Copy link
Contributor

nrxus commented Mar 17, 2018

Given I have a trait like this:
Foo,
and a struct:
Bar<T, Q>

I want to be able to write a derive macro that transforms:

#[derive(Foo)]
struct Bar<T, Q> {
  field: SomeGenericStruct<Q>,
  another_field: T,
}

into:

impl<T, Q> Foo for Bar<T, Q>
  where SomeGenericStruct<Q>: Foo,
             T: Foo,
{
  // implement trait methods here....
}

Currently, synstructure allows me to either generate code where every generic and field is bounded by my new trait.

impl<T, Q> Foo for Bar<T, Q>
  where SomeGenericStruct<Q>: Foo,
             T: Foo,
             Q: Foo,
{
  // implement trait methods here....
}

or generate code where neither my fields nor my generic parameters are bounded by my new trait.

impl<T, Q> Foo for Bar<T, Q>
{
  // implement trait methods here....
}

This makes it impossible to derive code where only only my fields need the bound and it is in fact incorrect to bind some of my generics by my trait.

I understand this is also how many (all?) of the common rust derives come (ie: Derive(Clone)) but I find it limiting in certain aspects. It would be nice (and needed for my use case) to add another layer of granularity to which bounds to add or not (:

@dtolnay
Copy link

dtolnay commented Mar 17, 2018

I believe bounds on the field types only is never what you want -- see dtolnay/syn#370.

@nrxus
Copy link
Contributor Author

nrxus commented Mar 17, 2018

Thank you for the context @dtolnay!

The current functionality of this crate already binds to fields which generics, while additionally binding to each generic parameter in each field. I don't think introducing an in-between level of which bounds to apply would introduce any of the issues mentioned in that comment.

I should have probably been more clear in the issue I created, I meant bounds to fields with generics, same as the current functionality.

I think it's difficult to say that bounds on field types with generics is never what someone would want because it is what I want.

I agree there are cases in which having bounds on the fields would not be desired and would break the derive macro (at compile time). This new finer-grained bound would be an opt-in and not change any current behavior, thus making this "harder" case possible rather than impossible, while not affecting the simple use case of this crate. In addition it would break in the way it would currently break already since I am not adding any bounds, just allowing to filter-in the kinds of bounds I want.

Maybe an additional part of addressing this issue would be also adding the ability to only add bounds to the generic parameters and not to the fields, thus having 4 levels of bounds:

  • None
  • Only on fields with generics
  • Only on generics within fields
  • All

For context, my use case that pushed me to create this issue is rendering of a game component, like this:

#[derive(Show)]
struct GameScreenAssets<T: Texture, F: Font> {
   player: PlayerAssets<T>,
   hud: HudAssets<T, F>,
   // other assets
}

In this case I do not need the F: Font to implement Show. In fact it would be incorrect to try to "show" a Font. What I want is for my HudAssets to derive show when it has the T and F generic parameter. How my HudAssets<T,F> struct implement Show is up to it.

@nrxus nrxus mentioned this issue Mar 20, 2018
@mitsuhiko
Copy link
Contributor

I ran into the same thing I believe. I have a struct like this:

#[derive(Clone, Debug, MetaStructure)]
pub struct ValuesHelper<T> {
    pub values: Annotated<Array<T>>,
    #[metastructure(additional_properties)]
    pub other: Object<Value>,
}

And right now it derives Annotated<Array<T>>: MetaStructure which is incorrect in my case. The bound I actually need is Array<T>: MetaStructure or just T: MetaStructure.

@nrxus
Copy link
Contributor Author

nrxus commented Oct 27, 2018

#20 fixes this so I am marking it as closed!! Thanks @mitsuhiko

The decision to make add_bounds only applies to gen_impl instead of it also applying to bound_imp and its unsafe counterpart is confusing in my opinion. There is no reason why the bounds specified in add_bounds cannot be used in bound_impl since the default is to add both kinds which means it is a non-breaking change either way.

I am also concerned that no tests were provided but the issue is fixed.

@nrxus nrxus closed this as completed Oct 27, 2018
@mystor
Copy link
Owner

mystor commented Oct 28, 2018

The decision to make add_bounds only applies to gen_impl instead of it also applying to bound_imp and its unsafe counterpart is confusing in my opinion. There is no reason why the bounds specified in add_bounds cannot be used in bound_impl since the default is to add both kinds which means it is a non-breaking change either way.

I'm changing this behaviour before the 0.15.1 release such that bound_impl and unsafe_bound_impl are affected by add_bounds.

I'm also adding some basic doctests.

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

No branches or pull requests

4 participants