Skip to content

allow value field names#127

Open
robamu wants to merge 1 commit intomainfrom
allow-value-field-names-with-builder
Open

allow value field names#127
robamu wants to merge 1 commit intomainfrom
allow-value-field-names-with-builder

Conversation

@robamu
Copy link
Collaborator

@robamu robamu commented Feb 25, 2026

Fixes #126

@robamu robamu requested a review from danlehmann February 25, 2026 13:56
@robamu
Copy link
Collaborator Author

robamu commented Feb 25, 2026

damn.. this is problematic with reserved identifiers..

@robamu robamu force-pushed the allow-value-field-names-with-builder branch from 7805dcc to cee0924 Compare February 25, 2026 14:10
@robamu robamu requested review from dgarrett and estebank February 25, 2026 14:11
Comment on lines +463 to +468
// Special handling for reserved identifiers prefix.
let mut field_name = def.field_name.to_string();
if field_name.starts_with("r#") {
field_name = field_name.strip_prefix("r#").unwrap().to_string();
}
syn::parse_str::<Ident>(&format!("__{}", field_name.to_uppercase())).unwrap()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this duplicated logic be moved to a separate function and called here and in 556?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

probably, yes..

|
= note: the method was found for
- `PartialTestAllowed<upper, false, c, b, false>`
- `PartialTestAllowed<__UPPER, false, __C, __B, false>`
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The original approach was explicitly to try and keep the output in the diagnostics as close to the user's code as possible, so changing their casing is unfortunate. We must address the issue with clashing with field names, of course, but we could handle that by exclusively changing value and nothing else, right? And at that point we'd have to check that the transformation target (lets say, VALUE) doesn't already exist as another const param...

Copy link
Collaborator Author

@robamu robamu Mar 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that would be a solution, but I think the diagnostics are still sufficient to know what the issue is. I can still do the special case solution. alternatively: what do you think about renaming the value field itself to __value or __private_value?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@estebank could you have a look at #131

@robamu
Copy link
Collaborator Author

robamu commented Mar 13, 2026

I now provided an alternative PR in #131 which renames the generated field names to __value instead

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.

Field name value in combination with the builder API is problematic with v2

2 participants