Should there be a warning at compile time when implementing Drop trait with creation of same type inside?

[tom7980]

I'm fairly new to Rust and have been working through the reference book recently, I've just covered the Drop trait implementation and thought it would be fun to see what happens if I create a new instance of the type I just dropped inside of the function.

Obviously this is completely nonsensical as nobody is going to do this but as you can probably tell this caused my stack to overflow and crash the program, would an error message at compile time be appropriate for something like this?

I can't see why anyone would want to initialise new variables inside of the drop trait but it's possible.

Example code of my implementation:

struct CustomSmartPointer {
    data: String,
}

impl CustomSmartPointer{
    fn new(s: String) -> CustomSmartPointer {
        CustomSmartPointer{
            data: s,
        }
    }
}

impl Drop for CustomSmartPointer {
    fn drop(&mut self){
        println!("Dropping & creating new pointer");
        let c = CustomSmartPointer::new(String::from("test"));
        println!("New pointer created in memory");
    }
}

fn main() {
    let c = CustomSmartPointer::new(String::from("test"));
}

[oli-obk]

cc @wesleywiser

Do you think your new MIR based unconditional recursion lint could be extended to also know about Drop calls?

[wesleywiser]

@oli-obk I think that's totally possible. It should just be a matter of handling TerminatorKind::Drop

rust/src/librustc/mir/mod.rs

Lines 1032 to 1036 in 8ec22e7

	Drop {
	location: Place<'tcx>,
	target: BasicBlock,
	unwind: Option<BasicBlock>,
	},

and treating it as a call to Drop::drop() for the item's type.

[oli-obk]

I'm guessing we should handle this in in

rust/src/librustc_mir/lints.rs

Line 128 in 694cf75

_ => {}

The type of the dropped item can probably be extracted from the Place

[agnxy]

Hi @oli-obk , I'd like to work on this to dive deep into rustc.

[oli-obk]

Great!

So there's already some advice up above, but I'll repeat what I think are the steps needed:

1. write a test that should be causing this lint to be emitted
2. add a match arm for TerminatorKind::Drop to

   rust/src/librustc_mir/lints.rs

   Line 128 in 694cf75

   _ => {}

3. obtain the drop type by calling place.ty(tcx, mir).to_ty(tcx)
4. figure out if that type is already the real type we wanted or whether it's a pointer/reference to the real type. Use info! statements for that and pass RUST_LOG=rustc_mir::lints to the command line when you invoke the test
5. if it's not the type, adjust it by extracting the correct type (e.g. using ty.builtin_deref(true)
6. register a call to Drop::drop with the Self type set to the type you obtained
   • this is a little bit involved, you first need to get the lang item for the drop trait. Start off by calling https://doc.rust-lang.org/nightly/nightly-rustc/rustc/ty/struct.TyCtxt.html#method.lang_items and have a look at the docs for how to get the drop trait's DefId out of that
   • When you have the drop trait's DefId, get the DefId of its method (No idea how to do that off the top of my head, if you get stuck on this, don't hesitate to ask)
   • With the DefId of the Drop::drop method and the type to be dropped, you can use Instance::resolve to get an Instance for the concrete T::drop method. The resolve method takes a DefId and Substs, so you'll have to create substs with just the concreate type as an element. Look at some calls to mk_substs to see how to do that.

[tom7980]

• When you have the drop trait, get the DefId of its method (No idea how to do that off the top of my head, if you get stuck on this, don't hesitate to ask)

It looks like this method in LanguageItems returns an option with the DefId inside of it, looks like it procedurally generates methods for all the kinds of items.

rust/src/librustc/middle/lang_items.rs

Lines 91 to 94 in 05812fa

	#[allow(dead_code)]
	pub fn $method(&self) -> Option<DefId> {
	self.items[$variant as usize]
	}

including the drop_trait method:

https://doc.rust-lang.org/nightly/nightly-rustc/rustc/middle/lang_items/struct.LanguageItems.html#method.drop_trait

I think this only gives you the DefId of the Drop Trait though, not the method implemented on the type.

Not sure if this is helpful but I'm quite interested in how it all works so thought I'd do some digging. Look forward to seeing how it's all worked out though.

[agnxy]

Hi @oli-obk @tom7980 ,
Thanks for your guidance.
I made a little progress with the following prototype.

let drop_type = location.ty(mir, tcx).to_ty(tcx);
// get the drop trait's DefId
let drop_trait = tcx.lang_items().drop_trait().unwrap();
// get DefId of its method
let drop_fn = tcx.associated_items(drop_trait).next().unwrap();

But I haven't figured out how to use mk_substs to create substs as a parameter of Instance::resolve. I tried something like tcx.mk_substs(std::iter::once(Kind::from(drop_type))) but got ICE when compiling std. And another question is, after we get an Instance, is the next step for TerminatorKind::Drop match arm similar with the following code?

rust/src/librustc_mir/lints.rs

Line 101 in 694cf75

if let Some(instance) = Instance::resolve(tcx,

Thanks a lot.

[tom7980]

Hi @agnxy

For your issue with the mk_substs, I found this code in the monomorphize.rs source. It looks like you can just call it with an array of items cast to an iter with the .iter().cloned() methods.

let substs = tcx.mk_substs([
        Kind::from(self_ty),
        Kind::from(sig.inputs()[0]),
    ].iter().cloned());

I imagine it could look something like this

let substs = tcx.mk_substs([Kind::from(drop_type)].iter().cloned());

As for the Instance, I'm unsure where to go from there I'd need to spend some time digging around. Let me know if this is useful at all hopefully oli can chime in to give a better explanation

[oli-obk]

but got ICE when compiling std

can you elaborate on the ICE? I'm guessing it's due to your unwrap calls?

is the next step for TerminatorKind::Drop match arm similar with the following code?

Kind of. I think you can factor out

rust/src/librustc_mir/lints.rs

Lines 100 to 120 in 0195812

	let (call_fn_id, call_substs) =
	if let Some(instance) = Instance::resolve(tcx,
	param_env,
	fn_def_id,
	substs) {
	(instance.def_id(), instance.substs)
	} else {
	(fn_def_id, substs)
	};
	let is_self_call =
	call_fn_id == def_id &&
	&call_substs[..caller_substs.len()] == caller_substs;
	if is_self_call {
	self_call_locations.push(terminator.source_info);
	//this is a self call so we shouldn't explore
	//further down this path
	continue;
	}

into its own function in order to "just call it" from the original location and from the new drop checks

[agnxy]

Thanks @tom7980 @oli-obk . I'll continue investigating and studying trait solving in rustc.

can you elaborate on the ICE? I'm guessing it's due to your unwrap calls?

Removing unwrap calls doesn't fix this. It seems that the compiler fails to resolve an obligation at codegen_fulfill_obligation. I have attached the backtrace and debug log. And here is the sample code.

[oli-obk]

This failure is happening because we are in a generic context.

There are three ways to continue

1. make codegen_fulfill_obligation return an Option in case the arguments are too generic to resolve
2. call needs_subst() on the drop_type, if the result is true, then we don't proceed to use Instance::resolve and just use the fallback
3. always use the fallback and don't try to resolve Drop, not sure if that will actually work in practice, but it's worth a try.

[tom7980]

So I've been looking at this in my spare time and tried implementing it with needs_subst() on the drop type, but I'm also getting the same issue with being in a generic context and the compiler not being able to resolve an instance of the drop method on the type. I'm looking at how I would make codegen_fulfill_obligation return an Option currently but need to work on that further.

My real question is however, once we have the T::drop method defId & substs what is the best way to check that we are initializing the same type inside of the method and produce an error?

[Enselic]

PR with fix: #113902