Add documentation to has_deref #114505

ouz-a · 2023-08-05T09:05:35Z

Documentation of has_deref needed some polish to be more clear about where it should be used and what's it's purpose.

RalfJung · 2023-08-05T10:41:49Z

compiler/rustc_middle/src/mir/mod.rs

@@ -1593,7 +1593,8 @@ impl<'tcx> Place<'tcx> {
    }

    /// If MirPhase >= Derefered and if projection contains Deref,
-    /// It's guaranteed to be in the first place
+    /// It's guaranteed to be in the first place, this functions checks
+    /// both of these conditions and must only be called while MirPhase >= Derefered


Both of which conditions? And it only does that check in debug-mode, so really this is not checked most of the time! The caller can not rely on this check.

Also there is no phase called "Derefered"? At least look at https://doc.rust-lang.org/nightly/nightly-rustc/rustc_middle/mir/syntax/enum.MirPhase.html I cannot find one. The AnalysisPhase::PostCleanup says that * must be the first projection, so maybe this should say "when in a Runtime MirPhase"?

My bad, it should been MirPass not MirPhase, https://doc.rust-lang.org/nightly/nightly-rustc/src/rustc_mir_transform/lib.rs.html#454-463

Passes get reordered though. MirPhase is the right concept to refer to here. MirPhase make certain guarantees. For example, the PostCleanup one makes the guarantee we care about here. So that's what it should reference.

compiler/rustc_middle/src/mir/mod.rs

rustbot · 2023-08-06T06:55:22Z

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

RalfJung · 2023-08-06T11:41:45Z

compiler/rustc_middle/src/mir/mod.rs

-    /// If MirPhase >= Derefered and if projection contains Deref,
-    /// It's guaranteed to be in the first place
-    pub fn has_deref(&self) -> bool {
+    /// If MirPass >= Derefered and if projection contains Deref,


MirPass doesn't implement PartialOrd so this doesn't make a lot of sense.

MirPhase does implement PartialOrd, on the other hand.

RalfJung · 2023-08-06T11:42:08Z

compiler/rustc_middle/src/mir/mod.rs

+    ///
+    /// This function checks the statement above, and must only be called
+    /// while MirPass >= Derefer.
+    pub fn is_indirect_first_projection(&self) -> bool {
        // To make sure this is not accidentally used in wrong mir phase


Suggested change

// To make sure this is not accidentally used in wrong mir phase

this comment referred to the debug assertion that is no longer here

RalfJung · 2023-08-06T11:43:14Z

compiler/rustc_middle/src/mir/mod.rs

+    /// If MirPass >= Derefered and if projection contains Deref,
+    /// Deref must to be in the first place.
+    ///
+    /// This function checks the statement above, and must only be called


The comment is wrong or confusing. It says that the function checks that "if there is a deref it is in the first place", but that's not what the function checks.

Simplified the wording I think it's clearer now.

RalfJung · 2023-08-06T12:08:19Z

compiler/rustc_middle/src/mir/mod.rs

+    /// Returns 'true' if this 'Place's first projection equals to 'Deref' projection else
+    /// returns false.
+    ///
+    /// Must only be called, after `MirPass::Derefer`.


Suggested change

/// Returns 'true' if this 'Place's first projection equals to 'Deref' projection else

/// returns false.

///

/// Must only be called, after `MirPass::Derefer`.

/// Returns 'true' if this `Place`'s first projection is `Deref`.

///

/// This is useful because for MIR phases `AnalysisPhase::PostCleanup` and later,

/// `Deref` projections can only occur as the first projection. In that case this method

/// is equivalent to `is_indirect`, but faster.

Well that's certainly way better than mine, thanks for the review

RalfJung

Missed that in my previous suggestions.

With that fix, r=me. Thanks for taking care of this!

RalfJung · 2023-08-06T13:14:22Z

compiler/rustc_middle/src/mir/mod.rs

-            self.projection.is_empty() || !self.projection[1..].contains(&PlaceElem::Deref)
-        );
-        self.projection.first() == Some(&PlaceElem::Deref)
+    /// Returns 'true' if this `Place`'s first projection is `Deref`.


Suggested change

/// Returns 'true' if this `Place`'s first projection is `Deref`.

/// Returns `true` if this `Place`'s first projection is `Deref`.

RalfJung · 2023-08-06T13:14:34Z

compiler/rustc_middle/src/mir/mod.rs

-    /// If MirPhase >= Derefered and if projection contains Deref,
-    /// It's guaranteed to be in the first place
-    pub fn has_deref(&self) -> bool {
+    /// Returns 'true' if this `Place`'s first projection is `Deref`.


Suggested change

/// Returns 'true' if this `Place`'s first projection is `Deref`.

/// Returns `true` if this `Place`'s first projection is `Deref`.

ouz-a · 2023-08-06T14:29:54Z

@bors r=RalfJung

edit: that doesn't seem like worked ? 🤔

bors · 2023-08-06T14:29:56Z

@ouz-a: 🔑 Insufficient privileges: Not in reviewers

RalfJung · 2023-08-06T14:35:19Z

@bors r+ rollup

bors · 2023-08-06T14:35:21Z

📌 Commit 6df5462 has been approved by RalfJung

It is now in the queue for this repository.

…iaskrgr Rollup of 5 pull requests Successful merges: - rust-lang#114466 (Add Allocation to SMIR) - rust-lang#114505 (Add documentation to has_deref) - rust-lang#114519 (use offset_of! to calculate dirent64 field offsets) - rust-lang#114537 (Migrate GUI colors test to original CSS color format) - rust-lang#114539 (linkchecker: Remove unneeded FIXME about intra-doc links) Failed merges: - rust-lang#114485 (Add trait decls to SMIR) r? `@ghost` `@rustbot` modify labels: rollup

Add documentation to has_deref Documentation of `has_deref` needed some polish to be more clear about where it should be used and what's it's purpose. cc rust-lang#114401 r? `@RalfJung`

rustbot assigned RalfJung Aug 5, 2023

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Aug 5, 2023

ouz-a mentioned this pull request Aug 5, 2023

Simplify some code that depend on Deref #97077

Merged

RalfJung reviewed Aug 5, 2023

View reviewed changes

compiler/rustc_middle/src/mir/mod.rs Outdated Show resolved Hide resolved

RalfJung reviewed Aug 5, 2023

View reviewed changes

compiler/rustc_middle/src/mir/mod.rs Outdated Show resolved Hide resolved

RalfJung reviewed Aug 6, 2023

View reviewed changes

ouz-a force-pushed the cleanup_mir branch from 8d6a8a8 to 11f817c Compare August 6, 2023 12:13

RalfJung reviewed Aug 6, 2023

View reviewed changes

cleanup misinformation regarding has_deref

6df5462

ouz-a force-pushed the cleanup_mir branch from c66b036 to 6df5462 Compare August 6, 2023 14:29

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 6, 2023

matthiaskrgr mentioned this pull request Aug 6, 2023

Rollup of 5 pull requests #114553

Merged

bors merged commit 13de583 into rust-lang:master Aug 6, 2023
11 checks passed

rustbot added this to the 1.73.0 milestone Aug 6, 2023

ouz-a deleted the cleanup_mir branch August 10, 2023 10:03

ouz-a mentioned this pull request Aug 15, 2023

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

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add documentation to has_deref #114505

Add documentation to has_deref #114505

ouz-a commented Aug 5, 2023

RalfJung Aug 5, 2023

RalfJung Aug 5, 2023 •

edited

Loading

ouz-a Aug 6, 2023

RalfJung Aug 6, 2023

rustbot commented Aug 6, 2023

RalfJung Aug 6, 2023

RalfJung Aug 6, 2023

RalfJung Aug 6, 2023

ouz-a Aug 6, 2023

RalfJung Aug 6, 2023

ouz-a Aug 6, 2023

RalfJung left a comment

RalfJung Aug 6, 2023

RalfJung Aug 6, 2023

ouz-a commented Aug 6, 2023 •

edited

Loading

bors commented Aug 6, 2023

RalfJung commented Aug 6, 2023

bors commented Aug 6, 2023

	/// Returns 'true' if this `Place`'s first projection is `Deref`.
	/// Returns `true` if this `Place`'s first projection is `Deref`.

Add documentation to has_deref #114505

Add documentation to has_deref #114505

Conversation

ouz-a commented Aug 5, 2023

Choose a reason for hiding this comment

RalfJung Aug 5, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rustbot commented Aug 6, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RalfJung left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ouz-a commented Aug 6, 2023 • edited Loading

bors commented Aug 6, 2023

RalfJung commented Aug 6, 2023

bors commented Aug 6, 2023

RalfJung Aug 5, 2023 •

edited

Loading

ouz-a commented Aug 6, 2023 •

edited

Loading