Clarify stability guarantee for lifetimes in enum discriminants by mkrasnitski · Pull Request #104299 · rust-lang/rust (original) (raw)

Conversation

This file contains hidden or 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

[ Show hidden characters]({{ revealButtonHref }})

mkrasnitski

Since std::mem::Discriminant erases lifetimes, it should be clarified that changing the concrete value of a lifetime parameter does not change the value of an enum discriminant for a given variant. This is useful as it guarantees that it is safe to transmute Discriminant<Foo<'a>> to Discriminant<Foo<'b>> for any combination of 'a and 'b. This also holds for type-generics as long as the type parameters do not change, e.g. Discriminant<Foo<T, 'a>> can be transmuted to Discriminant<Foo<T, 'b>>.

Side note: Is what I've written actually enough to imply soundness (or rather codify it), or should it specifically be spelled out that it's OK to transmute in the above way?

@rustbot

r? @thomcc

(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-libs

Relevant to the library team, which will review and decide on the PR/issue.

labels

Nov 11, 2022

@rustbot

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

@CraftSpider

I'm not libs or anything, but I'd think this is already implied to be okay by the decision on #101520 - lifetimes are guaranteed to never change a type's layout. Not against documenting it, but just thought of that PR when I saw this one.

@thomcc

I think I agree here, but don't feel particularly qualified to make a decision about documentation like this. Lets see, does the new bot allow this?

r? @rust-lang/lang

@mkrasnitski

@CraftSpider That PR you linked simply allows the compiler to accept such transmutes in all cases (previously it wouldn't work if generic type parameters were present), but it says nothing about their soundness. If a type has some data tied to its lifetime parameter, there are surely times when transmuting the lifetime is unsound (by changing when the data gets dropped). But types like Discriminant<Foo<'a>> and PhantomData<'a> do not carry any data tied to their lifetime parameter, so transmuting the value of that parameter will always be sound, as far as I understand. Maybe the scope of this documentation change should be expanded to encompass all such types.

@mkrasnitski

Bumping and re-requesting review as it's been several months with no activity, and also the currently assigned reviewer is no longer t-lang.

r? @rust-lang/lang

@tmandry tmandry added T-lang

Relevant to the language team, which will review and decide on the PR/issue.

and removed T-libs

Relevant to the library team, which will review and decide on the PR/issue.

labels

Mar 2, 2023

@tmandry

@rfcbot fcp merge

This PR documents that the discriminants of enums that are generic over a lifetime do not change when only the concrete value of the lifetime changes.

It does look related to the decision in #101520, but that was about transmutes, and this is about the value of enum discriminants.

I don't see how we could sensibly do anything different here, so I think we should go ahead and document this.

@rfcbot

Team member @tmandry has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

cc @rust-lang/lang-advisors: FCP proposed for lang, please feel free to register concerns.
See this document for info about what commands tagged team members can give me.

@scottmcm

I agree we can't do anything different here -- any attempt would presumably hit the same lifetime soundness issue that's blocking arbitrary specialization -- so it makes sense to guarantee it.

@rfcbot reviewed

That said, I wonder if we could be more direct about "will not change" in the docs somehow, since "the discriminant" has a couple different possible meanings (variant index, encoded discriminant, declared discriminant, mem::Discriminant).

Quick attempt:

The value of a mem::Discriminant<T> is independent of any lifetimes in T. As such, reading or writing a Discriminant<Foo<'a>> as a Discriminant<Foo<'b>> (whether via transmute or otherwise) is sound. (Note that this is not true for other kinds of generic parameters; Discriminant<Foo<A>> and Discriminant<Foo<B>> might be incompatible.)

@mkrasnitski

@mkrasnitski

@scottmcm Thank you for the suggestion - your wording is much clearer than what I'd originally written! I've adapted it and pushed a new commit.

@nikomatsakis

@rfcbot reviewed

We are pretty committed to lifetime erasure :)

@rfcbot

🔔 This is now entering its final comment period, as per the review above. 🔔

@rfcbot

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.

@mkrasnitski

Anything blocking this from being merged? @tmandry

@mkrasnitski

Bumping - @tmandry, would appreciate a merge :)

@oli-obk

@bors r+ rollup

FCP finished and the wording got adjusted as requested

@bors

📌 Commit 571b0fe has been approved by oli-obk

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors

Status: Waiting on bors to run and complete tests. Bors will change the label on completion.

and removed S-waiting-on-review

Status: Awaiting review from the assignee but also interested parties.

labels

Sep 8, 2023

bors added a commit to rust-lang-ci/rust that referenced this pull request

Sep 8, 2023

@bors

…llaumeGomez

Rollup of 6 pull requests

Successful merges:

r? @ghost @rustbot modify labels: rollup

rust-timer added a commit to rust-lang-ci/rust that referenced this pull request

Sep 8, 2023

@rust-timer

Rollup merge of rust-lang#104299 - mkrasnitski:discriminant-transmute-docs, r=oli-obk

Clarify stability guarantee for lifetimes in enum discriminants

Since std::mem::Discriminant erases lifetimes, it should be clarified that changing the concrete value of a lifetime parameter does not change the value of an enum discriminant for a given variant. This is useful as it guarantees that it is safe to transmute Discriminant<Foo<'a>> to Discriminant<Foo<'b>> for any combination of 'a and 'b. This also holds for type-generics as long as the type parameters do not change, e.g. Discriminant<Foo<T, 'a>> can be transmuted to Discriminant<Foo<T, 'b>>.

Side note: Is what I've written actually enough to imply soundness (or rather codify it), or should it specifically be spelled out that it's OK to transmute in the above way?

lcnr

@@ -1118,6 +1118,11 @@ impl fmt::Debug for Discriminant {
///
/// [Reference]: ../../reference/items/enumerations.html#custom-discriminant-values-for-fieldless-enumerations
///
/// The value of a [`Discriminant`] is independent of any *lifetimes* in `T`. As such, reading

Choose a reason for hiding this comment

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

/// The value of a [`Discriminant`] is independent of any *lifetimes* in `T`. As such, reading
/// The value of a [`Discriminant`] is independent of any **free** *lifetimes* in `T`. As such, reading

It can be unsound to transmute Discrimant<MyType<for<'a, 'b> fn(Inv<'a>, Inv<'b>)>> to Discrimant<MyType<for<'a> fn(Inv<'a>, Inv<'a>)>>.

Choose a reason for hiding this comment

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

(as a general procedural thought, I feel like this decision should have been at least co-owned by t-types)

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

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

My apologies about not looping in t-types. That team didn't exist when I first opened this PR. However, I agree they should have been added while this PR was still under review.

netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this pull request

Jan 8, 2024

@he32

Pkgsrc changes:

Upstream changes:

Version 1.74.1 (2023-12-07)

Version 1.74.0 (2023-11-16)

Language

Compiler

Libraries

Stabilized APIs

These APIs are now stable in const contexts:

Cargo

Rustdoc

Compatibility Notes

Internal Changes

These changes do not affect any public interfaces of Rust, but they represent significant improvements to the performance or internals of rustc and related tools.

None this cycle.

Labels

disposition-merge

This issue / PR is in PFCP or FCP with a disposition to merge it.

finished-final-comment-period

The final comment period is finished for this PR / Issue.

S-waiting-on-bors

Status: Waiting on bors to run and complete tests. Bors will change the label on completion.

T-lang

Relevant to the language team, which will review and decide on the PR/issue.