[experiment] Make Cell<T>::update work for T: Default | Copy.

[m-ou-se]

This uses some unstable trickery to implement Cell::update for types that are either Copy or Default or both, and use the .get() + .set() behaviour if possible (if it is Copy), or use .take() + .set() otherwise (if it is Default and not Copy).

See #50186 (comment)

[m-ou-se]

Who would be the right person to evaluate soundness of hacks like this?

@matthewjasper maybe?

[eddyb]

My notes so far: thanks to CopyOrDefault, the only way Cell::update could be unsound is if both traits are implemented, but with different imposed relationships on lifetimes.

However, that means Copy is used, and Default isn't called. So this is just as unsound as any other specialization on Copy.

IOW, when Default::default gets called, T: CopyOrDefault is equivalent to T: Default and any requirements from the Default impl will be imposed on the caller of Cell::update.

[danielhenrymantilla]

Yes, this is right: IsDefault here is used to specialize over nothing.

In that regard, I find that writing the overriding impl before the default one, and slightly changing the naming of the things involved, can improve the readability of the code, as I've showcased in this Playground _{^{(I've also nested some of the trait definitions and impls, which as a bonus scopes the unsafe_specialization_marker, since I can fathom the branches better that way, but I understand that that's debatable. I stand by the order of my impls with the /* + !Trait */ annotations, though)}}

Extra comments / remarks regarding the soundness here

So, in the linked Playground one can see that the "parent impl" for the IsDefault specialization is CopyOrDefault, with the specialized Copy impls stripped. This means that we are only dealing with Default types in practice (modulo Copy specialization issues), so the specialization is effectively specializing over the whole subset / it is as if there effectively were a Default supertrait. That's why I consider this IsDefault specialization, when contained to this very usage, sound. But that does assume that the Copy specialization is fine to begin with, which it may technically not be.

• For instance, if the Copy specialization were not to kick in for a Copy + !Default type, could we reach the unreachable! impl? 🤔

• So, if anything, the only potential issue / area I'm not that confident about is whether a "flaw" with the Copy specialization could be "amplified" by allowing a different "flaw" with the IsDefault specialization.

• And even then, I could only see that causing "unsoundness" if we were to guarantee, at the API level, that for Copy + Default types update() shall necessarily Copy (because a hypothetical failed application of Copy specialization could break that guarantee). That's why, unless this situation is cleared up of all suspicion, I think we could loosen that guarantee to something like mem::needs_drop() and other such stuff:

  • For Copy + Default types, the "using get()" optimization is "likely" to be favored, but it is not guaranteed to do so: unsafe code cannot rely on this for soundness.

[comex]

It may be sound in this particular case, but isn't this basically a… I guess you could say soundness hole, in rustc_unsafe_specialization_marker? To quote the PR that created the attribute:

This is unsound but we allow it in the short term because it can't cause use after frees with purely safe code in the same way as specializing on traits methods can.

So it's not completely sound, but it's in some sense 'less unsound' than unrestricted specialization because you can't use it to call methods.

Except you are using it to call methods, by taking advantage of the supertrait Default, from a specialized impl (impl<T: IsDefault> DefaultSpec for T) whose parent impl does not have a Default bound.

With that ability, how is this any different from unrestricted specialization? In other words, what dangerous thing could you do by specializing on Default (which is not allowed) that you cannot do by creating a wrapper trait like this IsDefault and specializing on that? It seems like the answer is 'nothing', in which case the idea of rustc_unsafe_specialization_marker being limited to marker traits that have no methods is meaningless. But I may be missing something.

I note that in the case of some existing rustc_unsafe_specialization_marker traits, such as FusedIterator and TrustedLen, they do have supertraits – in both cases, Iterator – but impls that specialize on them always(?) have parent impls that are already bounded on Iterator. On the other hand, another such trait, MarkerEq, as used in RcEqIdent and ArcEqIdent, is the same kind of wrapper as IsDefault. It even has a comment saying:

Hack to allow specializing on Eq even though Eq has a method.

[matthewjasper]

I think this might be a soundness hole in the current min_specialization impl that I didn't get around to fixing.

[matthewjasper]

I've opened #89413 for this

[dtolnay]

I am 👍 on this overlapping marker trait + specialization approach if it works and is sound.

I tried it though, and saw some confusing behavior. Any idea what the deal is here? This code compiles on current nightly, and I believe you're intending for the PR to be strictly more general than what is now in nightly.

#![feature(cell_update)]

use std::cell::Cell;

#[derive(Copy, Clone, Default)]
struct Thing<'a>(&'a str);

fn main() {
    let cell: Cell<Thing> = Cell::new(Thing(""));
    cell.update(|Thing(_)| -> Thing { panic!() });
}

error[E0283]: type annotations needed for `Thing<'_>`
  --> src/main.rs:10:18
   |
10 |     cell.update(|Thing(_)| -> Thing { panic!() });
   |                  ^^^^^^^^ consider giving this closure parameter a type
   |
   = note: cannot satisfy `Thing<'_>: cell::get_or_take::CopyOrDefault`

[m-ou-se]

I suppose the problem is that either Copy or Default could be implemented only for 'static:

impl Copy for Thing<'static> {}

or

impl Default for Thing<'static> {
    fn default() -> Self {
        Self("shark")
    }
}

Overlapping trait implementations and lifetimes don't mix perfectly, it seems.

[Dylan-DPC]

Closing this as it was an experiment