Tracking issue for RFC 1892, "Deprecate uninitialized in favor of a new MaybeUninit type"

[Centril]

NEW TRACKING ISSUE = #63566

This is a tracking issue for the RFC "Deprecate uninitialized in favor of a new MaybeUninit type" (rust-lang/rfcs#1892).

Steps:

• Implement the RFC (cc @rust-lang/libs)
• Adjust documentation (in stabilize core parts of MaybeUninit #60445)
• Stabilization PR (in stabilize core parts of MaybeUninit #60445)

Unresolved questions:

• Should we have a safe setter that returns an &mut T?
• Should we rename MaybeUninit? (Rename MaybeUninit to MaybeUninitialized #56138)
• Should we rename into_inner? Should it be more like take instead and take &mut self?
• Should MaybeUninit<T> be Copy for T: Copy?
• Should we allow calling get_ref and get_mut (but not reading from the returned references) before data got initialized? (AKA: "Are references to uninitialized data insta-UB, or only UB when being read from?") Should we rename it similar to into_inner?
• Can we make into_inner (or whatever it ends up being called) panic when T is uninhabited, like mem::uninitialized does currently? (done)
• Seems like we want to not deprecate mem::zeroed. We should however remember to also update its documentation together with MaybeUninit, make sure people are aware that this is insta-UB if all-0-bits does not satisfy the type's validity invariant.

[eddyb]

cc @RalfJung

[japaric]

[ ] Implement the RFC

I can help implement the RFC.

[RalfJung]

Awesome, I can help reviewing :)

[japaric]

I'd like some clarification on this part of the RFC:

Make calling uninitialized on an empty type trigger a runtime panic which also prints the deprecation message.

Should only mem::uninitialized::<!>() panic? Or should this also cover structs (and maybe enums?) that contain the empty type (e.g. (!, u8))?

[RalfJung]

AFAIK we only do the really harmful code generation for !. Most other uses of mem::uninitialized are just as incorrect, but the compiler does not happen to exploit them.

So I'd do it for ! only, but also for mem::zeroed. (I forgot to amend that part when I added zeroed to the RFC, it seems.)

[eddyb]

We could start off by making this:

rust/src/librustc_codegen_llvm/intrinsic.rs

Lines 184 to 198 in 8928de7

	"init" => {
	let ty = substs.type_at(0);
	if !cx.layout_of(ty).is_zst() {
	// Just zero out the stack slot.
	// If we store a zero constant, LLVM will drown in vreg allocation for large data
	// structures, and the generated code will be awful. (A telltale sign of this is
	// large quantities of `mov [byte ptr foo],0` in the generated code.)
	memset_intrinsic(bx, false, ty, llresult, C_u8(cx, 0), C_usize(cx, 1));
	}
	return;
	}
	// Effectively no-ops
	"uninit" => {
	return;
	}

check whether fn_ty.ret.layout.abi is Abi::Uninhabited and at the very least emit a trap, e.g.:

rust/src/librustc_codegen_llvm/mir/operand.rs

Lines 400 to 403 in 8928de7

	// Allow RalfJ to sleep soundly knowing that even refactorings that remove
	// the above error (or silence it under some conditions) will not cause UB
	let fnname = bx.cx.get_intrinsic(&("llvm.trap"));
	bx.call(fnname, &[], None);

Once you've seen the trap (i.e. intrinsics::abort) in action, you can see if there's any nice way of triggering a panic. It'' be tricky because of unwinding, we'll need to special-case them here:

rust/src/librustc_codegen_llvm/mir/block.rs

Lines 445 to 447 in 8928de7

	let intrinsic = intrinsic.as_ref().map(|s| &s[..]);
	if intrinsic == Some("transmute") {

To actually panic, you'd need something like this:

rust/src/librustc_codegen_llvm/mir/block.rs

Lines 360 to 407 in 8928de7

	// Get the location information.
	let loc = bx.sess().codemap().lookup_char_pos(span.lo());
	let filename = Symbol::intern(&loc.file.name.to_string()).as_str();
	let filename = C_str_slice(bx.cx, filename);
	let line = C_u32(bx.cx, loc.line as u32);
	let col = C_u32(bx.cx, loc.col.to_usize() as u32 + 1);
	let align = tcx.data_layout.aggregate_align
	.max(tcx.data_layout.i32_align)
	.max(tcx.data_layout.pointer_align);
	// Put together the arguments to the panic entry point.
	let (lang_item, args) = match *msg {
	EvalErrorKind::BoundsCheck { ref len, ref index } => {
	let len = self.codegen_operand(&mut bx, len).immediate();
	let index = self.codegen_operand(&mut bx, index).immediate();
	let file_line_col = C_struct(bx.cx, &[filename, line, col], false);
	let file_line_col = consts::addr_of(bx.cx,
	file_line_col,
	align,
	Some("panic_bounds_check_loc"));
	(lang_items::PanicBoundsCheckFnLangItem,
	vec![file_line_col, index, len])
	}
	_ => {
	let str = msg.description();
	let msg_str = Symbol::intern(str).as_str();
	let msg_str = C_str_slice(bx.cx, msg_str);
	let msg_file_line_col = C_struct(bx.cx,
	&[msg_str, filename, line, col],
	false);
	let msg_file_line_col = consts::addr_of(bx.cx,
	msg_file_line_col,
	align,
	Some("panic_loc"));
	(lang_items::PanicFnLangItem,
	vec![msg_file_line_col])
	}
	};
	// Obtain the panic entry point.
	let def_id = common::langcall(bx.tcx(), Some(span), "", lang_item);
	let instance = ty::Instance::mono(bx.tcx(), def_id);
	let fn_ty = FnType::of_instance(bx.cx, &instance);
	let llfn = callee::get_fn(bx.cx, instance);
	// Codegen the actual panic invoke/call.
	do_call(self, bx, fn_ty, llfn, &args, None, cleanup);

(you can ignore the EvalErrorKind::BoundsCheck arm)

[japaric]

@eddyb Thanks for the pointers.

---

I'm now fixing (several) deprecation warnings and I feel (very) tempted to just run sed -i s/mem::uninitialized()/mem::MaybeUninit::uninitialized().into_inner()/g but I guess that would miss the point ... Or is that OK if I know that the value is a concrete (Copy) type? e.g. let x: [u8; 1024] = mem::uninitialized();.

[RalfJung]

That would exactly miss the point, yeah.^^

At least for now, I would like to consider mem::MaybeUninit::uninitialized().into_inner() UB for all non-union types. Notice that Copy is certainly not sufficient; both bool and &'static i32 are Copy and your snippet is intended to be insta-UB for them. We may want an exception for "types where all bit patterns are okay" (integer types, essentially), but I would be opposed to making such an exception because undef is not a normal bit pattern. That's why the RFC says you need to fully initialize before calling into_inner.

It also says that for get_mut, but the RFC discussion brought up desired by some folks to relax the restriction here. That's an option I could live with. But not for into_inner.

I'm afraid all these uses of uninitialized will have to be more carefully reviewed, and in fact this was one of the intents of the RFC. We'd like the wider ecosystem to be more careful here, if everyone just uses into_inner immediately then the RFC was worthless.

[Centril]

We'd like the wider ecosystem to be more careful here, if everyone just uses into_inner immediately then the RFC was worthless.

This gives me an idea... perhaps we should lint (group: "correctness") for this sort of code? cc @oli-obk

[SimonSapin]

I'm now fixing (several) deprecation warnings

We should only ship Nightly with those warnings once the recommended replacement is available at least on Stable. See similar discussion at #52994 (comment)