Skip to content

derive_where attribute macro instead of derive macro #33

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

Merged
merged 12 commits into from
Jan 10, 2022
Merged

Conversation

ModProg
Copy link
Owner

@ModProg ModProg commented Jan 5, 2022

closes #27

@ModProg ModProg requested a review from daxpedda January 5, 2022 11:19
@daxpedda
Copy link
Collaborator

daxpedda commented Jan 5, 2022

Lol, after all this talk on how to do this, it looks ingenius to me. Looking forward to see how this plays out.

@ModProg
Copy link
Owner Author

ModProg commented Jan 5, 2022

Lol, after all this talk on how to do this, it looks ingenius to me. Looking forward to see how this plays out.

The only problem seams to be that prior to 1.52.1, you cannot have an attribute_macro be the same identifier as a derive_helper:

error[E0659]: `derive_where` is ambiguous (derive helper attribute vs any other name)
  --> tests/bound.rs:21:2
   |
21 |     #[crate::derive_where(Clone; T::Type)]
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ ambiguous name
   |
note: `derive_where` could refer to the derive helper attribute defined here
  --> tests/bound.rs:21:2
   |
21 |     #[crate::derive_where(Clone; T::Type)]
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
note: `derive_where` could also refer to the attribute macro imported here
  --> tests/bound.rs:5:20
   |
5  | use derive_where::{derive_where, DeriveWhere};
   |                    ^^^^^^^^^^^^
   = help: use `crate::derive_where` to refer to this attribute macro unambiguously

Only way around this is removing the import for the derive_where macro and calling it like so:

#[derive_where::derive_where(Clone; T::Type)]
struct Test<T: Trait>(T::Type);

This can be somewhat circumvented by using an internal replacement: #[__derive_where_internal_derive_attr(#attr)] .
But that breaks down as soon as you add additional derive_where attributes for default or skip or something else.

So pre 1.52.1 do not import derive_where::derive_where, use an alias or use the full path when invoking the macro:

use derive_where::derive_where as dw_macro;
#[dw_macro(Clone, Copy, Debug)]
#[derive_where(Default, Eq, Hash, Ord, PartialEq, PartialOrd)]
enum Test<T> {
	#[derive_where(default)]
	A {
		a: Wrapper<T>,
	},
	B(Wrapper<T>),
	C,
}

But since 1.52.1 this would enable using it like so:

use derive_where::derive_where;
#[derive_where(Clone, Copy, Debug)]
#[derive_where(Default, Eq, Hash, Ord, PartialEq, PartialOrd)]
enum Test<T> {
	#[derive_where(default)]
	A {
		a: Wrapper<T>,
	},
	B(Wrapper<T>),
	C,
}

@daxpedda
Copy link
Collaborator

daxpedda commented Jan 5, 2022

I'm starting to remember what the issues were, how are we going to resolve multiple calls and paths?

use derive_where::derive_where as blubb;

#[blubb(Clone, Copy, Debug)]
#[blubb(Default, Eq, Hash, Ord, PartialEq, PartialOrd)]
struct Test<T: Trait>(T::Type);

@ModProg
Copy link
Owner Author

ModProg commented Jan 5, 2022

I'm starting to remember what the issues were, how are we going to resolve multiple calls and paths?

I would rather have to do

use derive_where::derive_where as blubb;

#[blubb(Clone, Copy, Debug)]
#[derive_where(Default, Eq, Hash, Ord, PartialEq, PartialOrd)]
struct Test<T: Trait>(T::Type);

and if I only need one be able to do:

use derive_where::derive_where as blubb;

#[blubb(Clone, Copy, Debug)]
struct Test<T: Trait>(T::Type);

than always have the #[derive(DeriveWhere)]

we can make this a feature that can be opt-out/opt-in, if there is no way around this limitation.

another problem is, that in this case the first invocation would fail on the default

#[dw_macro(Clone, Copy, Debug)]
#[dw_macro(Default, Eq, Hash, Ord, PartialEq, PartialOrd)]
enum Test<T> {
	#[derive_where(default)]
	A,
    B(PhantomData<T>),
}

But with the internal_derive I can fix multiple dw_macro calls. but not multiple dw_macro calls plus a derive_where attr:

#[dw_macro(Clone, Copy)]
#[dw_macro(Eq, Hash, Ord, PartialEq, PartialOrd)]
#[derive_where(Debug)]
enum Test<T> {
	A {
		a: Wrapper<T>,
	},
	B(Wrapper<T>),
	C,
}

But this would have a msrv of 1.57.0:

use derive_where::derive_where as dw_macro;
#[dw_macro(Clone, Copy, Debug)]
#[dw_macro(Eq, Hash, Ord, PartialEq, PartialOrd)]

as prior you will get

error: macro attributes must be placed before `#[derive]`
  --> tests/enum_mixed.rs:18:2
   |
18 |     #[dw_macro(Eq, Hash, Ord, PartialEq, PartialOrd)]
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

TLDR

We can support prior 1.52.1:

use derive_where::derive_where as dw_macro;
#[dw_macro(Clone, Copy, Debug)]
#[derive_where(Default, Eq, Hash, Ord, PartialEq, PartialOrd)]
enum Test<T> {
	#[derive_where(default)]
	A,
	B(Wrapper<T>),
}

We can support prior 1.57.0:

use derive_where::derive_where;
#[derive_where(Clone, Copy, Debug)]
#[derive_where(Default, Eq, Hash, Ord, PartialEq, PartialOrd)]
enum Test<T> {
	#[derive_where(default)]
	A,
	B(Wrapper<T>),
}

With current Rust, we can support:

use derive_where::derive_where  as dw_macro;
#[dw_macro(Clone, Copy, Debug)]
#[dw_macro(Default, Eq, Hash, Ord, PartialEq, PartialOrd)]
enum Test<T> {
	#[derive_where(default)]
	A,
	B(Wrapper<T>),
}

But probably neither:

use derive_where::derive_where  as dw_macro;
#[dw_macro(Clone, Copy, Debug)]
#[dw_macro(, Eq, Hash, Ord, PartialEq, PartialOrd)]
#[derive_where(Default)]
enum Test<T> {
	#[derive_where(default)]
	A,
	B(Wrapper<T>),
}

nor:

use derive_where::derive_where  as dw_macro;
#[dw_macro(Clone, Copy, Debug)]
#[dw_macro(Default, Eq, Hash, Ord, PartialEq, PartialOrd)]
enum Test<T> {
	#[dw_macro(default)]
	A,
	B(Wrapper<T>),
}

@ModProg
Copy link
Owner Author

ModProg commented Jan 5, 2022

Ok maybe we can do in current rust

use derive_where::derive_where  as dw_macro;
#[dw_macro(Clone, Copy, Debug)]
#[dw_macro(, Eq, Hash, Ord, PartialEq, PartialOrd)]
#[derive_where(Default)]
enum Test<T> {
	#[derive_where(default)]
	A,
	B(Wrapper<T>),
}

The idea would be to replace all derive_where calls with

#[derive(DeriveWhere)]
#[__derive_where_internal_derive_attr]

@ModProg
Copy link
Owner Author

ModProg commented Jan 5, 2022

If rust-lang/rust#53012 is fixed, we could also do this:

use derive_where::derive_where  as dw_macro;
#[dw_macro(Clone, Copy, Debug)]
#[dw_macro(Default, Eq, Hash, Ord, PartialEq, PartialOrd)]
enum Test<T> {
	#[dw_macro(default)]
	A,
	B(Wrapper<T>),
}

All we need to do is have the attribute macro expand to derive_where when on a field.
for now the only way would be to pass the attribute name to dw_macro and have the first procmacro do the replacing. But that feals so unergonomic that I would just say If you want to use attributes on fields, you have to use derive_where even when the macro has a different name

@ModProg
Copy link
Owner Author

ModProg commented Jan 5, 2022

Oh and we would need to throw out all the sanity checks like dont have default if there is no derive_where(Default) as no single invocation knows which other invocation will contain Default.

I could live with that, but this is a nice feature we would loose this way. (Don't think this would be a breaking change tho as only some errors will get removed)

@ModProg
Copy link
Owner Author

ModProg commented Jan 5, 2022

As sanity checks only need to be disabled if someone renames derive_where and calls the macro multiple times instead of using the derive attribute derive_where I think making this usage an opt-in feature to enable sounds like a good idea, (If we even want to support it) by having an disable_sanity_checks attribute/feature.

@ModProg
Copy link
Owner Author

ModProg commented Jan 5, 2022

As the usage of the attribute is pretty seamless (as far as I can tell) in up-to-date rust versions, we can make this the normal (maybe even only) way of using derive_where.
But as we are using the #[derive(DeriveWhere)] way internally, we could also keep it as a different (maybe deprecated) way of doing it.

@ModProg ModProg force-pushed the derive_where-attr branch from 168fa51 to e06a40d Compare January 9, 2022 13:52
@ModProg ModProg linked an issue Jan 9, 2022 that may be closed by this pull request
@ModProg ModProg force-pushed the derive_where-attr branch from e06a40d to 51a37b9 Compare January 9, 2022 17:52
@ModProg ModProg marked this pull request as ready for review January 9, 2022 17:52
@ModProg ModProg force-pushed the derive_where-attr branch from cfc92e3 to e342c42 Compare January 9, 2022 18:02
@ModProg ModProg force-pushed the derive_where-attr branch from 7823762 to 589f6b9 Compare January 9, 2022 18:43
@ModProg ModProg force-pushed the derive_where-attr branch from 589f6b9 to 36913ba Compare January 9, 2022 18:43
Copy link
Collaborator

@daxpedda daxpedda left a comment

Choose a reason for hiding this comment

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

This looks good. https://github.com/ModProg/derive-where/pull/33/files#r780831274 is the only thing really left to do, the rest are nits.

Copy link
Owner Author

@ModProg ModProg left a comment

Choose a reason for hiding this comment

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

Should be fine now, hopefully

@ModProg ModProg changed the title derive_where attribute macro that triggers derive of DeriveWhere derive_where attribute macro instead of derive macro Jan 10, 2022
@ModProg ModProg requested a review from daxpedda January 10, 2022 04:21
@ModProg
Copy link
Owner Author

ModProg commented Jan 10, 2022

we should document the restriction conserning renaming, and maybe run a test how useful the error message in such a case will be.

@ModProg
Copy link
Owner Author

ModProg commented Jan 10, 2022

And like I said test what happens if put the attribute_macro on a non struct/enum/unit.

Copy link
Collaborator

@daxpedda daxpedda left a comment

Choose a reason for hiding this comment

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

The span issue is the only thing remaining.

we should document the restriction conserning renaming, and maybe run a test how useful the error message in such a case will be.

And like I said test what happens if put the attribute_macro on a non struct/enum/unit.

We could also do these in a separate PR, however you prefer.

@daxpedda
Copy link
Collaborator

Feel free to merge or handle these last two points in this PR.

@ModProg ModProg merged commit c4e01dc into main Jan 10, 2022
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.

Remove need for derive
2 participants