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

Clarify stability guarantee for lifetimes in enum discriminants #104299

Merged
merged 1 commit into from
Sep 8, 2023

Conversation

mkrasnitski
Copy link
Contributor

@mkrasnitski mkrasnitski commented Nov 11, 2022

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
Copy link
Collaborator

rustbot commented Nov 11, 2022

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
Copy link
Collaborator

rustbot commented Nov 11, 2022

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:

  • Stabilizing library features
  • Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
  • Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
  • Changing public documentation in ways that create new stability guarantees
  • Changing observable runtime behavior of library APIs

@CraftSpider
Copy link
Contributor

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
Copy link
Member

thomcc commented Nov 12, 2022

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

@rustbot rustbot assigned cramertj and unassigned thomcc Nov 12, 2022
@mkrasnitski
Copy link
Contributor Author

mkrasnitski commented Nov 14, 2022

@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
Copy link
Contributor Author

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

@rustbot rustbot assigned tmandry and unassigned cramertj Feb 13, 2023
@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
Copy link
Member

tmandry commented Mar 2, 2023

@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
Copy link

rfcbot commented Mar 2, 2023

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.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Mar 2, 2023
@scottmcm
Copy link
Member

scottmcm commented Mar 7, 2023

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
Copy link
Contributor Author

@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
Copy link
Contributor

@rfcbot reviewed

We are pretty committed to lifetime erasure :)

@rfcbot rfcbot added final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. and removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. labels Mar 14, 2023
@rfcbot
Copy link

rfcbot commented Mar 14, 2023

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

@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this PR / Issue. to-announce Announce this issue on triage meeting and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Mar 24, 2023
@rfcbot
Copy link

rfcbot commented Mar 24, 2023

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.

@apiraino apiraino removed the to-announce Announce this issue on triage meeting label Mar 30, 2023
@mkrasnitski
Copy link
Contributor Author

Anything blocking this from being merged? @tmandry

@mkrasnitski
Copy link
Contributor Author

Bumping - @tmandry, would appreciate a merge :)

@oli-obk
Copy link
Contributor

oli-obk commented Sep 8, 2023

@bors r+ rollup

FCP finished and the wording got adjusted as requested

@bors
Copy link
Contributor

bors commented Sep 8, 2023

📌 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
…llaumeGomez

Rollup of 6 pull requests

Successful merges:

 - rust-lang#104299 (Clarify stability guarantee for lifetimes in enum discriminants)
 - rust-lang#115088 (Fix Step Skipping Caused by Using the `--exclude` Option)
 - rust-lang#115201 (rustdoc: list matching impls on type aliases)
 - rust-lang#115633 (Lint node for `PRIVATE_BOUNDS`/`PRIVATE_INTERFACES` is the item which names the private type)
 - rust-lang#115638 (`-Cllvm-args` usability improvement)
 - rust-lang#115643 (fix: return early when has tainted in mir-lint)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 2cceedd into rust-lang:master Sep 8, 2023
@rustbot rustbot added this to the 1.74.0 milestone Sep 8, 2023
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Sep 8, 2023
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?
@mkrasnitski mkrasnitski deleted the discriminant-transmute-docs branch November 9, 2023 18:22
@@ -1118,6 +1118,11 @@ impl<T> fmt::Debug for Discriminant<T> {
///
/// [Reference]: ../../reference/items/enumerations.html#custom-discriminant-values-for-fieldless-enumerations
///
/// The value of a [`Discriminant<T>`] is independent of any *lifetimes* in `T`. As such, reading
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// The value of a [`Discriminant<T>`] is independent of any *lifetimes* in `T`. As such, reading
/// The value of a [`Discriminant<T>`] 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>)>>.

Copy link
Contributor

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)

Copy link
Contributor

Choose a reason for hiding this comment

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

opened #118006

Copy link
Contributor Author

@mkrasnitski mkrasnitski Nov 17, 2023

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
Pkgsrc changes:

 * Remove NetBSD-8 support (embedded LLVm requires newer C++
   than what is in -8; it's conceivable that this could still
   build with an external LLVM)
 * undo powerpc 9.0 file naming tweak, since we no longer support -8.
 * Remove patch to LLVM for powerpc now included by upstream.
 * Minor adjustments, checksum changes etc.


Upstream changes:

Version 1.74.1 (2023-12-07)
===========================

- [Resolved spurious STATUS_ACCESS_VIOLATIONs in LLVM]
  (rust-lang/rust#118464)
- [Clarify guarantees for std::mem::discriminant]
  (rust-lang/rust#118006)
- [Fix some subtyping-related regressions]
  (rust-lang/rust#116415)

Version 1.74.0 (2023-11-16)
==========================

Language
--------

- [Codify that `std::mem::Discriminant<T>` does not depend on any
  lifetimes in T]
  (rust-lang/rust#104299)
- [Replace `private_in_public` lint with `private_interfaces` and
  `private_bounds` per RFC 2145]
  (rust-lang/rust#113126)
  Read more in
  [RFC 2145](https://rust-lang.github.io/rfcs/2145-type-privacy.html).
- [Allow explicit `#[repr(Rust)]`]
  (rust-lang/rust#114201)
- [closure field capturing: don't depend on alignment of packed fields]
  (rust-lang/rust#115315)
- [Enable MIR-based drop-tracking for `async` blocks]
  (rust-lang/rust#107421)

Compiler
--------

- [stabilize combining +bundle and +whole-archive link modifiers]
  (rust-lang/rust#113301)
- [Stabilize `PATH` option for `--print KIND=PATH`]
  (rust-lang/rust#114183)
- [Enable ASAN/LSAN/TSAN for `*-apple-ios-macabi`]
  (rust-lang/rust#115644)
- [Promote loongarch64-unknown-none* to Tier 2]
  (rust-lang/rust#115368)
- [Add `i686-pc-windows-gnullvm` as a tier 3 target]
  (rust-lang/rust#115687)

Libraries
---------

- [Implement `From<OwnedFd/Handle>` for ChildStdin/out/err]
  (rust-lang/rust#98704)
- [Implement `From<{&,&mut} [T; N]>` for `Vec<T>` where `T: Clone`]
  (rust-lang/rust#111278)
- [impl Step for IP addresses]
  (rust-lang/rust#113748)
- [Implement `From<[T; N]>` for `Rc<[T]>` and `Arc<[T]>`]
  (rust-lang/rust#114041)
- [`impl TryFrom<char> for u16`]
  (rust-lang/rust#114065)
- [Stabilize `io_error_other` feature]
  (rust-lang/rust#115453)
- [Stabilize the `Saturating` type]
  (rust-lang/rust#115477)
- [Stabilize const_transmute_copy]
  (rust-lang/rust#115520)

Stabilized APIs
---------------

- [`core::num::Saturating`]
  (https://doc.rust-lang.org/stable/std/num/struct.Saturating.html)
- [`impl From<io::Stdout> for std::process::Stdio`]
  (https://doc.rust-lang.org/stable/std/process/struct.Stdio.html#impl-From%3CStdout%3E-for-Stdio)
- [`impl From<io::Stderr> for std::process::Stdio`]
  (https://doc.rust-lang.org/stable/std/process/struct.Stdio.html#impl-From%3CStderr%3E-for-Stdio)
- [`impl From<OwnedHandle> for std::process::Child{Stdin, Stdout, Stderr}`]
  (https://doc.rust-lang.org/stable/std/process/struct.Stdio.html#impl-From%3CStderr%3E-for-Stdio)
- [`impl From<OwnedFd> for std::process::Child{Stdin, Stdout, Stderr}`]
  (https://doc.rust-lang.org/stable/std/process/struct.Stdio.html#impl-From%3CStderr%3E-for-Stdio)
- [`std::ffi::OsString::from_encoded_bytes_unchecked`]
  (https://doc.rust-lang.org/stable/std/ffi/struct.OsString.html#method.from_encoded_bytes_unchecked)
- [`std::ffi::OsString::into_encoded_bytes`]
  (https://doc.rust-lang.org/stable/std/ffi/struct.OsString.html#method.into_encoded_bytes)
- [`std::ffi::OsStr::from_encoded_bytes_unchecked`]
  (https://doc.rust-lang.org/stable/std/ffi/struct.OsStr.html#method.from_encoded_bytes_unchecked)
- [`std::ffi::OsStr::as_encoded_bytes`]
  (https://doc.rust-lang.org/stable/std/ffi/struct.OsStr.html#method.as_encoded_bytes)
- [`std::io::Error::other`]
  (https://doc.rust-lang.org/stable/std/io/struct.Error.html#method.other)
- [`impl TryFrom<char> for u16`]
  (https://doc.rust-lang.org/stable/std/primitive.u16.html#impl-TryFrom%3Cchar%3E-for-u16)
- [`impl<T: Clone, const N: usize> From<&[T; N]> for Vec<T>`]
  (https://doc.rust-lang.org/stable/std/vec/struct.Vec.html#impl-From%3C%26%5BT;+N%5D%3E-for-Vec%3CT,+Global%3E)
- [`impl<T: Clone, const N: usize> From<&mut [T; N]> for Vec<T>`]
  (https://doc.rust-lang.org/stable/std/vec/struct.Vec.html#impl-From%3C%26mut+%5BT;+N%5D%3E-for-Vec%3CT,+Global%3E)
- [`impl<T, const N: usize> From<[T; N]> for Arc<[T]>`]
  (https://doc.rust-lang.org/stable/std/sync/struct.Arc.html#impl-From%3C%5BT;+N%5D%3E-for-Arc%3C%5BT%5D,+Global%3E)
- [`impl<T, const N: usize> From<[T; N]> for Rc<[T]>`]
  (https://doc.rust-lang.org/stable/std/rc/struct.Rc.html#impl-From%3C%5BT;+N%5D%3E-for-Rc%3C%5BT%5D,+Global%3E)

These APIs are now stable in const contexts:

- [`core::mem::transmute_copy`]
  (https://doc.rust-lang.org/beta/std/mem/fn.transmute_copy.html)
- [`str::is_ascii`]
  (https://doc.rust-lang.org/beta/std/primitive.str.html#method.is_ascii)
- [`[u8]::is_ascii`]
  (https://doc.rust-lang.org/beta/std/primitive.slice.html#method.is_ascii)

Cargo
-----

- [fix: Set MSRV for internal packages]
  (rust-lang/cargo#12381)
- [config: merge lists in precedence order]
  (rust-lang/cargo#12515)
- [fix(update): Clarify meaning of --aggressive as --recursive]
  (rust-lang/cargo#12544)
- [fix(update): Make `-p` more convenient by being positional]
  (rust-lang/cargo#12545)
- [feat(help): Add styling to help output ]
  (rust-lang/cargo#12578)
- [feat(pkgid): Allow incomplete versions when unambigious]
  (rust-lang/cargo#12614)
- [feat: stabilize credential-process and registry-auth]
  (rust-lang/cargo#12649)
- [feat(cli): Add '-n' to dry-run]
  (rust-lang/cargo#12660)
- [Add support for `target.'cfg(..)'.linker`]
  (rust-lang/cargo#12535)
- [Stabilize `--keep-going`]
  (rust-lang/cargo#12568)
- [feat: Stabilize lints]
  (rust-lang/cargo#12648)

Rustdoc
-------

- [Add warning block support in rustdoc]
  (rust-lang/rust#106561)
- [Accept additional user-defined syntax classes in fenced code blocks]
  (rust-lang/rust#110800)
- [rustdoc-search: add support for type parameters]
  (rust-lang/rust#112725)
- [rustdoc: show inner enum and struct in type definition for concrete type]
  (rust-lang/rust#114855)

Compatibility Notes
-------------------

- [Raise minimum supported Apple OS versions]
  (rust-lang/rust#104385)
- [make Cell::swap panic if the Cells partially overlap]
  (rust-lang/rust#114795)
- [Reject invalid crate names in `--extern`]
  (rust-lang/rust#116001)
- [Don't resolve generic impls that may be shadowed by dyn built-in impls]
  (rust-lang/rust#114941)

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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.