Dedup & de-lint

[dwijnand]

Just some cleanup I noticed in reading the code.

The last commit is extra - if you prefer the before state I can drop the commit.

[withoutboats]

So I have kind of meandering train of thought on this PR.

First of all, there are some things that I think are strictly code clean up, where I was unnecessarily verbose because I didn't know how smart our compiler was. Specifically I mean:

• Removing the unsafe on static reference's clone (the same could be done to static reference's drop). I didn't realize the compiler could coerce safe functions into unsafe function pointers.
• Removing the turbofishes everywhere. I didn't realize the compiler could infer it should apply T to these functions.

A separate PR which just removed those two unnecessary unsafes and all the unnecessary turbofishes I would accept immediately.

However, the other changes I'm less certain about.

First, I notice overall that the wrap function increases the lines of code, because you replace 1 line expressions with a new function. I'm generally wary of abstractions that increase the amount of code rather than decrease it.

In particular, I think creating a wrap function which just calls NonNull::from in the static ref case is really abstracting nothing - its just a function that calls a function. Especially remember that these are all safe functions. I assume wrap was added for symmetry, but then you deleted clone, making static ref's impl even less symetric. I'm most strongly disinclined to merge the wrap function here.

The other two wraps are a little more complicated, but I don't find the code they're unifying particularly hard to audit and so I don't really see the advantage of not repeating it. However, what I noticed is that they're not actualizing the real win that is possible here: both of these wrap functions are 100% safe, and could be marked safe.

Creating a NonNull<T> from an Rc<T> or an Arc<T> is completely safe, because Rc and Arc both guarantee that their pointers are non-null. The advantage of these wrap functions could be that it asserts that this particular bit of code is supposed to be a safe conversion, documenting that fact for future reference.

But I'd also point out that given that these are safe, these conversions should really be available in the standard library. I was surprised at how limited our APIs for safely converting smart pointers to NonNulls are, and I think its just an oversight. All we have is the ability to convert a Box using the incredibly verbose and unstable Box::into_raw_non_null. I haven't investigated, but if it hasn't already been rejected for some reason, I would really like to see NonNull<T> implement From<Box<T>>, From<Rc<T>> and From<Arc<T>>, the same way it implements these for references.

In other words, I think I'd be inclined to accept the wrap functions if they were marked safe, only for Rc and Arc, but I'd also like to start moving toward providing this functionality through the standard library instead of writing the abstraction ourselves.

[dwijnand]

Makes sense to me, I'll resubmit. Thanks for sharing your thoughts, boats!

[dwijnand]

Btw, I'm surprised Clippy doesn't have a check for an unnecessary unsafe modifier.

[withoutboats]

@dwijnand its pretty difficult to tell that the unsafe is unnecessary, because a function could provide a totally safe operation that is only unsafe in the context of other methods on the same object. For example, Vec::set_len is totally unsafe, but all it does is assign the len to a new value you pass in (a safe operation in isolation).

[dwijnand]

Ah, I see. Makes sense.

[dwijnand]

Creating a NonNull<T> from an Rc<T> or an Arc<T> is completely safe, because Rc and Arc both guarantee that their pointers are non-null. The advantage of these wrap functions could be that it asserts that this particular bit of code is supposed to be a safe conversion, documenting that fact for future reference.

But I'd also point out that given that these are safe, these conversions should really be available in the standard library. I was surprised at how limited our APIs for safely converting smart pointers to NonNulls are, and I think its just an oversight. All we have is the ability to convert a Box using the incredibly verbose and unstable Box::into_raw_non_null. I haven't investigated, but if it hasn't already been rejected for some reason, I would really like to see NonNull<T> implement From<Box<T>>, From<Rc<T>> and From<Arc<T>>, the same way it implements these for references.

In other words, I think I'd be inclined to accept the wrap functions if they were marked safe, only for Rc and Arc, but I'd also like to start moving toward providing this functionality through the standard library instead of writing the abstraction ourselves.

Hey @withoutboats,

So I tried making this addition but I couldn't figure out where to put them, with NonNull in libcore and Rc/Arc in liballoc. I tried both. Any ideas? I also asked on Zulip.

rust-lang/rust@master...dwijnand:impl-From-Arc-for-NonNull

[withoutboats]

@dwijnand It needs to be in liballoc because libcore doesn't have access to Rc/Arc, but you can't impl From there because of the way the orphan rules currently work (though I think one of the RFCs currently under consideration would change this). There are a few ways to handle this:

• Add inherent methods like Box currently has, though I would probably name them into_non_null instead of into_raw_non_null because that name is quite long
• Add Into implementations instead of From. From is preferred over Into, but if you can't write it because of the orphan rules, that's why Into exists.

(I also think the division of libstd into these different crates instead of using cfg flags is sort of a bad design choice specifically because of these orphan rules problems, but that would be very hard to change at this point.)

[dwijnand]

Thanks boats! I'll try Into, and if not fall back to the inherent.