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

Externalize derive macro implementation #2765

Closed

Conversation

vic1707
Copy link

@vic1707 vic1707 commented Jun 28, 2024

Hi,
I'm currently trying to make a crate built on top of serde to try to implement a relatively naive idea for struture versioning regarding #1137 as a POC (and hopefully a suitable solution).

For ease of use (both as the developper and user of the crate) I'd like my crate to be able to benefit from serde's derive implementation that is thankfully in a separate function that could be exported.

This would allow my crate (and other ones of the same kind) to be 100% compatible with all of serde derive's attribute and features by manually running the derive function and modifying the AST with quote and syn.

This would allow crate developers to inject their features direclty on the Serialize or Deserialize implementation outputed by serde_derive, kinda like what I'm doing:

--- usage.expanded.initial.rs	2024-06-28 23:42:09
+++ usage.expanded.modified.rs	2024-06-28 23:42:09
@@ -4,7 +4,7 @@
 use std::prelude::rust_2021::*;
 #[macro_use]
 extern crate std;
-use serde_derive::Deserialize;
+use my_serde::Deserialize;
 struct Foo {
     name: String,
     age: u8,
@@ -24,6 +24,11 @@
         where
             __D: _serde::Deserializer<'de>,
         {
+            'my_serde: {
+                {
+                    ::std::io::_print(format_args!("HI!!!\n"));
+                };
+            }
             #[allow(non_camel_case_types)]
             #[doc(hidden)]
             enum __Field {

original file being:

/* Clippy config */
#![allow(dead_code)]
/* Dependencies */
use serde_derive::Deserialize;

#[derive(Deserialize)]
struct Foo {
    name: String,
    age: u8,
    #[serde(default)]
    place_holder: String,
}

fn main() {}

This is, I think, kinda related to #2021.

To do so I pretty much replicated the serde_derive_internals situation.

If this change is ok to you (if its not I can simply use my fork for the time being), I'd like to have your opinion on how you want me to fix the current clippy issues (the lib file currently being the same as for serde_derive_internals)
Simply add a bunch of #[allow()] on the lib.rs, try my best to fix every one of them, alternative ?

Notes

English isn't my primary language, I'm sorry if I made mistakes, if anything sounds bad or if I'm hard to understand 😓.

@vic1707 vic1707 force-pushed the test-derive-impl-externalization branch from 4a3641c to 0925e7d Compare July 9, 2024 10:34
@vic1707
Copy link
Author

vic1707 commented Jul 10, 2024

Just released my code for versioning here if you want to take a look at what this PR allows ^^
https://github.com/vic1707/serde-versioning

btw, if you have time, I'd love to hear your opinion the project

@vic1707
Copy link
Author

vic1707 commented Nov 6, 2024

May I ask for your review @dtolnay ?

@@ -0,0 +1 @@
../serde_derive/src/implementation/
Copy link
Member

Choose a reason for hiding this comment

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

Is this a symbolic link?

Copy link
Author

Choose a reason for hiding this comment

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

yup it was, just like for serde_derive_internals
See: https://github.com/serde-rs/serde/blob/master/serde_derive_internals/src

@oli-obk
Copy link
Member

oli-obk commented Nov 6, 2024

The issues raised in #2021 are not really resolved by this.

I think this is a feature needed commonly enough that we could probably quickly figure out a small robust rust language feature to allow you to call proc macros from other proc macros. I think that would be better in general

@oli-obk
Copy link
Member

oli-obk commented Nov 6, 2024

Going to close this as I don't see it landing in this form any time soon.

@oli-obk oli-obk closed this Nov 6, 2024
@vic1707
Copy link
Author

vic1707 commented Nov 6, 2024

I think this is a feature needed commonly enough that we could probably quickly figure out a small robust rust language feature to allow you to call proc macros from other proc macros. I think that would be better in general

I also thought of a simple feature gate, but AFAIK

[lib]
proc-macro = true

cannot be set via by feature gate, at least not last time I checked, but it would be really nice to get that !
I also tried to enable it conditionally via the build.rs but it didn't work which is why I resulted to duplicating the serde_derive_internals solution.
(I'm trying to find the sources and issue about that without luck for now, will update if I ever find them again)

Do you have other ideas I could explore ?

@oli-obk
Copy link
Member

oli-obk commented Nov 6, 2024

For now you can create a git repo with this repo as a submodule and create your crate that way. It'll be on you to update then, but that was already true. The main problem with those approaches is keeping in sync with the serde version for the internal API that the derives use

@oli-obk
Copy link
Member

oli-obk commented Nov 6, 2024

I don't know what would be needed on the rustc side to support invoking proc macros from other proc macros, but it doesn't seem insurmountable if the lang team agrees this is desirable

@vic1707
Copy link
Author

vic1707 commented Nov 6, 2024

For now you can create a git repo with this repo as a submodule and create your crate that way.

Haha, I honestly didn't even thought about submodules, thanks for the idea!


The main problem with those approaches is keeping in sync with the serde version for the internal API that the derives use

Yup but that's a burden for libraries developers not for serde maintainers right ?
Lib devs would write down which version of serde is supported for each versions of their crates just like some already do when depending on/extending serde_derive themselves (thinking of crates like nutype which, I think, could face similar sync issues)?
And no matter how I'm able to run serde_derive manually I will be facing those issues right?

(Not trying to get the PR opened again, just genuinely trying to understand, I probably am missing a point 😄)


I don't know what would be needed on the rustc side to support invoking proc macros from other proc macros, but it doesn't seem insurmountable if the lang team agrees this is desirable

Don't know either, hopefully I can find the issues/RFC back and link this PR or ask questions about it 🙃

@oli-obk
Copy link
Member

oli-obk commented Nov 6, 2024

Yup but that's a burden for libraries developers not for serde maintainers right ?

Oh yea I meant problems for you if you go with the submodule approach and potentially users of your crate.

Though it may be automatable in a cronjob to generate and publish the crate whenever a new serde_derive is released

@vic1707
Copy link
Author

vic1707 commented Nov 6, 2024

Oh, I thought you were saying that for serde and the PR itself, yup it could become a pain for me and others if serde changes too much.

I didn't saw it as an issue for me as in #1137 dtolnay said that if an extension of serde becomes a popular/viable solution it could get merged directly in serde and was hoping I could one day get that chance 🙃.

Thanks for the explanations and your time 👍

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

Successfully merging this pull request may close these issues.

2 participants