Remove `T: Sized` on pointer `as_ref()` and `as_mut()` #44932

cuviper · 2017-09-29T19:37:39Z

NonZero::is_zero() was already casting all pointers to thin *mut u8 to check for null. The same test on unsized fat pointers can also be used with as_ref() and as_mut() to get fat references.

(This PR formerly changed is_null() too, but checking just the data pointer is not obviously correct for trait objects, especially if *const self sorts of methods are ever allowed.)

`NonZero::is_zero()` was already casting all pointers to thin `*mut u8` to check for null. It seems reasonable to apply that for `is_null()` in general, and then unsized fat pointers can also be used with `as_ref()` and `as_mut()` to get fat references.

rust-highfive · 2017-09-29T19:37:50Z

r? @alexcrichton

(rust_highfive has picked a reviewer for you, use r? to override)

bluss · 2017-09-29T19:50:42Z

src/libcore/tests/ptr.rs

+    assert!(!mz.is_null());
+
+    unsafe {
+        let ncs: *const [u8] = slice::from_raw_parts(null(), 0);


This is UB (null in a &[u8]) so unfortunately can begin to fail when the compiler gets smarter.

Is there any better way to get a null unsized pointer? Or are they impossible to be null in practice? If the latter, we could just drop those parts of the tests.

I suppose I could implement a local Repr hack for the tests, similar to slice::from_raw_parts but without ever calling it a &.

Depending on how much you hate transmute, you could do let ncs: *const [u8] = unsafe { mem::transmute((0usize, 0usize)) };

I went ahead and added a transmute with Repr, which I think is a little safer than a tuple because it can be #[repr(C)]. This is just for testing anyway, so whatever we do isn't going to affect anyone in the wild.

cuviper · 2017-09-29T21:03:17Z

@bluss also pointed out that it could be surprising that there are many possible null values for fat pointers. I added docs mentioning this, but if that's too weird, we could back off to just unsized as_ref and as_mut, leaving is_null sized.

bluss · 2017-09-30T07:10:34Z

To do the null pointer right (via @eddyb): ptr::null::<[u8; 4]>() as *const [u8] or other unsizing coercions.

alexcrichton · 2017-09-30T07:57:57Z

Looks great to me! Since this'll be instantly stable, let's....

@rfcbot fcp merge

rfcbot · 2017-09-30T08:05:08Z

Team member @alexcrichton has proposed to merge this. The next step is review by the rest of the tagged teams:

Concerns:

go (Remove T: Sized on pointer as_ref() and as_mut() #44932 (comment))

Once these reviewers reach consensus, this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

eddyb · 2017-09-30T09:24:26Z

src/libcore/tests/ptr.rs

+            pub len: usize,
+        }
+
+        mem::transmute(Repr { data: null_mut::<T>(), len: 0 })


You don't need this: you can just cast null_mut::<[T; 0]>() to *mut [T].

EDIT: ah yes I see that @bluss commented about this already.

cuviper · 2017-09-30T21:47:01Z

OK, it now uses coercion to get the null pointers, and I added trait objects this way too.

dtolnay · 2017-10-01T16:31:32Z

This is not the behavior that Go has for comparing interfaces (read: traits) against nil (read: null). Are we sure the behavior in this PR is useful? How does Go justify their behavior and do they regret it?

The as_ref / as_mut changes seem fine.

package main

import "fmt"

type S struct {}

func (*S) String() string {
	return "S"
}

func main() {
	var s fmt.Stringer = (*S)(nil)
	fmt.Println(s == nil) // false
}

@rfcbot concern go

eddyb · 2017-10-01T16:36:22Z

@dtolnay Currently it is UB to have a null vtable pointer, it's as if *mut Trait and &mut Trait / Box<Trait> all have the same safe &'static VtableForTrait pointer as the fat metadata.

cuviper · 2017-10-01T17:07:12Z

@dtolnay Note however that this isn't changing PartialEq behavior. That's why I added the doc note that two is_null pointers may not compare equal.

I don't really know Go. If you have such a nil-initialized interface pointer, is there no way to tell if it's actually nil? (thus shouldn't be called)

dtolnay · 2017-10-03T20:41:29Z

Re: @cuviper

If you have such a nil-initialized interface pointer, is there no way to tell if it's actually nil? (thus shouldn't be called)

By design, it is not easy to tell if the data pointer is nil. This can be confusing but it is because interfaces with a nil data pointer are totally fine to call interface methods on -- as long as those methods do not lead to the nil pointer being dereferenced. In the code above, calling s.String() is fine even though the data pointer in s is nil. It invokes the String method on *S passing nil as the receiver.

If checking the data pointer for nil is really what you mean, there are some ways. But as I understand it, this would be very unusual in Go code (which is why I was startled to see us define ptr::is_null this way).

// use type assertion to check for nil data pointer of a particular type
p, ok := s.(*S)
if ok {
	isNil = p == nil
}

// use reflection to check for nil data pointer of any type
isNil = reflect.ValueOf(s).IsNil()

Re: @eddyb

Currently it is UB to have a null vtable pointer

Do you have any sense of what the future holds here? In particular, I would be concerned about how this PR interacts with the following two potential changes:

We allow ptr::null to support T: ?Sized by returning a null vtable pointer.
```
ptr::null::<ToString>()
```
As part of pointer-related unsafe ergonomics in Improve unsafe ergonomics rfcs#433, we allow methods to take self by *const or *mut and this is object-safe.
```
trait Trait {
    unsafe fn f(*const self);
}
```

In combination, these would put us in a state like Go where checking for null data pointer is almost never what you want and checking for null data pointer + null vtable is almost always what you want.

Have there been any discussions that would rule out one or both of these from being pursued in the future?

cuviper · 2017-10-03T21:07:26Z

If Rust ptr::null starts returning null vtables too, then == null() should suffice for users to check for this case. Then as_ref and as_mut can still be used to check for null data, assuming a good vtable. But what can one do for the possibility of (non-null data, null vtable)?

But even with the possibility of trait Trait { unsafe fn f(*const self); }, I think it's still useful and not too surprising for is_null() to return true for a null self, even if that fat pointer has a concrete vtable. It seems fine to me that one should use == null() to check for totally null, and document the distinction. I think it's an improvement over Go to be able to check both meanings of null (without reflection).

Slices are weird too. ptr::null would probably return 0 length, but == null() is not as useful here, as you may have a null pointer with other lengths. For this I think it makes sense that is_null only cares about the data pointer.

If we want to defer a decision on is_null, I'd be OK with just as_ref and as_mut for now.

dtolnay · 2017-10-03T21:40:07Z

what can one do for the possibility of (non-null data, null vtable)?

I think that can't happen.

I think it's still useful and not too surprising for is_null() to return true for a null self, even if that fat pointer has a concrete vtable.

I don't yet agree that this would be useful. Do you have a use case in mind where the best solution involves checking whether the data pointer of a trait object is null? It is possible that these are universally better served by some combination of not using trait objects, using as_ref / as_mut, or checking null data pointer + null vtable.

cuviper · 2017-10-03T22:07:31Z

If nothing else, I think as_ref and as_mut are more easily explained in terms of an is_null check which the user could also perform manually, but it's not a big deal.

eddyb · 2017-10-04T10:17:54Z

I don't think we'll ever see distinct requirements for validity of metadata between safe and unsafe pointers if we get any kind of Custom DSTs, because of the high complexity associated with having it not be uniform and requiring potentially user-defined conversions for casts which are builtin now.

As for trait Trait { unsafe fn f(*const self); } I don't see how that changes anything? You still need a valid vtable to call the method, in fact it makes having a valid vtable even more important.

dtolnay · 2017-10-04T11:29:55Z

As for trait Trait { unsafe fn f(*const self); } I don't see how that changes anything?

This is what makes Go's choice of behavior for == nil make sense -- see the code above with (*S)(nil). In Go if you have a trait object, by convention you get to assume that it is either completely nil including vtable, or it is a valid implementation of the interface. Whether or not the data pointer is nil is none of your business. If the caller gave you a trait object with nil data and non-nil vtable, it is the caller's responsibility that calling the trait methods with a nil data pointer is fine.

Transliterated into Rust it would be:

struct S {}

impl Trait for S {
    unsafe fn f(*const self) -> &'static str {
        "S"
    }
}

fn main() {
    let s: *const Trait = ptr::null::<S>();
    println!("{}", s.is_null()); // false, calling Trait::f is fine

    let s: *const Trait = ptr::null::<Trait>();
    println!("{}", s.is_null()); // true, calling Trait::f is not fine
}

KamilaBorowska · 2017-10-08T11:48:18Z

@dtolnay

This is not the behavior that Go has for comparing interfaces (read: traits) against nil (read: null). Are we sure the behavior in this PR is useful? How does Go justify their behavior and do they regret it?

https://golang.org/doc/faq#nil_error

Considering advice like "It's a good idea for functions that return errors always to use the error type in their signature (as we did above) rather than a concrete type such as *MyError, to help guarantee the error is created correctly.", I can imagine this being a design mistake.

As for why it happens, it's because there is no explicit is_nil function for interfaces, but rather a comparison is used. For instance, assuming p is interface{}((*int)(nil)) (interface of *int type and nil value).

p == nil

The types of comparison have to be identical, so an implicit cast is inserted

p == interface{}(nil)

An interface is pair of a type pointer and a value. As nil is a value that exists in multiple types, Go doesn't try to assume any particular type, and just uses null pointer as type name.

Essentially this makes it a comparison like the following (this is pseudo-Go).

(*int, nil) == (nil, nil)

nil is a different type than *int, so comparison does fail.

A type needs to be compared while comparing 2 interfaces, as Go does provide value types. For instance, the following program returns true and false.

package main

import "fmt"

func main() {
	fmt.Println(interface{}(int(2)) == interface{}(int(2)))
	fmt.Println(interface{}(int(2)) == interface{}(uint(2)))
}

Considering Rust doesn't allow null vtable pointers, has a function checking for null pointers instead of a comparison, and *const Trait being always implemented as pair of 2 pointers (not required by Go) this doesn't sound like an issue for Rust, as long std::ptr::null cannot be used to directly create trait objects pointers.

To be precise, 0 as *const ToString and std::ptr::null::<ToString>() must be an error to avoid this issue (and it currently is) - language needs to require specifying any type that implements a trait, for example std::ptr::null::<i32>() as *const ToString.

dtolnay · 2017-10-08T15:09:00Z

@xfix we agree that the is_null behavior in this PR makes sense in Rust today. I disagree that we definitively never want to support ptr::null::<Trait>() in the future. I would prefer to contextualize that decision in the broader discussion around raw pointer ergonomics in rust-lang/rfcs#433.

The exact semantics of `is_null` on unsized pointers are still debatable, especially for trait objects. It may be legal to call `*mut self` methods on a trait object someday, as with Go interfaces, so `is_null` might need to validate the vtable pointer too. For `as_ref` and `as_mut`, we're assuming that you cannot have a non-null data pointer with a null vtable, so casting the unsized check is fine.

cuviper · 2017-10-11T23:46:22Z

I pushed an update to restore is_null, so we can leave that as an open design question. Now this PR is only unsizing as_ref and as_mut.

ExpHP · 2017-11-01T23:51:07Z

My comment concerns the semantics not of pointers, but of borrows. I am saying that, because ptr.as_ref().is_none() constructs a borrow in the case where it returns true, there are only two possibilities for how it could behave on a null data pointer with a non-null vtable:

it returns false, or
it formats your hard drive.

is_null() is completely different, because it does not create a borrow.

bluss · 2017-11-01T23:57:12Z

@ExpHP Yes, it seems necessary to produce None in that case. It seems it is unresolved if there are other cases that also produce None.

dtolnay · 2017-11-02T00:00:47Z

@ExpHP @bluss in #44932 (comment) I would expect the first case to produce Some with a null data pointer and a non-null vtable. By Go's logic, the fact that the caller passed a non-null pointer *const Trait gives callee permission to call trait methods on it -- whether the data pointer is null is none of the callee's business.

EDIT: disregard, this would not make sense

ExpHP · 2017-11-02T00:09:49Z

@dtolnay so if I understand your responses correctly, you want the following to be on the table as well? (i.e. allow not just raw pointers, but also borrows to have null data):

struct S {}

impl Trait for S {
    // Notice: &self, not *const self
    fn f(&self) -> &'static str {
        "S"
    }
}

fn main() {
    // Notice: &Trait, not a raw pointer
    let s: &'static Trait = unsafe { ptr::null::<S>().as_ref().unwrap() };
    println!("{}", s.f());
}

dtolnay · 2017-11-02T17:28:08Z

@ExpHP that code is stable already so changing it would not be on the table.

I read your comments again and I find the point about as_ref constructing a borrow compelling. I see why that means as_ref and as_mut are not problematic the way is_null is. Thanks for spelling it out for us! That + removing is_null from the PR resolves the concern I had so I think we can try this again (I will copy over the previous checkboxes).

@rfcbot fcp merge

rfcbot · 2017-11-02T17:28:08Z

Team member @dtolnay has proposed to merge this. The next step is review by the rest of the tagged teams:

No concerns currently listed.

Once these reviewers reach consensus, this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

carols10cents · 2017-11-06T14:18:57Z

@BurntSushi ticky box here waiting for you!

rfcbot · 2017-11-07T00:14:32Z

🔔 This is now entering its final comment period, as per the review above. 🔔

alexcrichton · 2017-11-07T18:17:01Z

@bors: r+

bors · 2017-11-07T18:17:02Z

📌 Commit 604f049 has been approved by alexcrichton

alexcrichton · 2017-11-07T18:17:09Z

@bors: r+

oops...

bors · 2017-11-07T18:17:09Z

💡 This pull request was already approved, no need to approve it again.

There's another pull request that is currently being tested, blocking this pull request: Rollup of 9 pull requests #45822

bors · 2017-11-07T18:17:10Z

📌 Commit 604f049 has been approved by alexcrichton

Remove `T: Sized` on pointer `as_ref()` and `as_mut()` `NonZero::is_zero()` was already casting all pointers to thin `*mut u8` to check for null. The same test on unsized fat pointers can also be used with `as_ref()` and `as_mut()` to get fat references. (This PR formerly changed `is_null()` too, but checking just the data pointer is not obviously correct for trait objects, especially if `*const self` sorts of methods are ever allowed.)

bors · 2017-11-07T20:55:08Z

⌛ Testing commit 604f049 with merge ee22861...

bors · 2017-11-07T23:53:16Z

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing ee22861 to master...

@cuviper

Remove `T: Sized` on `ptr::is_null()` Originally from #44932 -- this is purely a revert of the last commit of that PR, which was removing some changes from the previous commits in the PR. So a revert of a revert means this is code written by @cuviper! @mikeyhew makes a compelling case in rust-lang/rfcs#433 (comment) for why this is the right way to implement `is_null` for trait objects. And the behavior for slices makes sense to me as well. ```diff impl<T: ?Sized> *const T { - pub fn is_null(self) -> bool where T: Sized; + pub fn is_null(self) -> bool; } impl<T: ?Sized> *mut T { - pub fn is_null(self) -> bool where T: Sized; + pub fn is_null(self) -> bool; }

rust-highfive assigned alexcrichton Sep 29, 2017

bluss reviewed Sep 29, 2017

View reviewed changes

cuviper added 2 commits September 29, 2017 13:19

Avoid the UB of a null reference to a slice

8b2e09f

Document that there are many possible null pointers

5c61183

alexcrichton added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Sep 30, 2017

rfcbot added the proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. label Sep 30, 2017

eddyb reviewed Sep 30, 2017

View reviewed changes

Use unsized coercions for null ptr tests

40a678d

carols10cents added the S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). label Oct 2, 2017

plietar mentioned this pull request Oct 5, 2017

Implement RFC 1861: Extern types #44295

Merged

rfcbot added the proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. label Nov 2, 2017

rfcbot added final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. and removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. labels Nov 7, 2017

kennytm added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). labels Nov 7, 2017

alexcrichton closed this Nov 7, 2017

alexcrichton reopened this Nov 7, 2017

bors merged commit 604f049 into rust-lang:master Nov 7, 2017

dtolnay mentioned this pull request Nov 19, 2017

Remove T: Sized on ptr::is_null() #46094

Merged

mikeyhew mentioned this pull request Dec 13, 2017

arbitrary_self_types: add support for raw pointer self types #46664

Merged

cuviper deleted the unsized-ptr-is_null branch January 26, 2018 21:28

ExpHP mentioned this pull request Feb 21, 2018

*const T has quite a few methods that seem like they don't need to require T: Sized #42847

Open

cuviper mentioned this pull request Nov 11, 2019

Null fat pointers #66316

Closed

Remove T: Sized on pointer as_ref() and as_mut() #44932

Remove T: Sized on pointer as_ref() and as_mut() #44932

Conversation

cuviper commented Sep 29, 2017 • edited Loading

rust-highfive commented Sep 29, 2017

bluss Sep 29, 2017

Choose a reason for hiding this comment

cuviper Sep 29, 2017

Choose a reason for hiding this comment

cuviper Sep 29, 2017

Choose a reason for hiding this comment

cramertj Sep 29, 2017

Choose a reason for hiding this comment

cuviper Sep 29, 2017 • edited Loading

Choose a reason for hiding this comment

cuviper commented Sep 29, 2017

bluss commented Sep 30, 2017

alexcrichton commented Sep 30, 2017 • edited Loading

rfcbot commented Sep 30, 2017 • edited by aturon Loading

eddyb Sep 30, 2017 • edited Loading

Choose a reason for hiding this comment

cuviper commented Sep 30, 2017

dtolnay commented Oct 1, 2017

eddyb commented Oct 1, 2017

cuviper commented Oct 1, 2017

dtolnay commented Oct 3, 2017

cuviper commented Oct 3, 2017

dtolnay commented Oct 3, 2017

cuviper commented Oct 3, 2017

eddyb commented Oct 4, 2017

dtolnay commented Oct 4, 2017

KamilaBorowska commented Oct 8, 2017 • edited Loading

dtolnay commented Oct 8, 2017

cuviper commented Oct 11, 2017

ExpHP commented Nov 1, 2017 • edited Loading

bluss commented Nov 1, 2017

dtolnay commented Nov 2, 2017 • edited Loading

ExpHP commented Nov 2, 2017 • edited Loading

dtolnay commented Nov 2, 2017

rfcbot commented Nov 2, 2017 • edited by BurntSushi Loading

carols10cents commented Nov 6, 2017

rfcbot commented Nov 7, 2017

alexcrichton commented Nov 7, 2017

bors commented Nov 7, 2017

alexcrichton commented Nov 7, 2017

bors commented Nov 7, 2017

bors commented Nov 7, 2017

bors commented Nov 7, 2017

bors commented Nov 7, 2017

Remove `T: Sized` on pointer `as_ref()` and `as_mut()` #44932

Remove `T: Sized` on pointer `as_ref()` and `as_mut()` #44932

cuviper commented Sep 29, 2017 •

edited

Loading

cuviper Sep 29, 2017 •

edited

Loading

alexcrichton commented Sep 30, 2017 •

edited

Loading

rfcbot commented Sep 30, 2017 •

edited by aturon

Loading

eddyb Sep 30, 2017 •

edited

Loading

KamilaBorowska commented Oct 8, 2017 •

edited

Loading

ExpHP commented Nov 1, 2017 •

edited

Loading

dtolnay commented Nov 2, 2017 •

edited

Loading

ExpHP commented Nov 2, 2017 •

edited

Loading

rfcbot commented Nov 2, 2017 •

edited by BurntSushi

Loading