Don't use double ref vars in Debug derive #380
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
tyranron
approved these changes
Jul 4, 2024
3 tasks
tyranron
added a commit
that referenced
this pull request
Jul 31, 2024
Resolves #328 Requires #377 Requires #380 ## Synopsis `Debug` and `Display` derives allow referring fields via short syntax (`_0` for unnamed fields and `name` for named fields): ```rust #[derive(Display)] #[display("{_0:o}")] struct OctalInt(i32); ``` The way this works is by introducing a local binding in the macro expansion: ```rust let _0 = &self.0; ``` This, however, introduces double pointer indirection. For most of the `fmt` traits, this is totally OK. However, the `fmt::Pointer` is sensitive to that: ```rust #[derive(Display)] #[display("--> {_0:p}")] struct Int(&'static i32); // expands to impl fmt::Display for Int { fn fmt(&self, f: fmt::Formatter<'_>) -> fmt::Result { let _0 = &self.0; // has `&&i32` type, not `&i32` write!(f, "--> {_0:p}") // so, prints address of the `_0` local binding, // not the one of the `self.0` field as we would expect } } ``` ## Solution Pass all local bindings also as named parameters and dereference them there. This allows `"{_0:p}"` to work as expected. Positional arguments and expressions still have the previous behaviour. This seems okay IMHO, as we can explain that in expressions these local bindings are references and that you need to dereference when needed, such as for `Pointer`. A downside of the current implementation is that users cannot use the names of our named parameters as names for their own named parameters, because we already use them. With some additional code this is fixable, but it doesn't seem important enough to fix. People can simply use a different name when creating their own named parameters, which is a good idea anyway because it will be less confusing to any reader of the code. If it turns out to be important to support this after all, we can still start to support it in a backwards compatible way (because now it causes a compilation failure). Co-authored-by: Kai Ren <tyranron@gmail.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Related to #289
Related to #328
Synopsis
While looking into #328 I realized the current situation around Pointer derives
and references was even weirder because we store a double-reference to the
fields in the local variables for Debug, but not for Display. The reason we
were doing this was because of #289.
Solution
This stops storing a double-reference, and only adds the additional reference
in the places where its needed.