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

clap_derive can parse arbitrary Rust code, which is not always necessary and costs compile time #5657

Open
2 tasks done
hanna-kruppe opened this issue Aug 10, 2024 · 3 comments
Labels
A-derive Area: #[derive]` macro API C-enhancement Category: Raise on the bar on expectations M-breaking-change Meta: Implementing or merging this will introduce a breaking change. S-waiting-on-decision Status: Waiting on a go/no-go before implementing

Comments

@hanna-kruppe
Copy link

Please complete the following tasks

Clap Version

4.5.15

Describe your use case

I have a workspace where I care a little too much about build times (e.g., for iterating on cargo profile settings / RUSTFLAGS) and use a handful of syn-based custom derives, including clap's. Unlike most other derives I've seen (and unlike all others I'm using in this workspace), clap_derive enables syn's "full" feature that enables parsing arbitrary expressions, items, etc. instead of just the parts that are usually needed for derives. This has negative effects for compilation time (cc #2037):

  1. It makes syn take longer to compile (by ca. one second on my machine), which due to feature unification holds back everything that depends on syn. In my specific case, this only has negligible impact (ca. 0.2s wall clock time for clean cargo test, out of ca. 10s total) because there happens to be enough independent work for the number of CPUs I have and another critical path in the schedule. It matters more in smaller workspaces (or with more CPUs, presumably), e.g., for the git-derive example by itself, disabling syn/full reduces clean dev builds from 5s to 4.3s on my machine.
  2. When different workspace members want different sets of features from syn, this means syn and everything it depends on may get rebuilt multiple times while switching between workspace members (i.e., unlike the previous point this also affects some incremental builds). This is the problem that the workspace-hack pattern addresses. But even with the wonderful cargo-hakari, it takes some manual effort to set up and maintain a workspace hack, so many projects don't do that. (I currently do it in the workspace in question, but it provides less value nowadays than when I first set it up, so I've considered dropping it.)

Describe the solution you'd like

Ideally, the syn/full feature could just be removed completely (cc #4784). clap_derive does build when the feature is removed (some care is needed while testing this because several dev-dependencies in clap's workspace enable it anyway), but some uses of the derives break because syn can't parse some kinds of expressions without the "full" feature. I don't have a full list of what's affected, but at least it causes an error for arg(num_args = n..=m) as used e.g., in the git-derive example. From my understanding of how clap_derive works, I think it's always possible to work around this by extracting those expressions into separate constants, but that's still annoying and a breaking change.

On the other hand, many simpler expressions (literals, identifiers, paths, binary operators, taking references, etc.) don't seem to be affected. Case in point, my aforementioned workspace works just fine if test a patched version of clap_derive minus syn/full via [patch.crates-io]. So instead I propose a new feature, enabled by default, flag that controls whether syn/full is enabled. Those who care to fiddle around with feature flags and find that disabling this one doesn't break their project can do so, while everyone else is unaffected.

Alternatives, if applicable

In addition to or instead of a new feature flag, the breaking change of not enabling syn/full (by default or ever) could be rolled into clap v5. But I don't seriously want to propose this without fully understanding the consequences. Are there any clap features that would become annoying or even impossible to use via the derive API without syn/full? For example, is there an ergonomic alternative for num_args = n..=m?

Additional Context

No response

@hanna-kruppe hanna-kruppe added the C-enhancement Category: Raise on the bar on expectations label Aug 10, 2024
@epage epage added M-breaking-change Meta: Implementing or merging this will introduce a breaking change. A-derive Area: #[derive]` macro API S-waiting-on-decision Status: Waiting on a go/no-go before implementing labels Aug 12, 2024
@epage
Copy link
Member

epage commented Aug 12, 2024

Note that anything we do here would be a breaking change.

For myself, I feel like feature flags are a crude tool to work with and try as much as possible to reduce their use. From what you've described, I'm not sure there is enough benefit for this to outweigh that cost.

If we do anything with this, know of any good documentation on the difference between parsing and full with syn? The docs don't make it too clear what all is lost or what workarounds are available. I'm loath to use the serde` approach of having attribute values be strings.

@hanna-kruppe
Copy link
Author

I haven't found much documentation on the difference when I looked into this, but there are clues. The doc comment for syn::Expr says "most of the variants are not available unless “full” is enabled" and each variant just wraps an ExprWhatever struct that is annotated (on docs.rs) with what features it requires. Grouped by features and stripped of the common Expr prefix:

  • Requires "full": Array, Assign, Async, Await, Block, Break, Closure, Const, Continue, ForLoop, Group, If, Infer, Let, Loop, Match, Range, Repeat, Return, Try, TryBlock, Tuple, Unsafe, While, Yield
  • Requires "full" or "derive": Binary, Call, Cast, Field, Index, Lit, Macro, MethodCall, Paren, Path, Reference, Struct, Unary

Looking at this list, ExprRange and possibly ExprArray ([a, b, c]) seem to be the only "full"-exclusive ones that are likely to be commonly useful for clap (but I don't know the full API surface area by heart). On the other hand I'm surprised how many things are already enabled by "derive" alone (struct literals, method calls, macros, etc.). It seems that quite a few expressions kinds were moved out of full and into derive in the past year (e.g., struct literals in dtolnay/syn@3b947f3). I wonder why the line was drawn exactly there, and why this has changed over time. If syn can already parse that many different expressions without "full", perhaps the missing expression variants that clap needs could also be added with little cost?

@epage
Copy link
Member

epage commented Aug 12, 2024

Tuple is another likely case.

Closure is questionable. It can be convenient but anything larger than a single value should likely be a separate function.

If someone is willing to see if syn would move these to derive, I would be open to exploring this change during 5.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-derive Area: #[derive]` macro API C-enhancement Category: Raise on the bar on expectations M-breaking-change Meta: Implementing or merging this will introduce a breaking change. S-waiting-on-decision Status: Waiting on a go/no-go before implementing
Projects
None yet
Development

No branches or pull requests

2 participants