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

Make pointer field of Pin private #119615

Closed
wants to merge 5 commits into from
Closed

Conversation

joboet
Copy link
Member

@joboet joboet commented Jan 5, 2024

This PR is an alternative to #119562.

By using the macro hygiene of macros 2.0 inside the pin macro, the pointer field of Pin does no longer need to be public. Hiding it avoids the issue outlined in #119562, against which a regression test is added.

As the fix-me comment suggested, use the macro hygiene of macros 2.0 to access the field in the `pin` macro. Therefore, the field can be made private.
@rustbot
Copy link
Collaborator

rustbot commented Jan 5, 2024

r? @Mark-Simulacrum

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Jan 5, 2024
@rust-log-analyzer

This comment has been minimized.

@Noratrieb
Copy link
Member

Back when this macro was added, @danielhenrymantilla did originally want to use a macro 2.0, but if I remember it correctly, it was decided that relying on such a macro was not good and that it should use a macro_rules with a hack instead.
Given that we can always go back to a hacked macro_rules (by renaming the field), I don't immediately see the problem, but it would be good to revisit that as an explicit decision.

@mohe2015
Copy link
Contributor

mohe2015 commented Jan 5, 2024

What does cargo expand (or the rust equivalent) do now? Because I remember that it expanded to code with the pointer field.

library/core/tests/pin.rs Outdated Show resolved Hide resolved
@@ -1246,5 +1237,5 @@ pub macro pin($value:expr $(,)?) {
//
// See https://doc.rust-lang.org/1.58.1/reference/destructors.html#temporary-lifetime-extension
// for more info.
$crate::pin::Pin::<&mut _> { pointer: &mut { $value } }
Pin { pointer: &mut { $value } }
Copy link
Contributor

@danielhenrymantilla danielhenrymantilla Jan 5, 2024

Choose a reason for hiding this comment

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

Removal of turbofish type is orthogonal to field privacy / hygiene, and technically allows for code such as let p: Pin<*mut _> = pin!(...); (letting a coërcion happen on the pointer: ... assignment).
Since this whole thing is already quite tricky/subtle, let's not risk unnecessary flexibility (which appears to be unsound ⚠️ ):

Suggested change
Pin { pointer: &mut { $value } }
Pin::<&mut _> { pointer: &mut { $value } }

Copy link
Member Author

Choose a reason for hiding this comment

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

I've readded this and added a comment explaining why it should not be removed.

Copy link
Member Author

Choose a reason for hiding this comment

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

I wonder if I should also add a test for this case in this PR, so that others cannot stumble over the same thing.

@fmease
Copy link
Member

fmease commented Jan 5, 2024

cc @LegionMammal978

@fmease
Copy link
Member

fmease commented Jan 5, 2024

What does cargo expand (or the rust equivalent) do now?

Well, it's going to show pointer. However, cargo expand / rustc -Zunpretty=expanded doesn't come with any stability guarantees.

Co-authored-by: Daniel Henry-Mantilla <daniel.henry.mantilla@gmail.com>
@mohe2015
Copy link
Contributor

mohe2015 commented Jan 5, 2024

What does cargo expand (or the rust equivalent) do now?

Well, it's going to show pointer. However, cargo expand / rustc -Zunpretty=expanded doesn't come with any stability guarantees.

I sometimes compile the expanded code to get better source line information etc. and to my understanding I can't easily make it compile anymore then by adding the internal feature flag? Not a too big issue but just came to my mind.

@Mark-Simulacrum
Copy link
Member

r? @Nilstrieb

Perhaps you remember which team / group made the decision referenced here:

Given that we can always go back to a hacked macro_rules (by renaming the field), I don't immediately see the problem, but it would be good to revisit that as an explicit decision.

@rustbot rustbot assigned Noratrieb and unassigned Mark-Simulacrum Jan 6, 2024
@Noratrieb
Copy link
Member

Noratrieb commented Jan 6, 2024

I went through the discussion and it looks like this was a libs-api team decision in a meeting: #93176 (comment)
The status of macro 2.0 hasn't change in the meantime. What has changed is that we now have a concrete case of this hack going wrong in the issue this PR tries to fix, where the field name leaks through deref.
With this knowledge, is it worth revisiting the original choice of using macro 2.0, or is it deemed "good enough" to just give the pointer field a "more obscure" name like __pointer or something even more obscure?
With that question, r? @m-ou-se (or @rust-lang/libs-api in general)

@rustbot rustbot assigned m-ou-se and unassigned Noratrieb Jan 6, 2024
@bors
Copy link
Contributor

bors commented Jan 8, 2024

☔ The latest upstream changes (presumably #119722) made this pull request unmergeable. Please resolve the merge conflicts.

@LegionMammal978
Copy link
Contributor

LegionMammal978 commented Jan 9, 2024

I noticed today that there's also a comment in compiler/rustc_hir_typeck/src/fn_ctxt/suggestions.rs:

                        // Skip suggestions for unstable public fields (for example `Pin::pointer`)

The example should probably be omitted. Apart from Pin::pointer, there don't appear to be any other unstable public fields in the standard library.

@m-ou-se
Copy link
Member

m-ou-se commented Jan 10, 2024

I don't think we're supposed to use full macro 2.0 hygiene in a public macro yet. cc @petrochenkov

@m-ou-se
Copy link
Member

m-ou-se commented Feb 15, 2024

Considering #119562 is merged, macros 2.0 hygiene isn't ready yet, and the temporary lifetime rfc work progressing towards a solution for the pin!() macro without public Pin fields, I'm closing this.

@m-ou-se m-ou-se closed this Feb 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.