Allow calling const methods on mut values #82436

osa1 · 2021-02-23T07:37:18Z

This allows *const methods to be called on *mut values.

TODOs:

~~Remove debug logs~~ Done.
I haven't tested, but I think this currently won't work when the self value has type like &&&&& *mut X because I don't do any autoderefs when probing. To fix this the new code in rustc_typeck::check::method::probe needs to reuse pick_method somehow as I think that's the function that autoderefs. This works, because autoderefs are done before calling pick_core, in method_autoderef_steps, called by probe_op.
~~I should probably move the new Pick to pick_autorefd_method. If not, I should move it to its own function.~~ Done.
~~Test this with a Pick with to_ptr = true and unsize = true.~~ I think this case cannot happen, because we don't have any array methods with *mut [X] receiver. I should confirm that this is true and document this. I've placed two assertions about this.
~~Maybe give (Mutability, bool) a name and fields~~ I now have a to_const_ptr field in Pick.
Changes in adjust_self_ty is quite hacky. The problem is we can't deref a pointer, and even if we don't have an adjustment to get the address of a value, so to go from *mut to *const we need a special case. There's still a special case for to_const_ptr, but I'm not sure if we can avoid this.
Figure out how reached_raw_pointer stuff is used. I suspect only for error messages.

Fixes #80258

rust-highfive · 2021-02-23T07:37:21Z

r? @lcnr

(rust-highfive has picked a reviewer for you, use r? to override)

osa1 · 2021-02-24T08:04:26Z

I'd like to get some reviews at this point. I didn't remove the log lines as I'll probably need them again.

Who knows about method probing? Who should I ping?

bors · 2021-02-26T00:49:18Z

☔ The latest upstream changes (presumably #82530) made this pull request unmergeable. Please resolve the merge conflicts.

lcnr · 2021-02-26T07:27:00Z

r? @nikomatsakis maybe 🤔

nikomatsakis

Looking good! Left some comments after a first pass.

compiler/rustc_typeck/src/check/method/confirm.rs

nikomatsakis · 2021-03-01T18:32:09Z

compiler/rustc_typeck/src/check/method/confirm.rs

            assert!(pick.unsize.is_none());
+
+            if pick.to_const_ptr {


This could use a comment -- what case is it handling?

to_const_ptr is only set when the receiver object has type *mut and the method's self is *const. Quoting the documentation:

/// Only valid when the receiver type is a `*mut`: coerce the receiver to `*const`. If this it /// `true` then `autoref` should be `None`. pub to_const_ptr: bool,

I will add a comment here.

nikomatsakis · 2021-03-01T18:32:19Z

compiler/rustc_typeck/src/check/method/confirm.rs

+                        assert_eq!(*mutbl, hir::Mutability::Mut);
+                        self.tcx.mk_ptr(ty::TypeAndMut { mutbl: hir::Mutability::Not, ty })
+                    }
+                    _ => panic!(),


Also, why is panic a reasonable thing to do here?

If pick.to_const_ptr is set then the receiver must be a *mut and the method's self type must be *const. to_const_ptr basically says "coerce mut ptr to const ptr". So if it's set, then target must be RawPtr, otherwise it's a bug and we shouldn't have set to_const_ptr = true.

I will add a panic message.

nikomatsakis · 2021-03-01T18:32:38Z

compiler/rustc_typeck/src/check/method/probe.rs

@@ -170,6 +170,10 @@ pub struct Pick<'tcx> {
    ///     B = A | &A | &mut A
    pub autoref: Option<hir::Mutability>,

+    /// Only valid when the self type is a `*mut`: coerce the receiver to `*const`. If this it
+    /// `true` then `autoref` should be `None`.


Nice comment :)

I reworded this slightly, saying "Only valid when the receiver type ..." instead of "... self type ...".

compiler/rustc_typeck/src/check/method/probe.rs

nikomatsakis · 2021-03-01T18:34:51Z

compiler/rustc_typeck/src/check/method/probe.rs

@@ -1143,6 +1160,33 @@ impl<'a, 'tcx> ProbeContext<'a, 'tcx> {
        })
    }

+    /// If `self_ty` is `*mut T` then this picks `*const T` methods. The reason why we have a
+    /// special case for this is because going from `*mut T` to `*const T` with autoderefs and
+    /// autorefs would require dereferencing the pointer, which is not safe.


IIUC, it's not so much that the pointer would be dereferenced, but more that adding an autoref would create a safe reference, which we don't want to do.

I just tried this:

struct Test {} impl Test { fn x(&self) { println!("x called"); } } fn main() { let mut test = Test {}; let mut test_mut_ref = &mut test; test_mut_ref.x(); }

the relevant, unoptimized MIR:

_4 = &(*_2); _3 = Test::x(move _4) -> [return: bb1, unwind: bb2];

I thought the * dereferences, no? Similar code for the pointer version would also do &* and dereference.

You're not wrong, the * does dereference -- it's just that, when you combine it with a &, the net effect is kind of an "identity transform", at least at runtime. That is, &*x is sort of equivalent to x (the "address of the data that x points to" is kind of just "x") -- but only sort of, because now we've made a shared reference, and that has implications (e.g., if x: &mut T, it implies that *x is frozen while this shared reference is live).

Is this the case even when result of & is a pointer, rather than reference? I'm guessing the final assembly for &* (for pointer-to-pointer conversions) may disappear, but I thought in MIR it's still dereferencing, and has the implications of * operation (whatever those implications are).

Instead what we do here is more like x as *const T (where x : *mut T), which doesn't have unsafe dereferencing in MIR, and as a result would not have the same implications.

src/test/ui/methods/receiver-ptr-mutability.rs

compiler/rustc_typeck/src/check/method/confirm.rs

osa1 · 2021-03-02T08:54:47Z

With the last commit I ended up rewriting most of it, sorry for that!

The main change is, the Pick type now has one field for autorefing, unsizing, and converting *mut T to *const T (previously we had three fields). It looks like:

pub struct Pick<'tcx> {
    ...

    /// Indicates that we want to add an autoref (and maybe also unsize it), or if the receiver is
    /// `*mut T`, convert it to `*const T`.
    pub autoref_or_ptr_adjustment: Option<AutorefOrPtrAdjustment<'tcx>>,
}

The AutorefOrPtrAdjustment is an enum, making it clear in at the type level that we can't both autoref and convert from *mut to *const. The type:

/// When adjusting a receiver we often want to do one of
///
/// - Add a `&` (or `&mut`), converting the recevier from `T` to `&T` (or `&mut T`)
/// - If the receiver has type `*mut T`, convert it to `*const T`
///
/// This type tells us which one to do.
///
/// Note that in principle we could do both at the same time. For example, when the receiver has
/// type `T`, we could autoref it to `&T`, then convert to `*const T`. Or, when it has type `*mut
/// T`, we could convert it to `*const T`, then autoref to `&*const T`. However, currently we do
/// (at most) one of these. Either the type has `T` and we convert it to `&T` (or with `mut`), or
/// it has type `*mut T` and we convert it to `*const T`.
#[derive(Debug, PartialEq, Clone)]
pub enum AutorefOrPtrAdjustment<'tcx> {
    /// Receiver has type `T`, add `&` or `&mut` (it `T` is `mut`), and maybe also "unsize" it.
    /// Unsizing is used to convert a `[T; N]` to `[T]`, which only makes sense when autorefing.
    Autoref {
        mutbl: hir::Mutability,

        /// Indicates that the source expression should be "unsized" to a target type. This should
        /// probably eventually go away in favor of just coercing method receivers.
        unsize: Option<Ty<'tcx>>,
    },
    /// Receiver has type `*mut T`, convert to `*const T`
    ToConstPtr,
}

With this type, the assertions in adjust_self_ty are gone, because illegal cases are now unrepresentable.

@nikomatsakis, sorry for the large refactoring. Another review would be very helpful now. Thanks! reviewing the comments would be especially helpful as I'm not sure if I'm 100% right in some places.

I will squash the commits in the final version.

nikomatsakis

This looks good to me! I left a nit about an improved comment.

compiler/rustc_typeck/src/check/method/confirm.rs

nikomatsakis · 2021-03-08T14:52:45Z

compiler/rustc_typeck/src/check/method/probe.rs

+/// type `T`, we could autoref it to `&T`, then convert to `*const T`. Or, when it has type `*mut
+/// T`, we could convert it to `*const T`, then autoref to `&*const T`. However, currently we do
+/// (at most) one of these. Either the type has `T` and we convert it to `&T` (or with `mut`), or
+/// it has type `*mut T` and we convert it to `*const T`.


It may be worth adding a note that while we could do both at the same time, it would have a different meaning -- that is, converting to a &T first would create a safe reference, which asserts (among other things) that the reference is aligned, etc.

I think what you're saying is, if we allowed both autoref'ing converting a ref/ptr to const ptr, the order we do those would matter. Obviously the type of the result will be different, other than that:

Autoref, convert to const (*const T): The pointer will be aligned (I'm not sure if I get this part right, I don't know about reference semantics in detail), among other things (what other things? I'll ask on Zulip)

Convert to const first, then autoref (&*T): the pointer may not be aligned.

Did I get it right?

I'll try to update the comment.

That sounds roughly correct, yes. One nit:

The pointer will be aligned

It's not that the pointer will be aligned, so much as that it would be UB if the pointer were not aligned (i.e., it'd be incorrect code, and the compiler might misoptimize it).

Not sure how to best document this, mostly because I don't know the semantics with references and pointers in detail.

If the comments starting with "Note that ..." is confusing currently I could delete them. Would that be an improvement @nikomatsakis ?

nikomatsakis · 2021-03-08T14:54:25Z

@osa1 feel free to squash the commits

osa1 · 2021-03-08T18:12:08Z

Thanks @nikomatsakis . I'll definitely squash the commits once all is done.

Fixes rust-lang#80258

osa1 · 2021-03-09T10:42:08Z

@nikomatsakis I've squashed the commits.

nikomatsakis · 2021-03-12T10:41:39Z

@bors r+

bors · 2021-03-12T10:41:41Z

📌 Commit 98fbc09 has been approved by nikomatsakis

nikomatsakis · 2021-03-12T10:42:30Z

nice work, @osa1 !

osa1 · 2021-03-12T10:43:26Z

Thanks @nikomatsakis ! I'm aware that some of my comments may not be too clear, I'm happy to update them now or later if you have any suggestions.

Allow calling *const methods on *mut values This allows `*const` methods to be called on `*mut` values. TODOs: - [x] ~~Remove debug logs~~ Done. - [x] ~~I haven't tested, but I think this currently won't work when the `self` value has type like `&&&&& *mut X` because I don't do any autoderefs when probing. To fix this the new code in `rustc_typeck::check::method::probe` needs to reuse `pick_method` somehow as I think that's the function that autoderefs.~~ This works, because autoderefs are done before calling `pick_core`, in `method_autoderef_steps`, called by `probe_op`. - [x] ~~I should probably move the new `Pick` to `pick_autorefd_method`. If not, I should move it to its own function.~~ Done. - [ ] ~~Test this with a `Pick` with `to_ptr = true` and `unsize = true`.~~ I think this case cannot happen, because we don't have any array methods with `*mut [X]` receiver. I should confirm that this is true and document this. I've placed two assertions about this. - [x] ~~Maybe give `(Mutability, bool)` a name and fields~~ I now have a `to_const_ptr` field in `Pick`. - [x] ~~Changes in `adjust_self_ty` is quite hacky. The problem is we can't deref a pointer, and even if we don't have an adjustment to get the address of a value, so to go from `*mut` to `*const` we need a special case.~~ There's still a special case for `to_const_ptr`, but I'm not sure if we can avoid this. - [ ] Figure out how `reached_raw_pointer` stuff is used. I suspect only for error messages. Fixes rust-lang#80258

bors · 2021-03-13T04:38:43Z

⌛ Testing commit 98fbc09 with merge f42888c...

bors · 2021-03-13T07:05:55Z

☀️ Test successful - checks-actions
Approved by: nikomatsakis
Pushing f42888c to master...

Mark-Simulacrum · 2021-03-16T22:59:53Z

This led to a number of sizeable - 5-8% - regressions on several crates. @nikomatsakis @osa1 do you think this is just the cost of adding this additional ability is just necessary? maybe there's some suboptimal bits in the code added that could be polished?

Mark-Simulacrum · 2021-03-17T00:03:32Z

FWIW it also looks like this was a regression in memory usage - https://perf.rust-lang.org/compare.html?start=4d76b4ca52a65d63ab83d82d6630f1df8ec05a93&end=f42888c15fd370b8bca4c5646ffc3aac3005dca8&stat=max-rss

osa1 · 2021-03-17T06:02:21Z

@Mark-Simulacrum Interesting. If this causes regression in max residency, then I think the new Pick type must be larger than the old one. We don't allocate more Picks, or do more work in the common case (only when method probing does not succeed in the previous case). So I think it must be larger Pick.

I'll debug this. In the meantime please feel free to revert this, I'll hopefully submit a new PR with lower RSS.

osa1 · 2021-03-17T06:22:42Z

To test that Pick size is the problem, we could revert the patch, then add as many fields as possible to Pick (without copying the code in the patch) to make it larger, and run the benchmarks again. If I'm right, then it should regress similarly.

jyn514 · 2021-03-18T15:11:59Z

@osa1 you could also try boxing the AutorefOrPtrAdjustment and see if that helps.

osa1 · 2021-03-18T15:29:46Z

Thanks @jyn514 . I'll try that once I figure out how to collect RSS metrics and compare results.

osa1 · 2021-03-19T07:59:58Z

I'm unable to replicate perf results locally, but I compared size of Pick before and after this patch. The type grew from 104 bytes to 112 bytes. I'm surprised 8 bytes can make such an impact. (8.8% increase in RSS in some cases)

I'll try to reduce size of Pick now.

osa1 · 2021-03-19T08:08:14Z

I'm still baffled. The new code (called in pick_core) is only called when method search fails, and currently it should never in benchmarks because it would previously only fail in case of an error.

So in the benchmarks the new code should never be executed, and any increase in runtime, or instructions, should be because of the extra 8 byte allocation. Is Pick such a central type that adding one more word to it makes such a dramatic impact?

osa1 · 2021-03-19T08:36:37Z

I think I figured it out. PR coming.

This change was done in rust-lang#82436, as an "optimization". Unfortunately I missed that this code is not always executed, because of the "continue" in the conditional above it. This commit should solve the perf regressions introduced by rust-lang#82436 as I think there isn't anything else that could affect runtime performance in that PR. The `Pick` type grows only one word, which I doubt can cause up to 8.8% increase in RSS in some of the benchmarks.

osa1 · 2021-03-19T08:46:28Z

I think #83293 should sort this out. Could someone with the rights start a perf job for #83293 please?

osa1 · 2021-03-19T12:02:10Z

FTR, #83293 fixes the perf issues introduced with this patch.

Revert performance-sensitive change in rust-lang#82436 This change was done in rust-lang#82436, as an "optimization". Unfortunately I missed that this code is not always executed, because of the "continue" in the conditional above it. This commit should solve the perf regressions introduced by rust-lang#82436 as I think there isn't anything else that could affect runtime performance in that PR. The `Pick` type grows only one word, which I doubt can cause up to 8.8% increase in RSS in some of the benchmarks. --- Could someone with the rights start a perf job please?

pthariensflame · 2021-04-06T07:23:47Z

Does this need relnotes? (I’m genuinely not sure.)

osa1 · 2021-04-06T10:37:51Z

I think bug fixes don't appear in release notes, right? If that's the case then I guess this is a question of whether this is a change in the specification (in the language, or the arbitrary_self_types extension) or not. If it is then I would expect it to be announced.

I had checked the lang spec a while ago and couldn't see anything relevant to method calls on pointer types (which makes sense, this is an unstable extension).

rust-highfive assigned lcnr Feb 23, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 23, 2021

osa1 mentioned this pull request Feb 23, 2021

*const Self method can't be called with *mut Self (arbitrary_self_types) #80258

Closed

This comment has been minimized.

Sign in to view

osa1 force-pushed the issue80258 branch from cc7b734 to 3636544 Compare February 24, 2021 06:52

osa1 marked this pull request as ready for review February 24, 2021 08:03

rust-highfive assigned nikomatsakis and unassigned lcnr Feb 26, 2021

nikomatsakis reviewed Mar 1, 2021

View reviewed changes

oli-obk reviewed Mar 1, 2021

View reviewed changes

compiler/rustc_typeck/src/check/method/confirm.rs Outdated Show resolved Hide resolved

nikomatsakis approved these changes Mar 8, 2021

View reviewed changes

osa1 force-pushed the issue80258 branch 2 times, most recently from c038936 to cf8b700 Compare March 9, 2021 10:39

Allow calling *const methods on *mut values

98fbc09

Fixes rust-lang#80258

osa1 force-pushed the issue80258 branch from cf8b700 to 98fbc09 Compare March 9, 2021 10:40

osa1 changed the title ~~Allow calling *const methods on *mut values~~ Allow calling *const methods on *mut receivers Mar 9, 2021

osa1 changed the title ~~Allow calling *const methods on *mut receivers~~ Allow calling *const methods on *mut values Mar 9, 2021

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 Mar 12, 2021

Dylan-DPC-zz mentioned this pull request Mar 12, 2021

Rollup of 8 pull requests #83063

Closed

bors added the merged-by-bors This PR was explicitly merged by bors. label Mar 13, 2021

bors merged commit f42888c into rust-lang:master Mar 13, 2021

rustbot added this to the 1.52.0 milestone Mar 13, 2021

bors mentioned this pull request Mar 13, 2021

Include adjustments to allow unsizing coercions for raw slice pointers in receiver position #82190

Closed

osa1 deleted the issue80258 branch March 13, 2021 14:27

osa1 mentioned this pull request Mar 19, 2021

Revert performance-sensitive change in #82436 #83293

Merged

osa1 mentioned this pull request Jul 26, 2021

Bounds-check Array access for the debug runtime dfinity/motoko#2675

Merged

Allow calling *const methods on *mut values #82436

Allow calling *const methods on *mut values #82436

Conversation

osa1 commented Feb 23, 2021 • edited Loading

rust-highfive commented Feb 23, 2021

This comment has been minimized.

osa1 commented Feb 24, 2021

bors commented Feb 26, 2021

lcnr commented Feb 26, 2021

nikomatsakis left a 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

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

osa1 commented Mar 2, 2021 • edited Loading

nikomatsakis left a 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

nikomatsakis commented Mar 8, 2021

osa1 commented Mar 8, 2021

osa1 commented Mar 9, 2021

nikomatsakis commented Mar 12, 2021

bors commented Mar 12, 2021

nikomatsakis commented Mar 12, 2021

osa1 commented Mar 12, 2021

bors commented Mar 13, 2021

bors commented Mar 13, 2021

Mark-Simulacrum commented Mar 16, 2021

Mark-Simulacrum commented Mar 17, 2021

osa1 commented Mar 17, 2021

osa1 commented Mar 17, 2021

jyn514 commented Mar 18, 2021

osa1 commented Mar 18, 2021

osa1 commented Mar 19, 2021

osa1 commented Mar 19, 2021

osa1 commented Mar 19, 2021

osa1 commented Mar 19, 2021

osa1 commented Mar 19, 2021

pthariensflame commented Apr 6, 2021

osa1 commented Apr 6, 2021

Allow calling const methods on mut values #82436

Allow calling const methods on mut values #82436

osa1 commented Feb 23, 2021 •

edited

Loading

osa1 commented Mar 2, 2021 •

edited

Loading