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

Use T's discriminant type in mem::Discriminant<T> instead of u64. #70705

Merged
merged 8 commits into from
May 21, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions src/doc/unstable-book/src/language-features/lang-items.md
Original file line number Diff line number Diff line change
Expand Up @@ -287,6 +287,7 @@ the source code.
- `unsize`: `libcore/marker.rs`
- `sync`: `libcore/marker.rs`
- `phantom_data`: `libcore/marker.rs`
- `discriminant_kind`: `libcore/marker.rs`
- `freeze`: `libcore/marker.rs`
- `debug_trait`: `libcore/fmt/mod.rs`
- `non_zero`: `libcore/nonzero.rs`
Expand Down
6 changes: 6 additions & 0 deletions src/libcore/intrinsics.rs
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,8 @@
)]
#![allow(missing_docs)]

#[cfg(not(bootstrap))]
use crate::marker::DiscriminantKind;
use crate::mem;

#[stable(feature = "drop_in_place", since = "1.8.0")]
Expand Down Expand Up @@ -1912,6 +1914,10 @@ extern "rust-intrinsic" {
/// The stabilized version of this intrinsic is
/// [`std::mem::discriminant`](../../std/mem/fn.discriminant.html)
#[rustc_const_unstable(feature = "const_discriminant", issue = "69821")]
#[cfg(not(bootstrap))]
pub fn discriminant_value<T>(v: &T) -> <T as DiscriminantKind>::Discriminant;
#[rustc_const_unstable(feature = "const_discriminant", issue = "69821")]
#[cfg(bootstrap)]
pub fn discriminant_value<T>(v: &T) -> u64;

/// Rust's "try catch" construct which invokes the function pointer `try_fn`
Expand Down
32 changes: 32 additions & 0 deletions src/libcore/marker.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@

use crate::cell::UnsafeCell;
use crate::cmp;
use crate::fmt::Debug;
use crate::hash::Hash;
use crate::hash::Hasher;

Expand Down Expand Up @@ -679,6 +680,37 @@ mod impls {
unsafe impl<T: Send + ?Sized> Send for &mut T {}
}

/// Compiler-internal trait used to indicate the type of enum discriminants.
///
/// This trait is automatically implemented for every type and does not add any
/// guarantees to [`mem::Discriminant`]. It is **undefined behavior** to transmute
/// between `DiscriminantKind::Discriminant` and `mem::Discriminant`.
///
/// [`mem::Discriminant`]: https://doc.rust-lang.org/stable/core/mem/struct.Discriminant.html
#[unstable(
feature = "discriminant_kind",
issue = "none",
reason = "this trait is unlikely to ever be stabilized, use `mem::discriminant` instead"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
reason = "this trait is unlikely to ever be stabilized, use `mem::discriminant` instead"
reason = "this trait is unlikely to ever be stabilized, use `mem::Discriminant` instead"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I intentionally used mem::discriminant here, as I meant the method, not the type. Don't care either way though 🤷

Copy link
Member

Choose a reason for hiding this comment

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

I thought you meant the type, as this trait is used to get the type of the discriminant.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmm. This message is not optimal 🤔

Would something like this trait is unlikely to ever be stabilized, try using `mem::discriminant` directly be better?

)]
#[cfg_attr(not(bootstrap), lang = "discriminant_kind")]
pub trait DiscriminantKind {
/// The type of the dicriminant, which must satisfy the trait
/// bounds required by `mem::Discriminant`.
type Discriminant: Clone + Copy + Debug + Eq + PartialEq + Hash + Send + Sync + Unpin;
Copy link
Member

Choose a reason for hiding this comment

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

Might be interesting to have TryInto<i128> + TryInto<u128> as well, but it's not too relevant unless we really want to let people use this.

Copy link
Member

Choose a reason for hiding this comment

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

The fact that we have to add Unpin here makes me worried that we'll have to add more marker traits in the future... would that be backwards-compatible?

Copy link
Contributor Author

@lcnr lcnr Apr 13, 2020

Choose a reason for hiding this comment

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

I guess. DiscriminantKind is both unstable and cannot be implemented outside of libcore

}

// Manually implement `DiscriminantKind` for all types during bootstrap
// to reduce the required amount of conditional compilation.
#[unstable(
feature = "discriminant_kind",
issue = "none",
reason = "this trait is unlikely to ever be stabilized, use `mem::discriminant` instead"
)]
#[cfg(bootstrap)]
impl<T: ?Sized> DiscriminantKind for T {
type Discriminant = u64;
}

/// Compiler-internal trait used to determine whether a type contains
/// any `UnsafeCell` internally, but not through an indirection.
/// This affects, for example, whether a `static` of that type is
Expand Down
6 changes: 3 additions & 3 deletions src/libcore/mem/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ use crate::cmp;
use crate::fmt;
use crate::hash;
use crate::intrinsics;
use crate::marker::{Copy, PhantomData, Sized};
use crate::marker::{Copy, DiscriminantKind, Sized};
use crate::ptr;

mod manually_drop;
Expand Down Expand Up @@ -930,7 +930,7 @@ pub unsafe fn transmute_copy<T, U>(src: &T) -> U {
///
/// [`discriminant`]: fn.discriminant.html
#[stable(feature = "discriminant_value", since = "1.21.0")]
pub struct Discriminant<T>(u64, PhantomData<fn() -> T>);
pub struct Discriminant<T>(<T as DiscriminantKind>::Discriminant);
Copy link
Member

Choose a reason for hiding this comment

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

Uh, oh, cc @rust-lang/libs, this changes variance. PhantomData<fn() -> T> is covariant wrt T but projections are invariant (any "added for future use" parameters should probably start invariant, so we don't break code later).

That means this PR will require a crater run.

Copy link
Member

Choose a reason for hiding this comment

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

We could also do the variance change and crater run in a separate PR first.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

opened #71814


// N.B. These trait implementations cannot be derived because we don't want any bounds on T.

Expand Down Expand Up @@ -995,5 +995,5 @@ impl<T> fmt::Debug for Discriminant<T> {
#[stable(feature = "discriminant_value", since = "1.21.0")]
#[rustc_const_unstable(feature = "const_discriminant", issue = "69821")]
pub const fn discriminant<T>(v: &T) -> Discriminant<T> {
Discriminant(intrinsics::discriminant_value(v), PhantomData)
Discriminant(intrinsics::discriminant_value(v))
}
80 changes: 10 additions & 70 deletions src/librustc_builtin_macros/deriving/generic/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -187,7 +187,6 @@ use rustc_ast::ptr::P;
use rustc_attr as attr;
use rustc_data_structures::map_in_place::MapInPlace;
use rustc_expand::base::{Annotatable, ExtCtxt};
use rustc_session::parse::ParseSess;
use rustc_span::source_map::respan;
use rustc_span::symbol::{kw, sym, Ident, Symbol};
use rustc_span::Span;
Expand Down Expand Up @@ -437,14 +436,7 @@ impl<'a> TraitDef<'a> {
// This can only cause further compilation errors
// downstream in blatantly illegal code, so it
// is fine.
self.expand_enum_def(
cx,
enum_def,
&item.attrs,
item.ident,
generics,
from_scratch,
)
self.expand_enum_def(cx, enum_def, item.ident, generics, from_scratch)
}
ast::ItemKind::Union(ref struct_def, ref generics) => {
if self.supports_unions {
Expand Down Expand Up @@ -769,7 +761,6 @@ impl<'a> TraitDef<'a> {
&self,
cx: &mut ExtCtxt<'_>,
enum_def: &'a EnumDef,
type_attrs: &[ast::Attribute],
type_ident: Ident,
generics: &Generics,
from_scratch: bool,
Expand Down Expand Up @@ -801,7 +792,6 @@ impl<'a> TraitDef<'a> {
cx,
self,
enum_def,
type_attrs,
type_ident,
self_args,
&nonself_args[..],
Expand All @@ -816,38 +806,6 @@ impl<'a> TraitDef<'a> {
}
}

fn find_repr_type_name(sess: &ParseSess, type_attrs: &[ast::Attribute]) -> &'static str {
let mut repr_type_name = "isize";
for a in type_attrs {
for r in &attr::find_repr_attrs(sess, a) {
repr_type_name = match *r {
attr::ReprPacked(_)
| attr::ReprSimd
| attr::ReprAlign(_)
| attr::ReprTransparent
| attr::ReprNoNiche => continue,

attr::ReprC => "i32",

attr::ReprInt(attr::SignedInt(ast::IntTy::Isize)) => "isize",
attr::ReprInt(attr::SignedInt(ast::IntTy::I8)) => "i8",
attr::ReprInt(attr::SignedInt(ast::IntTy::I16)) => "i16",
attr::ReprInt(attr::SignedInt(ast::IntTy::I32)) => "i32",
attr::ReprInt(attr::SignedInt(ast::IntTy::I64)) => "i64",
attr::ReprInt(attr::SignedInt(ast::IntTy::I128)) => "i128",

attr::ReprInt(attr::UnsignedInt(ast::UintTy::Usize)) => "usize",
attr::ReprInt(attr::UnsignedInt(ast::UintTy::U8)) => "u8",
attr::ReprInt(attr::UnsignedInt(ast::UintTy::U16)) => "u16",
attr::ReprInt(attr::UnsignedInt(ast::UintTy::U32)) => "u32",
attr::ReprInt(attr::UnsignedInt(ast::UintTy::U64)) => "u64",
attr::ReprInt(attr::UnsignedInt(ast::UintTy::U128)) => "u128",
}
}
}
repr_type_name
}

impl<'a> MethodDef<'a> {
fn call_substructure_method(
&self,
Expand Down Expand Up @@ -1148,20 +1106,11 @@ impl<'a> MethodDef<'a> {
cx: &mut ExtCtxt<'_>,
trait_: &TraitDef<'b>,
enum_def: &'b EnumDef,
type_attrs: &[ast::Attribute],
type_ident: Ident,
self_args: Vec<P<Expr>>,
nonself_args: &[P<Expr>],
) -> P<Expr> {
self.build_enum_match_tuple(
cx,
trait_,
enum_def,
type_attrs,
type_ident,
self_args,
nonself_args,
)
self.build_enum_match_tuple(cx, trait_, enum_def, type_ident, self_args, nonself_args)
}

/// Creates a match for a tuple of all `self_args`, where either all
Expand All @@ -1181,11 +1130,11 @@ impl<'a> MethodDef<'a> {

/// ```{.text}
/// let __self0_vi = unsafe {
/// std::intrinsics::discriminant_value(&self) } as i32;
/// std::intrinsics::discriminant_value(&self) };
/// let __self1_vi = unsafe {
/// std::intrinsics::discriminant_value(&arg1) } as i32;
/// std::intrinsics::discriminant_value(&arg1) };
/// let __self2_vi = unsafe {
/// std::intrinsics::discriminant_value(&arg2) } as i32;
/// std::intrinsics::discriminant_value(&arg2) };
///
/// if __self0_vi == __self1_vi && __self0_vi == __self2_vi && ... {
/// match (...) {
Expand All @@ -1204,7 +1153,6 @@ impl<'a> MethodDef<'a> {
cx: &mut ExtCtxt<'_>,
trait_: &TraitDef<'b>,
enum_def: &'b EnumDef,
type_attrs: &[ast::Attribute],
type_ident: Ident,
mut self_args: Vec<P<Expr>>,
nonself_args: &[P<Expr>],
Expand Down Expand Up @@ -1392,39 +1340,31 @@ impl<'a> MethodDef<'a> {
//
if variants.len() > 1 && self_args.len() > 1 {
// Build a series of let statements mapping each self_arg
// to its discriminant value. If this is a C-style enum
// with a specific repr type, then casts the values to
// that type. Otherwise casts to `i32` (the default repr
// type).
// to its discriminant value.
//
// i.e., for `enum E<T> { A, B(1), C(T, T) }`, and a deriving
// with three Self args, builds three statements:
//
// ```
// let __self0_vi = unsafe {
// std::intrinsics::discriminant_value(&self) } as i32;
// std::intrinsics::discriminant_value(&self) };
// let __self1_vi = unsafe {
// std::intrinsics::discriminant_value(&arg1) } as i32;
// std::intrinsics::discriminant_value(&arg1) };
// let __self2_vi = unsafe {
// std::intrinsics::discriminant_value(&arg2) } as i32;
// std::intrinsics::discriminant_value(&arg2) };
// ```
let mut index_let_stmts: Vec<ast::Stmt> = Vec::with_capacity(vi_idents.len() + 1);

// We also build an expression which checks whether all discriminants are equal
// discriminant_test = __self0_vi == __self1_vi && __self0_vi == __self2_vi && ...
let mut discriminant_test = cx.expr_bool(sp, true);

let target_type_name = find_repr_type_name(&cx.parse_sess, type_attrs);

let mut first_ident = None;
for (&ident, self_arg) in vi_idents.iter().zip(&self_args) {
let self_addr = cx.expr_addr_of(sp, self_arg.clone());
let variant_value =
deriving::call_intrinsic(cx, sp, "discriminant_value", vec![self_addr]);

let target_ty = cx.ty_ident(sp, cx.ident_of(target_type_name, sp));
let variant_disr = cx.expr_cast(sp, variant_value, target_ty);
let let_stmt = cx.stmt_let(sp, false, ident, variant_disr);
let let_stmt = cx.stmt_let(sp, false, ident, variant_value);
index_let_stmts.push(let_stmt);

match first_ident {
Expand Down
12 changes: 9 additions & 3 deletions src/librustc_codegen_llvm/intrinsic.rs
Original file line number Diff line number Diff line change
Expand Up @@ -188,11 +188,11 @@ impl IntrinsicCallMethods<'tcx> for Builder<'a, 'll, 'tcx> {
}
"size_of" | "pref_align_of" | "min_align_of" | "needs_drop" | "type_id"
| "type_name" => {
let ty_name = self
let value = self
.tcx
.const_eval_instance(ty::ParamEnv::reveal_all(), instance, None)
.unwrap();
OperandRef::from_const(self, ty_name, ret_ty).immediate_or_packed_pair(self)
OperandRef::from_const(self, value, ret_ty).immediate_or_packed_pair(self)
}
// Effectively no-op
"forget" => {
Expand Down Expand Up @@ -549,7 +549,13 @@ impl IntrinsicCallMethods<'tcx> for Builder<'a, 'll, 'tcx> {
}
}

"discriminant_value" => args[0].deref(self.cx()).codegen_get_discr(self, ret_ty),
"discriminant_value" => {
if ret_ty.is_integral() {
args[0].deref(self.cx()).codegen_get_discr(self, ret_ty)
} else {
span_bug!(span, "Invalid discriminant type for `{:?}`", arg_tys[0])
}
}

name if name.starts_with("simd_") => {
match generic_simd_intrinsic(self, name, callee_ty, args, ret_ty, llret_ty, span) {
Expand Down
1 change: 1 addition & 0 deletions src/librustc_hir/lang_items.rs
Original file line number Diff line number Diff line change
Expand Up @@ -163,6 +163,7 @@ language_item_table! {
CopyTraitLangItem, "copy", copy_trait, Target::Trait;
CloneTraitLangItem, "clone", clone_trait, Target::Trait;
SyncTraitLangItem, "sync", sync_trait, Target::Trait;
DiscriminantKindTraitLangItem,"discriminant_kind", discriminant_kind_trait, Target::Trait;
FreezeTraitLangItem, "freeze", freeze_trait, Target::Trait;

DropTraitLangItem, "drop", drop_trait, Target::Trait;
Expand Down
1 change: 1 addition & 0 deletions src/librustc_middle/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@
#![feature(const_panic)]
#![feature(const_transmute)]
#![feature(core_intrinsics)]
#![feature(discriminant_kind)]
#![feature(drain_filter)]
#![feature(never_type)]
#![feature(exhaustive_patterns)]
Expand Down
12 changes: 12 additions & 0 deletions src/librustc_middle/traits/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -409,6 +409,9 @@ pub enum Vtable<'tcx, N> {
/// Same as above, but for a function pointer type with the given signature.
VtableFnPointer(VtableFnPointerData<'tcx, N>),

/// Vtable for a builtin `DeterminantKind` trait implementation.
VtableDiscriminantKind(VtableDiscriminantKindData),

/// Vtable automatically generated for a generator.
VtableGenerator(VtableGeneratorData<'tcx, N>),

Expand All @@ -427,6 +430,7 @@ impl<'tcx, N> Vtable<'tcx, N> {
VtableGenerator(c) => c.nested,
VtableObject(d) => d.nested,
VtableFnPointer(d) => d.nested,
VtableDiscriminantKind(VtableDiscriminantKindData) => Vec::new(),
VtableTraitAlias(d) => d.nested,
}
}
Expand All @@ -441,6 +445,7 @@ impl<'tcx, N> Vtable<'tcx, N> {
VtableGenerator(c) => &c.nested[..],
VtableObject(d) => &d.nested[..],
VtableFnPointer(d) => &d.nested[..],
VtableDiscriminantKind(VtableDiscriminantKindData) => &[],
VtableTraitAlias(d) => &d.nested[..],
}
}
Expand Down Expand Up @@ -482,6 +487,9 @@ impl<'tcx, N> Vtable<'tcx, N> {
fn_ty: p.fn_ty,
nested: p.nested.into_iter().map(f).collect(),
}),
VtableDiscriminantKind(VtableDiscriminantKindData) => {
VtableDiscriminantKind(VtableDiscriminantKindData)
}
VtableTraitAlias(d) => VtableTraitAlias(VtableTraitAliasData {
alias_def_id: d.alias_def_id,
substs: d.substs,
Expand Down Expand Up @@ -558,6 +566,10 @@ pub struct VtableFnPointerData<'tcx, N> {
pub nested: Vec<N>,
}

// FIXME(@lcnr): This should be refactored and merged with other builtin vtables.
#[derive(Clone, Debug, PartialEq, Eq, RustcEncodable, RustcDecodable, HashStable, TypeFoldable)]
pub struct VtableDiscriminantKindData;

#[derive(Clone, PartialEq, Eq, RustcEncodable, RustcDecodable, HashStable, TypeFoldable)]
pub struct VtableTraitAliasData<'tcx, N> {
pub alias_def_id: DefId,
Expand Down
Loading