Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Destructuring Drop ADTs via ManuallyDrop #3466

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

Jules-Bertholet
Copy link
Contributor

@Jules-Bertholet Jules-Bertholet commented Jul 30, 2023

Rendered

Pre-RFC discussion on Internals

@rustbot label A-drop

Rendered

@rustbot rustbot added the A-drop Proposals relating to the Drop trait or drop semantics label Jul 30, 2023
@Jules-Bertholet Jules-Bertholet force-pushed the manuallydrop-deref-move branch from edf3e52 to c86637f Compare July 30, 2023 22:03
@Jules-Bertholet Jules-Bertholet force-pushed the manuallydrop-deref-move branch from c86637f to 1888553 Compare July 30, 2023 22:04
`_` doesn't trigger a move.
@ehuss ehuss added the T-lang Relevant to the language team, which will review and decide on the RFC. label Jul 31, 2023
}
```

If code like the above example actually exists in the ecosystem, this proposal would make it unsound.
Copy link
Member

@fee1-dead fee1-dead Aug 6, 2023

Choose a reason for hiding this comment

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

I actually do have an unpublished crate that relies on fields not being able to be moved out if I implement Drop. In my crate a type SomeStruct has a field with type FieldReader. The latter type is a ZST that implements Deref based on the address of &FieldReader (in this case, it reads other fields from SomeStruct), that relies on the fact that if I make SomeStruct implement Drop, and construction of FieldReader unsafe, then all pointers to FieldReader are guaranteed to exist only within SomeStruct, since moving out of SomeStruct isn't allowed.

It would be nice if there is a way to spell "can't move out of any fields of this struct" that does not involve a no-op drop implementation, or maybe there is a way already and I just don't know about it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What I would expect/was expecting is that in practice, this guarantee would be provided via privacy. Your FieldReader would be a private field with a public get()/get_mut() accessor.

(Also, your use-case might be impacted by subobject provenance discussions, you might want to share details at rust-lang/unsafe-code-guidelines#256)

Copy link
Member

Choose a reason for hiding this comment

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

What I'm hoping to achieve with FieldReader is that I want it to be a public field, while emulating getters through Deref, so using accessor methods defeats its point entirely.

Copy link

Choose a reason for hiding this comment

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

This conversation reminds me of the debate over the soundness of partial-borrow vs replace_with. In partial-borrow I also (ab)use a language inflexibility to allow the use of fields rather than accessors.

(Providing fields rather than accessors is more than just a simple ergonomic improvement to elide some (). There are important borrowck features that are available only via borrowck's special handling of fields.)

So it seems both @fee1-dead's crate and partial-borrow would benefit from some way to get access to those "field-specific" borrowck behaviours. In the meantime this is another case where not only (i) adding a new language feature can make existing code unsound (ii) that existing code is not theoretical and cannot be readily expressed a different way.

Copy link
Member

Choose a reason for hiding this comment

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

(Providing fields rather than accessors is more than just a simple ergonomic improvement to elide some (). There are important borrowck features that are available only via borrowck's special handling of fields.)

Yes. I forgot to mention this in my comment, but this is one big reason I'm using a FieldReader design, to allow having mutable references for two fields.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@fee1-dead would your FieldReader also be unsound in the presence of replace_with alone? (Take &mut to a FieldReader, use replace_with to move out of it, then take a shared reference to the moved-out value)

@Jules-Bertholet
Copy link
Contributor Author

I've expanded the "Rationale and Alternatives" section in light of new discussion on Internals.

@Jules-Bertholet Jules-Bertholet changed the title Move out of deref for ManuallyDrop Destructuring Drop ADTs via ManuallyDrop Jul 10, 2024
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
A-drop Proposals relating to the Drop trait or drop semantics T-lang Relevant to the language team, which will review and decide on the RFC.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants