ia0 / data-encoding

Efficient and customizable data-encoding functions in Rust
https://data-encoding.rs/
MIT License
177 stars 23 forks source link

Removing dependency on syn #56

Closed chinedufn closed 2 years ago

chinedufn commented 2 years ago

Hey!

I was reading through data-encoding and it looks awesome. Thanks for your work.

I'm investigating whether or not I can use data-encoding-macro in my own project .

It looks like a perfect fit, except for one issue.

I checked the data-encoding-macro-internal dependencies and saw that it depends on syn. syn is a fairly heavy dependency that I wouldn't want to pull into my tiny library.

After quickly scanning through data-encoding-macro and data-encoding-macro-internal's source code I'm not seeing anything that couldn't be accomplished with regular macro_rules! declarative macros.

Am I missing something? Was there a specific reason that syn was chosen?

If not, are you open to not using syn in data-encoding-macro?

Thanks!

ia0 commented 2 years ago

Hi @chinedufn! Thanks for raising this issue!

are you open to not using syn in data-encoding-macro?

Could you expand as to why this dependency is an issue? This could help me find out how to best solve the issue.

I'm not seeing anything that couldn't be accomplished with regular macro_rules! declarative macros

I'm not convinced Rust compile-time functionality is good enough yet for what data-encoding-macro is doing. But I'll take a look. Otherwise, it might indeed be possible to write the proc-macro without syn. But I'll need to check what it would cost and compare with the issue that it would solve.

ia0 commented 2 years ago

Regarding compile-time functions, nightly would be needed and even then it's not sure. Also I don't really want to split the code base between stable and nightly unless there's a very good reason (or I release as a nightly-only v3). Ideally I'll prefer to wait until compile-time functions get more stable.

chinedufn commented 2 years ago

Hey! Thanks for taking the time to engage!

Could you expand as to why this dependency is an issue? This could help me find out how to best solve the issue.

Sure! Syn is very powerful, but that power comes at a cost of higher compile times.

So I think that it's best to avoid it when it isn't necessary.

To illustrate. Given the following:

cargo new --lib syn-compile-time
cd syn-compile-time
echo "syn = \"1\"" >> Cargo.toml
echo "pub use syn;" >> src/lib.rs
cargo check # Generate a Cargo.lock and download dependencies
cargo clean && cargo build --release

After running this on the following hardware:

MacBook Pro (16-inch, 2019)
2.4 GHz 8-Core Intel Core i9
64 GB 2667 MHz DDR4

I see a compile time of around 7 seconds:

Finished release [optimized] target(s) in 7.19s

Regarding compile-time functions, nightly would be needed and even then it's not sure.

Sorry, to clarify: I'm just talking about defining a macro_rules! declarative macro. I'm not talking about const fns.

I'm not convinced Rust compile-time functionality is good enough yet for what data-encoding-macro is doing.

Which aspect are you thinking can't be handled by a regular declarative macro?

I'm thinking that a proc macro might be required at all, but I haven't looked into what's going on deeply enough yet to feel sure. So I might be missing something here.

I'd be happy to try to submit a PR that works on stable without a proc macro if you're open to reviewing it. (Unless you're saying that there are things going on that would require nightly const fns? In which case I won't.)


Hopefully I've given enough context to answer everything that you asked? Let me know! Thanks.

chinedufn commented 2 years ago

Ahhhhh ok I started looking at the code more deeply and I think I see what you're saying.

The proc macro lets you run this on the inputs at compile time:

https://github.com/ia0/data-encoding/blob/cbc675226608445c3c741f2cf3fe52298918b5d2/lib/macro/internal/src/lib.rs#L159

Makes sense.


Ok cool so a proc macro is necessary until if/when const fns let that decode function run at compile time on stable.

Makes sense.


As for me.. Now that I understand that a proc macro is a requirement I'm comfortable pulling in the dependency.

Thanks for walking me through that. I'm good to go. Removing syn would still be nice but I haven't explored whether or not that's even possible.

At any rate.. I'm going move forward with using data-encoding.

Feel free to close this if you like, or to keep it open if we ever want to investigate whether or not removing syn is practical.

Cheers!

ia0 commented 2 years ago

Thanks for your feedback! Indeed compile-time is an interesting concern I didn't think of (I was thinking about possible supply-chain attacks, code bloat, or code performance).

I'll take a stab at removing syn if possible and update on this issue with my conclusion.

ia0 commented 2 years ago

I don't think it is possible to get rid of syn. Here is the reasoning:

However, I managed to reduce the compile time of syn by only using the features I need, from 2.25 seconds to 0.99 seconds.

chinedufn commented 2 years ago

Awesome, thanks!