-
Notifications
You must be signed in to change notification settings - Fork 6
Fix make_guard!() unsoundness in the presence of diverging code afterwards
#16
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
20d7a4b
08aea6c
1879f58
3699437
496dbec
e104bcc
fa71065
d2f62c1
aeaf376
698c6e2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -187,9 +187,108 @@ macro_rules! make_guard { | |
| #[allow(unused)] | ||
| let lifetime_brand = unsafe { $crate::LifetimeBrand::new(&branded_place) }; | ||
| let $name = unsafe { $crate::Guard::new(branded_place) }; | ||
|
|
||
| // The whole following `if false {}` block has only one role: to handle | ||
| // the case where follow-up code might diverge. | ||
| // See https://github.com/CAD97/generativity/issues/15 for more info. | ||
| if false { | ||
| #[allow(unreachable_code)] { | ||
| let phantom_ret_ty = $crate::__private::PhantomReturnType::<_>::NEW; | ||
| if false { | ||
| // Use inference to set the type parameter to that of the return type. | ||
| return $crate::__private::DefaultReify::reify(&phantom_ret_ty); | ||
| } | ||
|
|
||
| // Guarding against `PhantomReturnType<!>` itself does not suffice, | ||
| // we may be dealing with `PhantomReturnType<(!, !)>`, for instance. | ||
| // | ||
| // Observation: the very same mechanism which causes us trouble | ||
| // yields an `unreachable_code` warning in the following situation: | ||
| if false { | ||
| let _reified_ret = | ||
| $crate::__private::DefaultReify::reify(&phantom_ret_ty); | ||
| #[forbid(unreachable_code)] { | ||
| // any arbitrary statement works to trigger the lint. | ||
| if true {} | ||
| } | ||
| } | ||
|
|
||
| // Poorman's specialization; only shadowed in the | ||
| // `PhantomReturnType<!>` case. | ||
| // | ||
| // This is not strictly needed for soundness *per se*, since the above | ||
| // `forbid(unreachable_code)` takes care of that. | ||
| // | ||
| // But it greatly improves the diagnostics for the non-niche case. | ||
| #[allow(unused)] | ||
| use $crate::__private::DefaultReify as _; | ||
| return phantom_ret_ty.reify(); | ||
| } | ||
| } | ||
| }; | ||
| } | ||
|
|
||
| #[doc(hidden)] | ||
| /// NOT STABLE PUBLIC API. Used by the expansion of [`make_guard!`]. | ||
| pub mod __private { | ||
| pub struct PhantomReturnType<T>(::core::marker::PhantomData<T>); | ||
|
|
||
| impl<T> PhantomReturnType<T> { | ||
| pub const NEW: Self = Self(::core::marker::PhantomData); | ||
| } | ||
|
|
||
| /// Inlined [`::never-say-never`](https://docs.rs/never-say-never). | ||
| mod never_say_never { | ||
| pub trait FnPtr { type Never; } | ||
| impl<R> FnPtr for fn() -> R { type Never = R; } | ||
| } | ||
| type Never = <fn() -> ! as never_say_never::FnPtr>::Never; | ||
|
|
||
| /// Poorman's specialization for `Phony::<default T / override !>::get()`. | ||
| pub trait DefaultReify<T> { | ||
| /// Function to be used in dead code to reify/synthesize a `T` instance | ||
| /// out of our [`PhantomReturnType`]. | ||
| fn reify(&self) -> T { unreachable!() } | ||
| } | ||
|
|
||
| impl<T> DefaultReify<T> for PhantomReturnType<T> {} | ||
|
|
||
| impl PhantomReturnType<Never> { | ||
| /// Uncallable method, via an unmet predicate. | ||
| pub fn reify(&self) -> Never | ||
| where | ||
| // Clause needs to involve some lifetime parameter in order not to | ||
| // cause a `trivial_bounds` eager error. | ||
| // `for<'trivial>` is the "typical" workaround so far. | ||
| for<'trivial> Never : sealed::SupportedReturnType, | ||
| { | ||
| unreachable!() | ||
| } | ||
| } | ||
|
|
||
| mod sealed { | ||
| #[diagnostic::on_unimplemented( | ||
| message = "\ | ||
| `make_guard!()` cannot be used in a diverging/`!`-returning function\ | ||
| ", | ||
| label = "encompassing functions \"diverges\", e.g., returns `-> !`", | ||
| note = "\ | ||
| `make_guard!()` temporary and lifetime shenanigans, on which its soundness model hinges, \ | ||
| are broken whenever both a diverging expression follows the `make_guard!()` statement(s), \ | ||
| and also if the return type of the encompassing function is `!`, for technical reasons. \ | ||
| \n\ | ||
| \n\ | ||
| To this day, no workaround is known, so there is no other choice but to reject the \ | ||
| `-> !`-returning function case: it is quite niche, and sacrificing it allows every \ | ||
| other single instance of `make_guard!()` to remain sound.\ | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I feel like this note adds more noise than being helpful. I think it suffices to replace this with "see issue #15"
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We could add a "see 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fair enough, let's just wait for CD's opinion.
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
| \n\ | ||
| See https://github.com/CAD97/generativity/issues/15 for more info. | ||
| ", | ||
| )] | ||
| pub trait SupportedReturnType {} | ||
| } | ||
| } | ||
|
|
||
| #[cfg(test)] | ||
| mod test { | ||
| use super::*; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,17 @@ | ||
| use generativity::{Id, make_guard}; | ||
|
|
||
| fn assert_eq_lt<'id>(_: Id<'id>, _: Id<'id>) {} | ||
|
|
||
| fn diverging_fn() -> ! { | ||
| make_guard!(g_a); | ||
| make_guard!(g_b); | ||
|
|
||
| let a: Id = g_a.into(); | ||
| let b: Id = g_b.into(); | ||
|
|
||
| assert_eq_lt(a, b); | ||
|
|
||
| loop {} | ||
| } | ||
|
|
||
| fn main() {} |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,75 @@ | ||
| error: unreachable expression | ||
| --> tests/ui/diverging_fn.rs:6:5 | ||
| | | ||
| 6 | make_guard!(g_a); | ||
| | ^^^^^^^^^^^^^^^^ | ||
| | | | ||
| | unreachable expression | ||
| | any code following this expression is unreachable | ||
| | | ||
| note: the lint level is defined here | ||
| --> tests/ui/diverging_fn.rs:6:5 | ||
| | | ||
| 6 | make_guard!(g_a); | ||
| | ^^^^^^^^^^^^^^^^ | ||
| = note: this error originates in the macro `make_guard` (in Nightly builds, run with -Z macro-backtrace for more info) | ||
|
|
||
| error[E0277]: `make_guard!()` cannot be used in a diverging/`!`-returning function | ||
| --> tests/ui/diverging_fn.rs:6:5 | ||
| | | ||
| 6 | make_guard!(g_a); | ||
| | ^^^^^^^^^^^^^^^^ encompassing functions "diverges", e.g., returns `-> !` | ||
| | | ||
| = help: the trait `__private::sealed::SupportedReturnType` is not implemented for `!` | ||
| = note: `make_guard!()` temporary and lifetime shenanigans, on which its soundness model hinges, are broken whenever both a diverging expression follows the `make_guard!()` statement(s), and also if the return type of the encompassing function is `!`, for technical reasons. | ||
|
|
||
| To this day, no workaround is known, so there is no other choice but to reject the `-> !`-returning function case: it is quite niche, and sacrificing it allows every other single instance of `make_guard!()` to remain sound. | ||
| See https://github.com/CAD97/generativity/issues/15 for more info. | ||
|
|
||
| note: required by a bound in `PhantomReturnType::<<fn() -> ! as __private::never_say_never::FnPtr>::Never>::reify` | ||
| --> src/lib.rs | ||
| | | ||
| | pub fn reify(&self) -> Never | ||
| | ----- required by a bound in this associated function | ||
| ... | ||
| | for<'trivial> Never : sealed::SupportedReturnType, | ||
| | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ required by this bound in `PhantomReturnType::<<fn() -> ! as FnPtr>::Never>::reify` | ||
| = note: this error originates in the macro `make_guard` (in Nightly builds, run with -Z macro-backtrace for more info) | ||
|
|
||
| error: unreachable expression | ||
| --> tests/ui/diverging_fn.rs:7:5 | ||
| | | ||
| 7 | make_guard!(g_b); | ||
| | ^^^^^^^^^^^^^^^^ | ||
| | | | ||
| | unreachable expression | ||
| | any code following this expression is unreachable | ||
| | | ||
| note: the lint level is defined here | ||
| --> tests/ui/diverging_fn.rs:7:5 | ||
| | | ||
| 7 | make_guard!(g_b); | ||
| | ^^^^^^^^^^^^^^^^ | ||
| = note: this error originates in the macro `make_guard` (in Nightly builds, run with -Z macro-backtrace for more info) | ||
|
|
||
| error[E0277]: `make_guard!()` cannot be used in a diverging/`!`-returning function | ||
| --> tests/ui/diverging_fn.rs:7:5 | ||
| | | ||
| 7 | make_guard!(g_b); | ||
| | ^^^^^^^^^^^^^^^^ encompassing functions "diverges", e.g., returns `-> !` | ||
| | | ||
| = help: the trait `__private::sealed::SupportedReturnType` is not implemented for `!` | ||
| = note: `make_guard!()` temporary and lifetime shenanigans, on which its soundness model hinges, are broken whenever both a diverging expression follows the `make_guard!()` statement(s), and also if the return type of the encompassing function is `!`, for technical reasons. | ||
|
|
||
| To this day, no workaround is known, so there is no other choice but to reject the `-> !`-returning function case: it is quite niche, and sacrificing it allows every other single instance of `make_guard!()` to remain sound. | ||
| See https://github.com/CAD97/generativity/issues/15 for more info. | ||
|
|
||
| note: required by a bound in `PhantomReturnType::<<fn() -> ! as __private::never_say_never::FnPtr>::Never>::reify` | ||
| --> src/lib.rs | ||
| | | ||
| | pub fn reify(&self) -> Never | ||
| | ----- required by a bound in this associated function | ||
| ... | ||
| | for<'trivial> Never : sealed::SupportedReturnType, | ||
| | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ required by this bound in `PhantomReturnType::<<fn() -> ! as FnPtr>::Never>::reify` | ||
| = note: this error originates in the macro `make_guard` (in Nightly builds, run with -Z macro-backtrace for more info) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,18 @@ | ||
| use generativity::{Id, make_guard}; | ||
| use never_say_never::Never; | ||
|
|
||
| fn assert_eq_lt<'id>(_: Id<'id>, _: Id<'id>) {} | ||
|
|
||
| fn diverging_fn_cursed() -> (Never, Never) { | ||
| make_guard!(g_a); | ||
| make_guard!(g_b); | ||
|
|
||
| let a: Id = g_a.into(); | ||
| let b: Id = g_b.into(); | ||
|
|
||
| assert_eq_lt(a, b); | ||
|
|
||
| loop {} | ||
| } | ||
|
|
||
| fn main() {} |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,41 @@ | ||
| error: unreachable expression | ||
| --> tests/ui/diverging_fn_cursed.rs:8:5 | ||
| | | ||
| 8 | make_guard!(g_b); | ||
| | ^^^^^^^^^^^^^^^^ | ||
| | | | ||
| | unreachable expression | ||
| | any code following this expression is unreachable | ||
| | | ||
| note: this expression has type `(!, !)`, which is uninhabited | ||
| --> tests/ui/diverging_fn_cursed.rs:8:5 | ||
| | | ||
| 8 | make_guard!(g_b); | ||
| | ^^^^^^^^^^^^^^^^ | ||
| note: the lint level is defined here | ||
| --> tests/ui/diverging_fn_cursed.rs:8:5 | ||
| | | ||
| 8 | make_guard!(g_b); | ||
| | ^^^^^^^^^^^^^^^^ | ||
| = note: this error originates in the macro `make_guard` (in Nightly builds, run with -Z macro-backtrace for more info) | ||
|
|
||
| error: unreachable expression | ||
| --> tests/ui/diverging_fn_cursed.rs:7:5 | ||
| | | ||
| 7 | make_guard!(g_a); | ||
| | ^^^^^^^^^^^^^^^^ | ||
| | | | ||
| | unreachable expression | ||
| | any code following this expression is unreachable | ||
| | | ||
| note: this expression has type `(!, !)`, which is uninhabited | ||
| --> tests/ui/diverging_fn_cursed.rs:7:5 | ||
| | | ||
| 7 | make_guard!(g_a); | ||
| | ^^^^^^^^^^^^^^^^ | ||
| note: the lint level is defined here | ||
| --> tests/ui/diverging_fn_cursed.rs:7:5 | ||
| | | ||
| 7 | make_guard!(g_a); | ||
| | ^^^^^^^^^^^^^^^^ | ||
| = note: this error originates in the macro `make_guard` (in Nightly builds, run with -Z macro-backtrace for more info) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,15 @@ | ||
| use generativity::{Id, make_guard}; | ||
|
|
||
| fn assert_eq_lt<'id>(_: Id<'id>, _: Id<'id>) {} | ||
|
|
||
| fn main() { | ||
| make_guard!(g_a); | ||
| make_guard!(g_b); | ||
|
|
||
| let a: Id = g_a.into(); | ||
| let b: Id = g_b.into(); | ||
|
|
||
| assert_eq_lt(a, b); | ||
|
|
||
| loop {} | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,14 @@ | ||
| error[E0716]: temporary value dropped while borrowed | ||
| --> tests/ui/issue_15_panic_dropck.rs:7:5 | ||
| | | ||
| 7 | make_guard!(g_b); | ||
| | ^^^^^^^^^^^^^^^^ creates a temporary value which is freed while still in use | ||
| ... | ||
| 15 | } | ||
| | - | ||
| | | | ||
| | temporary value is freed at the end of this statement | ||
| | borrow might be used here, when `lifetime_brand` is dropped and runs the `Drop` code for type `LifetimeBrand` | ||
| | | ||
| = note: consider using a `let` binding to create a longer lived value | ||
| = note: this error originates in the macro `make_guard` (in Nightly builds, run with -Z macro-backtrace for more info) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You probably meant to remove this since you added
never-say-neveras a dependencyThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As a
dev-dependency only: I imagine this crate would like to keep its 0-dependencies aspect (but that's for @CAD97 to decide).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah — if the
Neverwere 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.