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.

Sign up for GitHub

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

Jump to bottom

Simplify dropck remark on PhantomData #125550

Closed
wants to merge 1 commit into from
Closed

Simplify dropck remark on PhantomData #125550

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions library/core/src/marker.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -724,9 +724,9 @@ impl<T: ?Sized> !Sync for *mut T {}
///
/// The exact interaction of `PhantomData` with drop check **may change in the future**.
///
/// Currently, adding a field of type `PhantomData<T>` indicates that your type *owns* data of type
/// `T` in very rare circumstances. This in turn has effects on the Rust compiler's [drop check]
/// analysis. For the exact rules, see the [drop check] documentation.
/// Currently, adding a field of type `PhantomData<T>` indicates the type *owns* data of type `T`.
/// In very rare circumstances, this has effects on the Rust compiler's [drop check] analysis.
Copy link
Member

@Noratrieb Noratrieb May 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

It still mentions the rare circumstances, so I don't think this fixes the issue. I don't like that sentence either. It should either be dropped, or explicitly mention those rare circumstances ("using the unstable may_dangle feature").

As long as we mention rare circumstances, we leave the reader puzzled what those circumstances may be.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm. My understanding is that the "rare circumstances" ambiguity comes about because the teams are hedging so they are allowed to change it.

My draft seems more acceptable to me than the status quo because it's unambiguous as to whether and when you should use PhantomData, it just is sly about whether it has any given effect.

Copy link
Contributor

Choose a reason for hiding this comment

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

cc #103413 (comment)

"Currently, adding a field of type PhantomData<T> indicates that your type owns data of type T in very rare circumstances" is intentional. Adding PhantomData<T> to your type only impacts whether your type 'owns' T in rare circumstances: if your type already has another field which requires drop:

struct DoesNotOwnT<T> {
   field: *const T,
   phantom_data: PhantomData<T>,
}
struct DoesOwnT<T> {
   field: *const T,
   other_field: String,
   phantom_data: PhantomData<T>,
}

Copy link
Contributor

@lcnr lcnr May 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I've opened an RFC to fix this behavior rust-lang/rfcs#3417, but I currently don't have the capacity to push this over the finish line. See the examples in that RFC for more details

Copy link
Member Author

Choose a reason for hiding this comment

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

oic, thank you for explaining!

hmm... the definition provided here still feels abstruse, but I understand why you phrased it that way now.

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah 🤔 I think for this to make sense you'd have to actually go into the difference between needs_drop and dropck_outlives (PhantomData is only considered for dropck_outlives, as there shouldn't be any types for which needs_drop and Copy both hold)

Ideally the way we make this more understandable is by just merging rust-lang/rfcs#3417, though I guess we can also go more in-depth in the comment if you think that's more achievable.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, I currently do not have an idea of how to proceed!

/// For the exact rules, see the [drop check] documentation.
///
/// ## Layout
///
Expand Down
Loading