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

chore: remove dependency on syn_derive #318

Merged
merged 1 commit into from
Nov 7, 2024

Conversation

kayagokalp
Copy link
Contributor

@kayagokalp kayagokalp commented Nov 5, 2024

Removes dependency on syn_derive crate.

The added dependency is saving 21 lines of code at the cost of added Cargo.lock entries for a large list of projects depending on borsh-rs. The motivation came from the fact that proc-macro-error crate is unmaintained and the rust ecosystem is slowly transitioning into proc-macro-error2 (already 800k downloads) but this change did not land on syn_derive. This causes crates depending on borsh-rs transtively depending on syn_derive and thus proc-macro-error. Also it is generally thought as a good practice to remove external dependencies especially if they are not gaining us much to begin with.

Related to https://rustsec.org/advisories/RUSTSEC-2024-0370.html

@kayagokalp kayagokalp force-pushed the kayagokalp/remove_syn_derive_dep branch from 0519d88 to 4a8bdbf Compare November 5, 2024 05:46
@dzmitry-lahoda
Copy link
Contributor

So if syn_derive will migrate to proc-macro-error2 there would be no problem in borsh-rs even in case of syn_derive usage?

@Kyuuhachi
Copy link

I just pushed an update to switch syn_derive to proc-macro-error2, but yeah I have to agree that it seems pretty silly to use syn_derive for such a small part.

@kayagokalp
Copy link
Contributor Author

kayagokalp commented Nov 7, 2024

So if syn_derive will migrate to proc-macro-error2 there would be no problem in borsh-rs even in case of syn_derive usage?

@dzmitry-lahoda yes that is correct.

As per next steps is your call, my original intention opening this was basically:

  1. syn_derive is nearly not used in the repo, just removes 21 lines of code while adding a dependency tree which contained an outdated dependency (proc-macro-error).
  2. I was under the impression that @Kyuuhachi was not active and deprecated the repo. But I left hem a bump comment (Replace proc-macro-error with proc-macro-error2 Kyuuhachi/syn_derive#5 (comment)) and they updated the version.

So I think you can either go to 0.2.0 or remove the dependency completely. If I were you I would be leaning towards removing the dependency if there are no other places you can impact with the added syn_derive in your code base.

@dj8yfo
Copy link
Collaborator

dj8yfo commented Nov 7, 2024

@kayagokalp this removes 2 dependencies from dependency tree,
proc-macro-error(2) and proc-macro-error-attr(2)

collapse

135d134
<  "syn_derive",
587,607d585
< name = "proc-macro-error-attr2"
< version = "2.0.0"
< source = "registry+https://github.com/rust-lang/crates.io-index"
< checksum = "96de42df36bb9bba5542fe9f1a054b8cc87e172759a1868aa05c1f3acc89dfc5"
< dependencies = [
<  "proc-macro2",
<  "quote",
< ]
< 
< [[package]]
< name = "proc-macro-error2"
< version = "2.0.1"
< source = "registry+https://github.com/rust-lang/crates.io-index"
< checksum = "11ec05c52be0a07b08061f7dd003e7d7092e0472bc731b4af7bb1ef876109802"
< dependencies = [
<  "proc-macro-error-attr2",
<  "proc-macro2",
<  "quote",
< ]
< 
< [[package]]
900,911d877
< ]
< 
< [[package]]
< name = "syn_derive"
< version = "0.2.0"
< source = "registry+https://github.com/rust-lang/crates.io-index"
< checksum = "cdb066a04799e45f5d582e8fc6ec8e6d6896040d00898eb4e6a835196815b219"
< dependencies = [
<  "proc-macro-error2",
<  "proc-macro2",
<  "quote",
<  "syn 2.0.87",

syn_derive would've been worth exactly the 21 lines it replaces, but i

didn't see any improvement in error location output in the variant when syn_derive is bumped,
despite proc-macro-error(2) and proc-macro-error-attr(2) being used.
At least for this simple use case.

@dj8yfo
Copy link
Collaborator

dj8yfo commented Nov 7, 2024

@race-of-sloths score 5

@race-of-sloths
Copy link

@dj8yfo Thank you for calling!

@kayagokalp Thank you for the contribution! Join Race of Sloths by simply mentioning me in your comment/PRs description and start collecting Sloth Points through contributions to open source projects.

What is the Race of Sloths

Race of Sloths is a friendly competition where you can participate in challenges and compete with other open-source contributors within your normal workflow

For contributors:

  • Tag @race-of-sloths inside your pull requests
  • Wait for the maintainer to review and score your pull request
  • Check out your position in the Leaderboard
  • Keep weekly and monthly streaks to reach higher positions
  • Boast your contributions with a dynamic picture of your Profile

For maintainers:

  • Score pull requests that participate in the Race of Sloths and receive a reward
  • Engage contributors with fair scoring and fast responses so they keep their streaks
  • Promote the Race to the point where the Race starts promoting you
  • Grow the community of your contributors

Feel free to check our website for additional details!

Bot commands
  • For contributors
    • Include a PR: @race-of-sloths include to enter the Race with your PR
  • For maintainers:
    • Invite contributor @race-of-sloths invite to invite the contributor to participate in a race or include it, if it's already a runner.
    • Assign points: @race-of-sloths score [1/2/3/5/8/13] to award points based on your assessment.
    • Reject this PR: @race-of-sloths exclude to send this PR back to the drawing board.
    • Exclude repo: @race-of-sloths pause to stop bot activity in this repo until @race-of-sloths unpause command is called

@dj8yfo dj8yfo merged commit cef1258 into near:master Nov 7, 2024
7 checks passed
@frol frol mentioned this pull request Oct 30, 2024
@kayagokalp kayagokalp deleted the kayagokalp/remove_syn_derive_dep branch November 7, 2024 22:09
@dj8yfo
Copy link
Collaborator

dj8yfo commented Nov 7, 2024

@Kyuuhachi oh, well, probably proc-macro-error2 is for reporting errors when syn_derive macros are being derived, and not for the ones, which happen during derived syn::parse::Parse parsing. That got me confused for a moment

@kayagokalp
Copy link
Contributor Author

syn_derive would've been worth exactly the 21 lines it replaces, but i

@dj8yfo I would still argue that is not the case. You have ~580 crates depending on borsh whereases there are 7 crates depending on syn_derive. By removing this dependency you are most likely removing the syn_derive entry from >573 crates' Cargo.lock which is very nice outcome. This numbers are considering only the number of crates published, the real numbers are probably more than this.

I guess this is the effect of being in a hot-path so it makes even more sense to be careful in these type of repos.

On top of this added level of indirection while managing your dependencies is something else and in my opinion should also be taken into account while accessing whether to depend on something or implement it yourself.

Thanks for the quick fix, and release!

@dj8yfo
Copy link
Collaborator

dj8yfo commented Nov 8, 2024

@kayagokalp only 2 mentioned ones (proc-macro-error2... )aren't depended on otherwise. Other 5 are included as dependencies anyway. it would've been worth even the small addition of 21 lines, if it didn't introduce any additional dependencies besides itself (syn_derive). cargo should be put to work for automating tasks and not programming everything by hand.

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.

5 participants