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

Expects the syn crate to be imported by the user as the name syn #202

Closed
shepmaster opened this issue Oct 17, 2022 · 7 comments · Fixed by #206
Closed

Expects the syn crate to be imported by the user as the name syn #202

shepmaster opened this issue Oct 17, 2022 · 7 comments · Fixed by #206

Comments

@shepmaster
Copy link

While creating the reproduction for #201, I originally did not add syn as my own dependency:

[package]
name = "repro"
version = "0.1.0"
edition = "2021"

# See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html

[dependencies]
darling = "0.14.1"

This causes the error:

error[E0433]: failed to resolve: could not find `syn` in the list of imported crates
 --> src/main.rs:1:10
  |
1 | #[derive(darling::FromVariant)]
  |          ^^^^^^^^^^^^^^^^^^^^ could not find `syn` in the list of imported crates
  |
  = note: this error originates in the derive macro `darling::FromVariant` (in Nightly builds, run with -Z macro-backtrace for more info)

Ideally, darling would not expect that the user has a crate named syn in scope. A better idea is to re-export syn from the darling crate and then you can use that path without worry.

A related problem is if darling itself is renamed. SNAFU uses the crate_root attribute to allow the end user to specify what the snafu crate should actually be called.

@TedDriggs
Copy link
Owner

Ideally, darling would not expect that the user has a crate named syn in scope. A better idea is to re-export syn from the darling crate and then you can use that path without worry.

Wouldn't this pin darling to the exact version of syn that it exports? That seems much worse than requiring callers to bring in syn themselves, but I'm not an expert on how dependency resolution in Rust would work if I'm reexporting a crate from darling.

A related problem is if darling itself is renamed. SNAFU uses the crate_root attribute to allow the end user to specify what the snafu crate should actually be called.

I don't see this changing. I don't relish the idea of touching every single file in darling_core::codegen to first thread this option through into every struct, and then updating every ToTokens impl to use the passed-in crate root over the hardcoded darling.

@shepmaster
Copy link
Author

Wouldn't this pin darling to the exact version of syn that it exports?

I'm not sure I understand you fully, but the answer should be "no". Darling requires syn 1.0.91 at the moment and syn is 1.0.102. Cargo will resolve to some version>= 1.0.91, < 2. Then whatever you export will be that version. There's nothing special about reexporting here; where do you see a "pin" occurring?

If there's ever a syn 2.x (unlikely, from what I understand), then the current setup will fail for anyone who does cargo add darling syn which would grab an incompatible version of syn.

I don't see this changing

🤷 your crate; I've made you aware of it at least.

@seritools
Copy link

but I'm not an expert on how dependency resolution in Rust would work if I'm reexporting a crate from darling.

Re-exporting doesn't change how dependency resolution works, so no worries there. Any semver-compatible version can be chosen, just as if the user added syn themself. Re-exporting is not like vendoring it in, it just exposes a crate under an additional path.

I don't see this changing. I don't relish the idea of touching every single file in darling_core::codegen to first thread this option through into every struct, and then updating every ToTokens impl to use the passed-in crate root over the hardcoded darling.

I guess just to ask for clarity for potential contributors: Do you oppose the idea itself or would you be fine with someone else (who might need it) to do it?

@TedDriggs
Copy link
Owner

Re-exporting is not like vendoring it in, it just exposes a crate under an additional path.

That's what I was worried about, so thanks for clarifying. Given that, I'd be fine with a PR that made generated code use ::darling::export::syn in lieu of ::syn (though I'm not sure how I'd enforce the use of the exported path on an ongoing basis, which is unfortunate).

Do you oppose the idea itself or would you be fine with someone else (who might need it) to do it?

I'd like to see the impact on part of the codebase before making a final decision; for example, having to thread a crate root into darling_core::codeine::DefaultExpression seems like it's going to have some unpleasant side-effects on the readability of the code.

I'd also be curious to understand better the situation in which the name darling isn't available, particularly within a proc-macro crate. There may be something I'm missing that would cause me to reassess the benefit of adding this complexity to the generated code.

@shepmaster
Copy link
Author

shepmaster commented Oct 18, 2022

understand better the situation in which the name darling isn't available

The primary reason is to allow people to rename dependencies. A key reason I've needed such a feature is when there are two semver-incompatible versions of a dependency that I need. The playground shows an example of that

how I'd enforce the use of the exported path on an ongoing basis, which is unfortunate

The same way you protect against people creating types also named Option or Some or None — with a bunch of tests built up after bug reports. At some point it's self-sustaining — you just always use the long path when writing macro code.

@TedDriggs
Copy link
Owner

A key reason I've needed such a feature is when there are two semver-incompatible versions of a dependency that I need.

I’m surprised that this has been an issue in practice, given darling’s fairly-specific use-case.

At some point it's self-sustaining — you just always use the long path when writing macro code.

I’ll be curious to see the impact such a change has on code readability in darling_core::codegen, and ease-of-first-contribution.

@TedDriggs
Copy link
Owner

I've just done this in colin-kiegel/rust-derive-builder#275.

  • There was an unexpected problem with error spans; moving from a literal token to an interpolated variable moved several errors to point at the macro call-site. darling is going to need good test coverage to avoid spanning regressions with this change.
  • There was a mildly detrimental effect on code readability; the structs for codegen all gained extra fields. I will need to look more closely at the darling_core code to see how bad the impact of this will be.

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 a pull request may close this issue.

3 participants