Fix make_guard!() unsoundness in the presence of diverging code afterwards

[danielhenrymantilla]

• Tentative fix for Duplicate brands with dead-code drop calls #15

(Haven't had the time to properly clean up the implementation.)

This works off @CAD97's idea of the if false { return None.unwrap(); }.

As they pointed out, that technique will not work if the inferred type is itself !, since then our phony unreachable instance synthesizer (phony()) will just be perceived as a !-synthesizer function, i.e., yet another diverging function!

We work around that by using poorman's specialization to detect that case, and straight up refusing to work then: (slightly) ⚠️ breaking change ⚠️.

[CAD97]

(oops, pressed the wrong button)

[ArhanChaudhary]

This is incredible; can you walk me through how this works?

[ArhanChaudhary]

Also why doesn't

if false { return phony() }
fn phony<T>() -> T { unreachable! () }

work? Isn't it still basically zero cost?

[CAD97]

That's basically what's being done here; the difference in this trick is that we make sure that in the case of phony::<!> we instead create a compiler error. This is because if phony returns !, the exact same hole as with any other !-producing expression get hit.

[ArhanChaudhary]

FWIW with a __private module you might as well move LifetimeBrand inside of there too.

[CAD97]

(sorry for force-pushing over your commits but i wanted to fix the CI staleness issues and autopiloted the rebase process)

[dvdhrm]

This looks like a neat trick. Some minor comments on the implementation, but those are just my personal taste/experience.

Anyway, this PR is not sufficient. This still compiles (notice the (Never, Never)):

use generativity::{Id, make_guard};
use never_say_never::Never;

fn test<'id>(a: Id<'id>, b: Id<'id>) {
    print!("{:?} and {:?}\n", a, b);
    assert_eq!(a, b);
}

fn foo() -> (Never, Never) {
    make_guard!(g_a);
    make_guard!(g_b);

    let a: Id = g_a.into();
    let b: Id = g_b.into();

    test(a, b);

    loop {}
}

fn main() {
    foo();
}

[dvdhrm]

The first Never, is not needed, is it? Works for me without it.

[danielhenrymantilla]

It makes Phony<T> be a ZST.

[dvdhrm]

Ahhhh, _PhantomVariant becomes uninhabited that way. I see.

[danielhenrymantilla]

I've added a comment to clarify this point; mainly, that the payloads making, together, a ZST uninhabited type are what allows the enum to elide that variant off its layout altogether. To clarify, a non-ZST uninhabited payload, such as with PhantomVariant(Never, u8), for instance, would not suffice to make the variant be elidable ⚠️.

[danielhenrymantilla]

-> (Never, Never) still compiles

Oh, using Never against me, how ironic 😄

More seriously, this observation kills this whole attempt dead in its tracks, I'd say 😕. I know of no hacks to detect whether a type has a field of type !, so it looks like there will always be ways to come up with types that cause Rust to skip drop-checking and borrow-checking, alas 😔

[ArhanChaudhary]

Surely there has to be a way...

[ArhanChaudhary]

TBH, this might even warrant discussion to change the behavior of the already unstable drop check for divergent branches. I find this to be very non-intuitive and I would be surprised if crater showed any regressions at all.

[ArhanChaudhary]

That's basically what's being done here; the difference in this trick is that we make sure that in the case of phony::<!> we instead create a compiler error. This is because if phony returns !, the exact same hole as with any other !-producing expression get hit.

Okay, can someone elaborate further why this doesn't work? I did a quick test and it still fails correctly despite the divergent branch.
https://play.rust-lang.org/?version=stable&mode=debug&edition=2024&gist=ef4d54852a44b1d65da59c97f0e587f3

EDIT: I think I understand. It's only when the function has an explicit -> ! return type!

[dvdhrm]

-> (Never, Never) still compiles

Oh, using Never against me, how ironic 😄

Sorry 😔.

More seriously, this observation kills this whole attempt dead in its tracks, I'd say 😕. I know of no hacks to detect whether a type has a field of type !, so it looks like there will always be ways to come up with types that cause Rust to skip drop-checking and borrow-checking, alas 😔

Again. Sorry.

I wonder, is there some way to make the compiler yield ! if an expression is of any type that contains !? For instance, crafting an enum with uninhabited variant like you did, but instead of using Never, you use the inferred return type of the calling context. If the enum ends up as a ZST, it must be some kind of never type, so we prevent the macro-evaluation. In all other cases, we use the return-trick as proposed?

[danielhenrymantilla]

(nothing to be sorry about 😄)

Oh that's an interesting and very promising idea @dvdhrm! I think I can make that work 😈

[ArhanChaudhary]

It is worth noting that &'static Never and (Never, usize) are valid return type signatures and have pointer sized sizes. I am more concerned about the former case. References to ZSTs will likely never be considered zero-sized, and the behavior of &! in particular is already really nebulous.

[danielhenrymantilla]

So, my original idea, alas, did not pan out (I wanted to try to make use of never type fallback).

But then I realized that we do have a way to deny types à la (!, !): the very aspect which rustc sees in them to mess with our lifetime shenanigans, is that which also makes Rust yield unreachable_code lints about code that follows a witnessed instance of these types.

So all we had to do was forbid(unreachable_code), and voilà, the impl of this PR is made robust.

• the "drawback" is that, contrary to poorman's ! with unmet trait bound with diagnostic::on_unimplemented(), is that we do not get to customize the error message. So the DX here is definitely bad.

  With that being said, a (!, !) returning function feels more adversarial than plausible, and for those scenarios being sound even with bad DX, is what matters 🙂

[danielhenrymantilla]

I'm aware we end up in a rather silly #[allow(X)] … #[forbid(X)] situation, here. I've considered removing the allow, but then the lint fires a bunch of times. The way it is done currently, it fires exactly once, so it does yield slightly better DX.

[ArhanChaudhary]

You probably meant to remove this since you added never-say-never as a dependency

[danielhenrymantilla]

As a dev-dependency only: I imagine this crate would like to keep its 0-dependencies aspect (but that's for @CAD97 to decide).

[CAD97]

Yeah — if the Never were part of the critical public API, I'd make the semi-canonical crate the way we achieve it. But since it's purely to enhance diagnostics, inlining the trick to name the type seems preferable to me.

[ArhanChaudhary]

I would name the trait to something like Retifier and the trait method to retify

[danielhenrymantilla]

Heh, I now find .reify() to be a rather nice method name! The ret part, however, is always external/outside this type in and of itself (return phantom.reify();).

• 🤔 tangentially, we could rename Phony as Phantom, or even PhantomReturnType: return phantom_ret.reify();

EDIT: went with that in e104bcc (#16)

[ArhanChaudhary]

I feel like this note adds more noise than being helpful. I think it suffices to replace this with "see issue #15"

[danielhenrymantilla]

We could add a "see https://github.com/CAD97/generativity/issues/15 for more info" at the end.

Whilst I do personally think we should not underestimate the burden of having to click a link while having internet connectivity, I am amenable to reducing the note to just the link reference, if that's what ends up preferred.

[ArhanChaudhary]

Fair enough, let's just wait for CD's opinion.

[CAD97]

Since this error should show up super rarely, I'm of the opinion that getting more context inline is preferable; it also serves as in-VCS record of the reasoning.

For anything more prevalent, I'd make it more concise. But it's also worth noting that we can't provide an --explain extended description, so our trade-off will be different than rustc/clippy.

[Random-Scientist]

I was talking with @ArhanChaudhary in the discord and realized that you can avoid passing a reference into LifetimeBrand::new in reachable code, forcing the reachable match branch (which purely summons a ZST) to unify its inferred lifetime with the return type of the proper new function without ever evaluating it.

#[doc(hidden)]
/// NOT STABLE PUBLIC API. Used by the expansion of [`make_guard!`].
impl<'id> LifetimeBrand<'id> {
    // ...
    #[doc(hidden)]
    #[inline(always)]
    /// NOT STABLE PUBLIC API. Used by the expansion of [`make_guard!`].
    pub unsafe fn new_inferred() -> LifetimeBrand<'id> {
        LifetimeBrand {
            phantom: PhantomData,
        }
    }
}
macro_rules! make_guard {
    ($name:ident) => {
        // ...
        let branded_place = unsafe { $crate::Id::new() };
        #[allow(unused)]
        let lifetime_brand = match () {
            _ => unsafe { $crate::LifetimeBrand::new_inferred() },
            _ => unsafe { $crate::LifetimeBrand::new(&branded_place) },
        };
        let $name = unsafe { $crate::Guard::new(branded_place) };
        // ...
    };
}

[CAD97]

FWIW, the original formulation didn't even initialize the lifetime_brand place at all, roughly

let lifetime_brand;
if false {
    let lifetime_brand = unsafe { $crate::LifetimeBrand::new(&branded_place) };
}

That level of "optimizing" the executed path is a net negative, though. We're not even relying on LLVM to optimize out the immediately discarded reference anymore; rustc's MIR opts inlines and removes that already. Anything more complicated than the obvious construction is just making it more complicated to understand and giving the compiler more code that it has to process.

For a similar reason, the fake specialization for -> ! should have a comment explaining that it only is part of the macro to improve the error messages and could be completely removed; that the #[forbid(unreachable_code)] is the actual protection against excluding drop glue from borrowck.

But legitimately, thanks all for finding a solution so quickly!

[ArhanChaudhary]

FWIW with a __private module you might as well move LifetimeBrand inside of there too.

Also we might as well just do this before merging to be consistent </bikeshed>

[CAD97]

Thanks again for this! Gonna merge this as-is now, then take my own pass at trying to micro optimize clarity both in technique and in the custom diagnostic once I get a chance to later this week.

[CAD97]

or, well, once I get back to my main computer so I can bless the trybuild test again. mobile isn't letting me manually bypass 🙃

[ArhanChaudhary]

I was talking to @danielhenrymantilla on Discord and they showed me a way to get rid of the called-within-a--> ! enscoping function improving the diagnostic making the implemplentation simpler (I wasn't able to get this working though; still waiting for their response).

// The following `if false {` block has two roles:
//  1. to guard against a follow-up diverging expression messing up with the
//     borrowck+dropck soundness design of `make_guard!()`;
//  2. to trigger a compile error when invoked from within a
//     known-diverging function (_e.g._, `-> !`, or even `-> (!, !)`, etc.),
//     as a last resort, since that case breaks `1.`
if false {
    #[allow(warnings, clippy::all)]
    let return_value = ::core::option::Option::None.unwrap();
    #[forbid(unreachable_code)] {
        // if `return_value: !`, or `: (!, !)` etc., then Rust will
        // know what follows is `unreachable_code` (it's the same mechanism
        // which breaks `1.` to begin with!)
        return return_value;
    }
}

According to them this should remove PhantomReturnType, .reify(), and the "poorman's specialization." I think this would definitely be worth looking into before merging!

[CAD97]

It should be possible, I'll definitely give it a try before publish. But I think that if it's kept fully orthogonal, the diagnostic call-out is worth the extra code.

I'll potentially stick it behind a feature flag to make the fact that it's unnecessary niceness more obvious, but it's worth including here as a semi-canonical source of lifetime branding.

FWIW, in the absence of the divergence hole, I would've generally recommended inlining the technique in a use case that wants branding, for API accessibility reasons to provide your own decl macro(s). With the extra handling necessary, that calculus changes.

[ArhanChaudhary]

Whoops sorry I should have said this earlier. @danielhenrymantilla showed me a version of generativity that works in a way simpler fashion.

use ::core::marker::PhantomData as PD;

pub
struct Guard<'id>(PD<fn(&'id ()) -> &'id ()>);

#[doc(hidden)] /** Not part of the public API */ pub
mod __private {
  use super::*;
  
  pub
  trait GuardConstructor<'id> {
      fn get(_: &Self) -> Guard<'id> {
          Guard(PD)
      }
  }
  
  /// Invariant (thanks to RPIT).
  ///
  /// Shall borrow the above instance for `'id`, so we get:
  ///
  /// > `'lifespan_of_borrowee_instance : 'id`
  ///
  /// Return has `impl drop_use<'id>`, which means:
  /// > `'id : 'lifespan_of_this_ret_instance`.
  #[inline]
  pub
  fn phantom_borrower<'id>(_maximal_borrow: &'id ())
    -> impl GuardConstructor<'id>
  {
      struct Helper {}
      impl GuardConstructor<'_> for Helper {}
      Helper {}
  }
  
  pub use {None, Some};
}

#[macro_export]
macro_rules! make_guard {($var:ident) => (
    // `'lifespan_of_borrowee : 'borrows_thereof`, so `borrowee`'s lifespan acts
    // as an upper bound.
    let borrowee = ();
    //              lifetime of this borrow is stored in `guard_yielder`
    //                                                      v
    let guard_yielder = $crate::__private::phantom_borrower(&borrowee);
    // it is invariant, and:
    //   - cannot span beyond the life-span of `borrowee`,
    //   - *nor below* that of `guard_yielder`, since it is `drop_use<'id>`.
    let $var = $crate::__private::GuardConstructor::get(&guard_yielder);
    if false {
      let infer = $crate::__private::None;
      if let $crate::__private::Some(value) = infer {
          // Make rust deduce the type of `return_value`.
          return value;
      } else {
          //            may be uninhabited
          //                 v
          let _return_value: _ = infer.unwrap();
          // If this is uninhabited enough to mess with our life-span
          // shenanigans above, then the following will be deemed unreachable.
          #[forbid(unreachable_code)] {
              if true {}
          }
      }
    }
)}

[CAD97]

I separately minimized the guard to just

#[forbid(unreachable_code)]
if false {
    let phantom_return = $crate::__private::PhantomData;
    let _ = $crate::__private::ghostbust(phantom_return);
    return $crate::__private::ghostbust(phantom_return);
}

// with
pub fn ghostbust<T>(_: PhantomData<T>) -> T {
    unreachable!()
}

As for the alternate way to create the brand lifetime... I personally think relying on the behavior of RPIT is clever, but for the same reason it's less clear why the lifetime is properly tied. I'm probably going to stick to the current lifetime tie method.

[ArhanChaudhary]

@danielhenrymantilla brought to my attention the --cap-lints flag which will make lints set to "forbid" just warn. We rely on this explicit behavior for soundness; hence, I believe we still have a soundness error. One way to fix this is do this

fn drop_glue(_: impl Sized) -> impl Sized {}

fn main() -> ! {
    let a1 = drop_glue(());
    let a2 = drop_glue(&a1);
    let b1 = drop_glue(());
    let b2 = drop_glue(&b1);
    [&a2, &b2]; // Error, distinct (inner) lifetimes.
    loop {}
}

@CAD97 should we re-open #15, and figure out another PR to get this in check?

[ArhanChaudhary]

So I looked into the issue once again, and it turns out the --cap-lints warn flag still triggers the error. I would be lying if I said I knew why. But what matters is that this was sound after all, hooray!