Support Default Field Values, backwards compatibly#1955
Support Default Field Values, backwards compatibly#1955DJMcNab wants to merge 1 commit intodtolnay:masterfrom
Conversation
This is exposed as a version of the structures with a `WithDefault` suffix, as the simpler solution has been ruled out. Co-Authored-By: =?UTF-8?q?Esteban=20K=C3=BCber?= <esteban@kuber.com.ar>
c50906d to
218d003
Compare
|
Suggestion: Rather than having unique names for these v2 types, they could exist in a separate module and retain their name. The primary motivation behind this is that it will be easier for people to migrate to supporting default field values, with a simple change: -use syn::Variants;
+use syn::with_default::Variants;And they won't have to do much else besides fixing the unhandled fields. Once syn v3 comes, they can switch back: -use syn::with_default::Variants;
+use syn::Variants;Imagine if instead the type names were different. You'd first have to rename every type, then you'd have to rename them back Rename table
|
|
It's plausible, and it does have appealing properties. I don't particularly want to spend much more time working on this without David's input, however. |
|
Gentle ping @dtolnay, in case you can quickly comment on the above open question even if you don't have the time to perform a full review. |
dtolnay
left a comment
There was a problem hiding this comment.
the parsing for
Field(and thereforeFieldsNamed,Fields,DeriveInput, etc.) will now ignore any default value provided (i.e. not give an error, but also not reflect it in the AST).
I feel that this is misconceived. This is going to result in attributes silently erasing default values written in the source code. For example the following program would print 1 0 instead of 1 1. Discovering occurrences of this in a large codebase is going to be difficult, and making sense of why their default value is not working is going to be difficult for users. I would expect instead an error explaining that code is being parsed by a macro that does not support default field values, with guidance what change to make to the macro.
#[derive(Default)]
struct Outer {
x: u8 = 1,
}
#[tracing::instrument]
fn main() {
#[derive(Default)]
struct Inner {
x: u8 = 1,
}
println!("{}", Outer::default().x);
println!("{}", Inner::default().x);
}|
Regarding #1955 (comment), taking a step back, it is not evident to me that unblocking rust-lang/rust#132162 requires these proposed data structures (regardless of name) being added into syn v2. If macros are going to be required to make code changes to become compatible with default field values, and then reverse those changes when updating to syn v3, then the transitory implementation might as well be served by a different crate. What is the subset of this PR that necessarily has to go into syn? |
As noted in the PR description, I agree that this is the worst part of this PR, and I'm happy to change this back. In a prior commit, I did have an error for this case: Lines 609 to 615 in 50da7cf
I absolutely considered this possibility, but the reason it hasn't been otherwise publicly discussed is as such:
In theory, the changes in the Hopefully I'm missing something which makes this more tractable... |
At the very least, we'd need syn to ignore but successfully parse default field values. That would enable using them for the pure-Rust benefits of the feature, while still gating the "leverage the syntax for proc-macros that want to specify a default without an attribute". |
This PR incorporates work from @estebank in #1851. See also rust-lang/rust#132162 (comment) for context; this is the PR for "solution 2", to unblock the language feature.
Fixes #1774
#![feature(default_field_values)]By running
cargo-semver-checks --all-features(v0.45.0), I have confirmed that this is not a breaking change.This PR has two related changes:
FieldWithDefault, which is a version ofField, with the addition of the optionaldefaultfield. This is then used to create a parallel hierarchy of all Syn types, with the same naming pattern. That is, it also createsFieldsNamedWithDefault,FieldsWithDefault,VariantsWithDefault,DataStructWithDefault, etc.Field, to allow for as much shared code as possible. There is one subtle change here; the parsing forField(and thereforeFieldsNamed,Fields,DeriveInput, etc.) will now ignore any default value provided (i.e. not give an error, but also not reflect it in the AST). This enables a silent upgrade of most derive macros to support their same codegen as if the default field values didn't exist, which will be extremely valuable in the ecosystem.This is explicitly not #1911; instead, that issue would likely be updated to mean "revert this PR, and land #1851 as part of v3.x".
The new structs in this PR is not behind a feature flag. This is because the current infrastructure in
syn-internal-codegenonly supports "any" style feature flags, whereas this would need to be handled as "all" style feature flags.I'm not going to make the changes to the codegen infrastructure myself. I would move these types behind feature flags if:
The silent ignoring of the default values would definitely be reasonable to object to. I think it's the least bad option on an ecosystem level, but I'm happy to change it (either some form of logging, or just throwing an error).
The parallel hierarchy is not complete. That is, we do not support structs with default field values inside blocks (i.e. inside functions/block expressions/etc.). That's because it requires changing so much more code. (But obviously if they exist, they'll be silently ignored).
A reasonable alternative would be to implement #1870 for those nested fields.