From 7650afc1cea284956080098d39d670ea0b007ce7 Mon Sep 17 00:00:00 2001 From: Eduard Burtescu Date: Fri, 10 Jun 2016 13:00:21 +0300 Subject: [PATCH] Make transmuting from fn item types to pointer-sized types a hard error. --- src/librustc/diagnostics.rs | 62 +++++++++++++++++++ src/librustc/lint/builtin.rs | 7 --- src/librustc/middle/intrinsicck.rs | 48 +++++++++++--- src/librustc_lint/lib.rs | 6 +- src/librustc_trans/mir/block.rs | 21 +------ .../transmute-from-fn-item-types-error.rs | 49 ++++++++++++++- .../transmute-from-fn-item-types-lint.rs | 49 --------------- .../run-pass/transmute-from-fn-item-types.rs | 27 -------- 8 files changed, 153 insertions(+), 116 deletions(-) delete mode 100644 src/test/compile-fail/transmute-from-fn-item-types-lint.rs delete mode 100644 src/test/run-pass/transmute-from-fn-item-types.rs diff --git a/src/librustc/diagnostics.rs b/src/librustc/diagnostics.rs index b1b1b849437d1..85b4ddcdd7190 100644 --- a/src/librustc/diagnostics.rs +++ b/src/librustc/diagnostics.rs @@ -1725,6 +1725,68 @@ If you want to get command-line arguments, use `std::env::args`. To exit with a specified exit code, use `std::process::exit`. "##, +E0591: r##" +Per [RFC 401][rfc401], if you have a function declaration `foo`: + +```rust,ignore +// For the purposes of this explanation, all of these +// different kinds of `fn` declarations are equivalent: +fn foo(x: i32) { ... } +extern "C" fn foo(x: i32); +impl i32 { fn foo(x: self) { ... } } +``` + +the type of `foo` is **not** `fn(i32)`, as one might expect. +Rather, it is a unique, zero-sized marker type written here as `typeof(foo)`. +However, `typeof(foo)` can be _coerced_ to a function pointer `fn(i32)`, +so you rarely notice this: + +```rust,ignore +let x: fn(i32) = foo; // OK, coerces +``` + +The reason that this matter is that the type `fn(i32)` is not specific to +any particular function: it's a function _pointer_. So calling `x()` results +in a virtual call, whereas `foo()` is statically dispatched, because the type +of `foo` tells us precisely what function is being called. + +As noted above, coercions mean that most code doesn't have to be +concerned with this distinction. However, you can tell the difference +when using **transmute** to convert a fn item into a fn pointer. + +This is sometimes done as part of an FFI: + +```rust,ignore +extern "C" fn foo(userdata: Box) { + ... +} + +let f: extern "C" fn(*mut i32) = transmute(foo); +callback(f); + +``` + +Here, transmute is being used to convert the types of the fn arguments. +This pattern is incorrect because, because the type of `foo` is a function +**item** (`typeof(foo)`), which is zero-sized, and the target type (`fn()`) +is a function pointer, which is not zero-sized. +This pattern should be rewritten. There are a few possible ways to do this: +- change the original fn declaration to match the expected signature, + and do the cast in the fn body (the prefered option) +- cast the fn item fo a fn pointer before calling transmute, as shown here: + - `let f: extern "C" fn(*mut i32) = transmute(foo as extern "C" fn(_))` + - `let f: extern "C" fn(*mut i32) = transmute(foo as usize) /* works too */` + +The same applies to transmutes to `*mut fn()`, which were observedin practice. +Note though that use of this type is generally incorrect. +The intention is typically to describe a function pointer, but just `fn()` +alone suffices for that. `*mut fn()` is a pointer to a fn pointer. +(Since these values are typically just passed to C code, however, this rarely +makes a difference in practice.) + +[rfc401]: https://github.com/rust-lang/rfcs/blob/master/text/0401-coercions.md +"##, + } diff --git a/src/librustc/lint/builtin.rs b/src/librustc/lint/builtin.rs index 70f03e02f46d9..6c2730f2d1a5f 100644 --- a/src/librustc/lint/builtin.rs +++ b/src/librustc/lint/builtin.rs @@ -155,12 +155,6 @@ declare_lint! { "uses of #[derive] with raw pointers are rarely correct" } -declare_lint! { - pub TRANSMUTE_FROM_FN_ITEM_TYPES, - Deny, - "transmute from function item type to pointer-sized type erroneously allowed" -} - declare_lint! { pub HR_LIFETIME_IN_ASSOC_TYPE, Deny, @@ -273,7 +267,6 @@ impl LintPass for HardwiredLints { ILLEGAL_STRUCT_OR_ENUM_CONSTANT_PATTERN, CONST_ERR, RAW_POINTER_DERIVE, - TRANSMUTE_FROM_FN_ITEM_TYPES, OVERLAPPING_INHERENT_IMPLS, RENAMED_AND_REMOVED_LINTS, SUPER_OR_SELF_IN_GLOBAL_PATH, diff --git a/src/librustc/middle/intrinsicck.rs b/src/librustc/middle/intrinsicck.rs index cdbf92e93a4cb..c9722adc9510c 100644 --- a/src/librustc/middle/intrinsicck.rs +++ b/src/librustc/middle/intrinsicck.rs @@ -17,7 +17,6 @@ use ty::{self, Ty, TyCtxt}; use ty::layout::{LayoutError, Pointer, SizeSkeleton}; use syntax::abi::Abi::RustIntrinsic; -use syntax::ast; use syntax_pos::Span; use hir::intravisit::{self, Visitor, NestedVisitorMap}; use hir; @@ -37,6 +36,35 @@ struct ExprVisitor<'a, 'gcx: 'a+'tcx, 'tcx: 'a> { infcx: &'a InferCtxt<'a, 'gcx, 'tcx> } +/// If the type is `Option`, it will return `T`, otherwise +/// the type itself. Works on most `Option`-like types. +fn unpack_option_like<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>, + ty: Ty<'tcx>) + -> Ty<'tcx> { + let (def, substs) = match ty.sty { + ty::TyAdt(def, substs) => (def, substs), + _ => return ty + }; + + if def.variants.len() == 2 && !def.repr.c && def.repr.int.is_none() { + let data_idx; + + if def.variants[0].fields.is_empty() { + data_idx = 1; + } else if def.variants[1].fields.is_empty() { + data_idx = 0; + } else { + return ty; + } + + if def.variants[data_idx].fields.len() == 1 { + return def.variants[data_idx].fields[0].ty(tcx, substs); + } + } + + ty +} + impl<'a, 'gcx, 'tcx> ExprVisitor<'a, 'gcx, 'tcx> { fn def_id_is_transmute(&self, def_id: DefId) -> bool { let intrinsic = match self.infcx.tcx.item_type(def_id).sty { @@ -46,7 +74,7 @@ impl<'a, 'gcx, 'tcx> ExprVisitor<'a, 'gcx, 'tcx> { intrinsic && self.infcx.tcx.item_name(def_id) == "transmute" } - fn check_transmute(&self, span: Span, from: Ty<'gcx>, to: Ty<'gcx>, id: ast::NodeId) { + fn check_transmute(&self, span: Span, from: Ty<'gcx>, to: Ty<'gcx>) { let sk_from = SizeSkeleton::compute(from, self.infcx); let sk_to = SizeSkeleton::compute(to, self.infcx); @@ -56,15 +84,17 @@ impl<'a, 'gcx, 'tcx> ExprVisitor<'a, 'gcx, 'tcx> { return; } + // Special-case transmutting from `typeof(function)` and + // `Option` to present a clearer error. + let from = unpack_option_like(self.infcx.tcx.global_tcx(), from); match (&from.sty, sk_to) { (&ty::TyFnDef(..), SizeSkeleton::Known(size_to)) if size_to == Pointer.size(&self.infcx.tcx.data_layout) => { - // FIXME #19925 Remove this warning after a release cycle. - let msg = format!("`{}` is now zero-sized and has to be cast \ - to a pointer before transmuting to `{}`", - from, to); - self.infcx.tcx.sess.add_lint( - ::lint::builtin::TRANSMUTE_FROM_FN_ITEM_TYPES, id, span, msg); + struct_span_err!(self.infcx.tcx.sess, span, E0591, + "`{}` is zero-sized and can't be transmuted to `{}`", + from, to) + .span_note(span, &format!("cast with `as` to a pointer instead")) + .emit(); return; } _ => {} @@ -140,7 +170,7 @@ impl<'a, 'gcx, 'tcx> Visitor<'gcx> for ExprVisitor<'a, 'gcx, 'tcx> { ty::TyFnDef(.., sig) if sig.abi() == RustIntrinsic => { let from = sig.inputs().skip_binder()[0]; let to = *sig.output().skip_binder(); - self.check_transmute(expr.span, from, to, expr.id); + self.check_transmute(expr.span, from, to); } _ => { span_bug!(expr.span, "transmute wasn't a bare fn?!"); diff --git a/src/librustc_lint/lib.rs b/src/librustc_lint/lib.rs index 8fb1740e66eac..091bc9cfdfaec 100644 --- a/src/librustc_lint/lib.rs +++ b/src/librustc_lint/lib.rs @@ -195,10 +195,6 @@ pub fn register_builtins(store: &mut lint::LintStore, sess: Option<&Session>) { id: LintId::of(SUPER_OR_SELF_IN_GLOBAL_PATH), reference: "issue #36888 ", }, - FutureIncompatibleInfo { - id: LintId::of(TRANSMUTE_FROM_FN_ITEM_TYPES), - reference: "issue #19925 ", - }, FutureIncompatibleInfo { id: LintId::of(OVERLAPPING_INHERENT_IMPLS), reference: "issue #36889 ", @@ -260,4 +256,6 @@ pub fn register_builtins(store: &mut lint::LintStore, sess: Option<&Session>) { store.register_removed("raw_pointer_deriving", "using derive with raw pointers is ok"); store.register_removed("drop_with_repr_extern", "drop flags have been removed"); + store.register_removed("transmute_from_fn_item_types", + "always cast functions before transmuting them"); } diff --git a/src/librustc_trans/mir/block.rs b/src/librustc_trans/mir/block.rs index 3cad2bc1d8423..34d8c6500b926 100644 --- a/src/librustc_trans/mir/block.rs +++ b/src/librustc_trans/mir/block.rs @@ -21,7 +21,7 @@ use builder::Builder; use common::{self, Funclet}; use common::{C_bool, C_str_slice, C_struct, C_u32, C_undef}; use consts; -use machine::{llalign_of_min, llbitsize_of_real}; +use machine::llalign_of_min; use meth; use type_of::{self, align_of}; use glue; @@ -869,24 +869,7 @@ impl<'a, 'tcx> MirContext<'a, 'tcx> { fn trans_transmute_into(&mut self, bcx: &Builder<'a, 'tcx>, src: &mir::Operand<'tcx>, dst: &LvalueRef<'tcx>) { - let mut val = self.trans_operand(bcx, src); - if let ty::TyFnDef(def_id, substs, _) = val.ty.sty { - let llouttype = type_of::type_of(bcx.ccx, dst.ty.to_ty(bcx.tcx())); - let out_type_size = llbitsize_of_real(bcx.ccx, llouttype); - if out_type_size != 0 { - // FIXME #19925 Remove this hack after a release cycle. - let f = Callee::def(bcx.ccx, def_id, substs); - let ty = match f.ty.sty { - ty::TyFnDef(.., f) => bcx.tcx().mk_fn_ptr(f), - _ => f.ty - }; - val = OperandRef { - val: Immediate(f.reify(bcx.ccx)), - ty: ty - }; - } - } - + let val = self.trans_operand(bcx, src); let llty = type_of::type_of(bcx.ccx, val.ty); let cast_ptr = bcx.pointercast(dst.llval, llty.ptr_to()); let in_type = val.ty; diff --git a/src/test/compile-fail/transmute-from-fn-item-types-error.rs b/src/test/compile-fail/transmute-from-fn-item-types-error.rs index 50bcd53ecb82c..c3fe1de895df9 100644 --- a/src/test/compile-fail/transmute-from-fn-item-types-error.rs +++ b/src/test/compile-fail/transmute-from-fn-item-types-error.rs @@ -10,14 +10,61 @@ use std::mem; +unsafe fn foo() -> (isize, *const (), Option) { + let i = mem::transmute(bar); + //~^ ERROR is zero-sized and can't be transmuted + //~^^ NOTE cast with `as` to a pointer instead + + let p = mem::transmute(foo); + //~^ ERROR is zero-sized and can't be transmuted + //~^^ NOTE cast with `as` to a pointer instead + + let of = mem::transmute(main); + //~^ ERROR is zero-sized and can't be transmuted + //~^^ NOTE cast with `as` to a pointer instead + + (i, p, of) +} + unsafe fn bar() { - // Error, still, if the resulting type is not pointer-sized. + // Error as usual if the resulting type is not pointer-sized. mem::transmute::<_, u8>(main); //~^ ERROR transmute called with differently sized types + //~^^ NOTE transmuting between 0 bits and 8 bits + + mem::transmute::<_, *mut ()>(foo); + //~^ ERROR is zero-sized and can't be transmuted + //~^^ NOTE cast with `as` to a pointer instead + + mem::transmute::<_, fn()>(bar); + //~^ ERROR is zero-sized and can't be transmuted + //~^^ NOTE cast with `as` to a pointer instead + + // No error if a coercion would otherwise occur. + mem::transmute::(main); +} + +unsafe fn baz() { + mem::transmute::<_, *mut ()>(Some(foo)); + //~^ ERROR is zero-sized and can't be transmuted + //~^^ NOTE cast with `as` to a pointer instead + + mem::transmute::<_, fn()>(Some(bar)); + //~^ ERROR is zero-sized and can't be transmuted + //~^^ NOTE cast with `as` to a pointer instead + + mem::transmute::<_, Option>(Some(baz)); + //~^ ERROR is zero-sized and can't be transmuted + //~^^ NOTE cast with `as` to a pointer instead + + // No error if a coercion would otherwise occur. + mem::transmute::, usize>(Some(main)); } fn main() { unsafe { + foo(); bar(); + baz(); } } diff --git a/src/test/compile-fail/transmute-from-fn-item-types-lint.rs b/src/test/compile-fail/transmute-from-fn-item-types-lint.rs deleted file mode 100644 index 08e660e878c29..0000000000000 --- a/src/test/compile-fail/transmute-from-fn-item-types-lint.rs +++ /dev/null @@ -1,49 +0,0 @@ -// Copyright 2016 The Rust Project Developers. See the COPYRIGHT -// file at the top-level directory of this distribution and at -// http://rust-lang.org/COPYRIGHT. -// -// Licensed under the Apache License, Version 2.0 or the MIT license -// , at your -// option. This file may not be copied, modified, or distributed -// except according to those terms. - -#![deny(transmute_from_fn_item_types)] - -use std::mem; - -unsafe fn foo() -> (isize, *const (), Option) { - let i = mem::transmute(bar); - //~^ ERROR is now zero-sized and has to be cast to a pointer before transmuting - //~^^ WARNING was previously accepted - - let p = mem::transmute(foo); - //~^ ERROR is now zero-sized and has to be cast to a pointer before transmuting - //~^^ WARNING was previously accepted - - let of = mem::transmute(main); - //~^ ERROR is now zero-sized and has to be cast to a pointer before transmuting - //~^^ WARNING was previously accepted - - (i, p, of) -} - -unsafe fn bar() { - mem::transmute::<_, *mut ()>(foo); - //~^ ERROR is now zero-sized and has to be cast to a pointer before transmuting - //~^^ WARNING was previously accepted - - mem::transmute::<_, fn()>(bar); - //~^ ERROR is now zero-sized and has to be cast to a pointer before transmuting - //~^^ WARNING was previously accepted - - // No error if a coercion would otherwise occur. - mem::transmute::(main); -} - -fn main() { - unsafe { - foo(); - bar(); - } -} diff --git a/src/test/run-pass/transmute-from-fn-item-types.rs b/src/test/run-pass/transmute-from-fn-item-types.rs deleted file mode 100644 index 574a90e2ad697..0000000000000 --- a/src/test/run-pass/transmute-from-fn-item-types.rs +++ /dev/null @@ -1,27 +0,0 @@ -// Copyright 2016 The Rust Project Developers. See the COPYRIGHT -// file at the top-level directory of this distribution and at -// http://rust-lang.org/COPYRIGHT. -// -// Licensed under the Apache License, Version 2.0 or the MIT license -// , at your -// option. This file may not be copied, modified, or distributed -// except according to those terms. - -#![allow(transmute_from_fn_item_types)] - -use std::mem; - -fn main() { - unsafe { - let u = mem::transmute(main); - let p = mem::transmute(main); - let f = mem::transmute(main); - let tuple: (usize, *mut (), fn()) = (u, p, f); - assert_eq!(mem::transmute::<_, [usize; 3]>(tuple), [main as usize; 3]); - - mem::transmute::<_, usize>(main); - mem::transmute::<_, *mut ()>(main); - mem::transmute::<_, fn()>(main); - } -}