Make transmuting from fn item types to pointer-sized types a hard error.

eddyb · eddyb · commit 7650afc1cea2 · 2017-02-28T23:47:55.000+02:00
diff --git 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<i32>) {
+   ...
+}
+
+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
@@ -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
@@ -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<T>`, 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<typeof(function)>` 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
@@ -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 <https://github.com/rust-lang/rust/issues/36888>",
         },
-        FutureIncompatibleInfo {
-            id: LintId::of(TRANSMUTE_FROM_FN_ITEM_TYPES),
-            reference: "issue #19925 <https://github.com/rust-lang/rust/issues/19925>",
-        },
         FutureIncompatibleInfo {
             id: LintId::of(OVERLAPPING_INHERENT_IMPLS),
             reference: "issue #36889 <https://github.com/rust-lang/rust/issues/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
@@ -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
@@ -10,14 +10,61 @@
 
 use std::mem;
 
+unsafe fn foo() -> (isize, *const (), Option<fn()>) {
+    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::<fn(), usize>(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<fn()>>(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::<Option<fn()>, 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
diff --git a/src/test/run-pass/transmute-from-fn-item-types.rs b/src/test/run-pass/transmute-from-fn-item-types.rs
