Cleanup: Place::is_indirect vs Place::has_deref

Since #97077 we have these two methods is_indirect and has_deref that are documented as doing the exact same thing, but don't even mention each other in the documentation. Calling the wrong one can either lead to slow code or to subtle bugs. That's pretty bad, we should find a way to clean this up -- and at the very least, this needs better documentation.

I think this is documented in Place::has_deref:

rust/compiler/rustc_middle/src/mir/mod.rs

Lines 1595 to 1603 in 474709a

    
               /// If MirPhase >= Derefered and if projection contains Deref, 
        
               /// It's guaranteed to be in the first place 
        
               pub fn has_deref(&self) -> bool { 
        
                   // To make sure this is not accidentally used in wrong mir phase 
        
                   debug_assert!( 
        
                       self.projection.is_empty() || !self.projection[1..].contains(&PlaceElem::Deref) 
        
                   ); 
        
                   self.projection.first() == Some(&PlaceElem::Deref) 
        
               }

Though why we have that assertion in only one place and not all the others, I have no idea.

I tried pasting this check into is_indirect and it fails in CopyProp so yeah I have no idea 🤷

That doc comment doesn't really tell a user much, does it? It should start by describing what the function does, which is the same as is_indirect. And then it should follow "WARNING: only call this in MIR passes with MirPhase >= Derefered! It will produce wrong results afterwards." But also is_indirect should probably say "If this is called in a MIR pass with MirPhase >= Derefered, use has_deref instead, it will be faster".

One of the two should probably be renamed, there's no good reason for them to have so different names when they implement almost the same spec.

The current comment is an implementation comment that could go inside the function, but not suitable as a doc comment.

Also the methods on Place should probably just delegate to the ones on PlaceRef to avoid the duplication.

I tried pasting this check into is_indirect and it fails in CopyProp so yeah I have no idea shrug

Uh, but doesn't that run sufficiently late that it should be allowed to use has_deref?

Uh, but doesn't that run sufficiently late that it should be allowed to use has_deref?

Yes. At a guess, there is MIR which is temporarily invalid during a pass.

Wow. That makes has_deref quite the footgun.

Rollup merge of rust-lang#114505 - ouz-a:cleanup_mir, r=RalfJung

Rollup merge of #114505 - ouz-a:cleanup_mir, r=RalfJung

Maybe we can close this since #114505 is merged now.

Navigation Menu

Repository navigation

Cleanup: Place::is_indirect vs Place::has_deref #114401

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

	/// If MirPhase >= Derefered and if projection contains Deref,
	/// It's guaranteed to be in the first place
	pub fn has_deref(&self) -> bool {
	// To make sure this is not accidentally used in wrong mir phase
	debug_assert!(
	self.projection.is_empty() \|\| !self.projection[1..].contains(&PlaceElem::Deref)
	);
	self.projection.first() == Some(&PlaceElem::Deref)
	}

Cleanup: Place::is_indirect vs Place::has_deref #114401

Description

Activity

saethlin commented on Aug 3, 2023

saethlin commented on Aug 3, 2023

RalfJung commented on Aug 4, 2023

RalfJung commented on Aug 4, 2023

RalfJung commented on Aug 4, 2023

saethlin commented on Aug 4, 2023

RalfJung commented on Aug 4, 2023

ouz-a commented on Aug 15, 2023

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions