Avoid name clashes with field names #15
Conversation
Fixes magiclen#14 completely, so there is now no risk of clashes between our different local variables, even though some of them are constructed from field names.
This is a local variable name, not the field name (since the fields are numbered, their "names" are the indices).
Fixes the analogous bug to magiclen#14 with fields called `state`.
This prevents name clashes with `source`. Because of the way clone_from works, we need to be able to bind both source and destination field names at once. I chose to rename some variables to reflect their purpose, rather than relying on `2` suffixes.
|
Thank you for providing this alternative approach to address the issues raised in #14. I appreciate the thorough explanation of the changes made in this branch. I've reviewed the changes you made, including the reversion of code changes in 983a143, the adjustments to implementations of It seems like you've taken a careful approach to avoid name clashes and have provided a comprehensive test case to demonstrate the effectiveness of the proposed solution. I also appreciate your consideration of potential issues with struct fields and the implementations of Regarding the suggestion to consider derived identifiers for cases involving local variables in struct fields and the mentioned traits, I agree that it's worth exploring to prevent potential future issues. Overall, the changes look promising, and I'd be in favor of merging this alternative approach. |
Hi. This is an alternative approach to #14, which I think fixes the problem definitively.
After 983a143 we still have name clashes if enum fields are things like
v_builder_. That's a slightly odd field name, but I don't think it's reasonable to demand that a user of educe avoid it.In this branch, I:
DebugHashCloneand the comparison traits for enums983a143~, and passes at the tip of the branchTo fix the enum impls I generally:
format_ident!to generate derived names like_FIELD.field_name_realandfield_name_varfor the actual name, and the local variable name (pattern binding), respectively (rather than, sayfield_nameandfield_name2).field_name2because it needs two lots of field names in scope, used twoformat_ident!calls to make two lots offield_name_var_...2suffix)I looked at the code for structs, and as far as I can see it doesn't bind the struct fields to local variable names, so it's not affected by issues analogous to #14. I also looked at the implementations of
Default,Deref[Mut]andInto. These don't have any local variables, so although they use the struct field names directly as local variables, they avoid #14. I think it might be worth considering derived identifiers for these cases too, in case we should ever introduce local variables into the generated code for these - right now doing so would introduce the #14 bug again. But I'm not sure now is the right time to do that.Please let me know what you think.