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

Bugfix/double reference #354

Closed
wants to merge 2 commits into from
Closed

Conversation

yfcai
Copy link

@yfcai yfcai commented May 8, 2024

Resolves #328

Part of #328

Synopsis

This is an alternative solution to the double reference problem reported in #328.

Solution

Instead of dereferencing macro-generated bindings such as _0 at the use sites, do the the following:

  1. In binding generation, generate let _0 = self.0 instead of let _0 = &self.0 in case the field self.0 is already a reference type.

  2. In body generation, add & before #ident in the call derive_more::core::fmt::#trait_ident::fmt(#ident, __derive_more_f), because the function fmt dereferences.

  3. In bounds generation, strip & in the bounded type for traits other than Pointer because the fmt function of those traits dereferences exhaustively.

Open questions

  1. All previous test function names are assert. The tests added in this PR have distinct names so that they can be executed alone via cargo test <substring-of-test-function-name>. Is there a reason for the naming convention assert? If so I'd rename the new tests.

  2. Are the improvements to tests in Fix incorrect fmt::Pointer implementations #328 important? I can incorporate them if needed.

  3. Are there any breaking changes? I'm not 100% sure.

Checklist

  • Documentation is updated (if required)
  • Tests are added/updated (if required)
  • CHANGELOG entry is added (if required)

@@ -94,8 +96,14 @@ fn expand_struct(
.ident
.clone()
.map_or_else(|| syn::Member::Unnamed(i.into()), syn::Member::Named);
quote! {
let #var = &self.#member;
if let syn::Type::Reference(_) = f.ty {
Copy link
Owner

Choose a reason for hiding this comment

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

Afaict this only works for references, and will fail as soon as you use other pointer types, such as Box<T>.

Copy link
Author

Choose a reason for hiding this comment

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

You are right, I will think about it more.

@yfcai yfcai marked this pull request as draft May 8, 2024 10:56
@yfcai
Copy link
Author

yfcai commented May 15, 2024

The approach in this PR does not handle non-reference types that implement Pointer.

Another approach is defining a newtype for bindings _0, _1 etc that transparently delegate Pointer and other Display traits to the underlying type. While implementing Deref makes the newtype a transparent reference for method and function calls, it does not work for operators including user implementations of traits in std::ops.

In conclusion, the approach in #328 is better.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants