std: Rename thread::catch_panic to panic::recover #29937

alexcrichton · 2015-11-19T19:37:32Z

This commit is an implementation of RFC 1236 and RFC 1323 which
rename the thread::catch_panic function to panic::recover while also
replacing the Send + 'static bounds with a new PanicSafe bound.

cc #27719

rust-highfive · 2015-11-19T19:37:43Z

r? @brson

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

steveklabnik · 2015-11-19T21:36:31Z

src/libstd/panic.rs

+
+//! Panic support in the standard library
+
+#![unstable(feature = "std_panic", reason = "module recently added",


so, we do this a lot, but random thought: it's been weird to see some long-living feature gates with "recently added" as a reason.

dunno if there should be a better reason or not, just musing

Ditto. Maybe "awaiting feedback"?

With our new issue tracking process, the reason for unstable items is less relevant; we could consider nixing it in favor of the issue number.

eefriedman · 2015-11-20T07:02:56Z

Are Arc<i32> and &Arc<i32> supposed to be PanicSafe?

elinorbgr · 2015-11-20T08:49:45Z

src/libstd/panic.rs

+impl<'a, T: NoUnsafeCell + ?Sized> PanicSafe for *const T {}
+impl<'a, T: NoUnsafeCell + ?Sized> PanicSafe for *mut T {}
+impl<'a, T: PanicSafe> PanicSafe for Unique<T> {}
+impl<T: ?Sized> PanicSafe for Mutex<T> {}


Isn't RwLock poisoned as well, hence PanicSafe ?

Good catch!

alexcrichton · 2015-11-20T17:10:01Z

@eefriedman not currently, mainly because you can have Arc<RefCell<T>> which isn't panic safe.

brson · 2015-11-24T00:02:29Z

src/libstd/panic.rs

+impl<'a, T: ?Sized> !PanicSafe for &'a mut T {}
+impl<'a, T: NoUnsafeCell + ?Sized> PanicSafe for &'a T {}
+impl<'a, T: NoUnsafeCell + ?Sized> PanicSafe for *const T {}
+impl<'a, T: NoUnsafeCell + ?Sized> PanicSafe for *mut T {}


Unsafe pointers are not listed as PanicSafe in the RFC. Does this say that user-defined types that contain unsafe pointers to panic-safe things are automatically panic-safe? Why is that ok? Unsafe pointers are the only negative impls for the other OIBITS.

On the point of safety, using Safe in the name is somewhat confusing/overloaded; if this trait isn't unsafe, maybe we could name it something else? (I don't personally have any good suggestions, though. :( )

Hm, I'm surprised that was missing from the RFC; it was a key part of the earlier discussion. It's important for the FFI case in particular, and since using the pointers requires unsafe code anyway the need for tailored assertions like AssertPanicSafe is lessened.

Remember that this trait is unlike the other OIBITs, by design, in several respects -- this being one of them. It's also a safe trait to implement. Like the existing approach to mitigating panic safety issues, the trait acts primarily as a "speed bump" when isolation boundaries may be violated.

brson · 2015-11-24T00:32:43Z

I continue to believe this design is needlessly complicated by the misguided decision to narrow the meaning of 'unsafe' to 'memory unsafe'. Exception-safety is largely about memory safety, and with this design 'panic safety' actually is only a tiny slice of exception-safety. It's very misleading - it actually doesn't have anything to do with panicking safely, but recovering safely.

I'm not going to be the one to approve this.

aturon · 2015-11-24T08:09:04Z

src/libstd/panic.rs

+use sys_common::unwind;
+use thread::Result;
+
+/// A marker trait which represents "panic safe" types in Rust.


Discussion on IRC suggested that this might be better called "recover safe". Is it worth trying to land with that naming (avoiding churn), or doing that through the FCP/comment process?

Seems fine to me to go ahead and land, tweaks to naming are pretty easy to do down the road.

aturon · 2015-11-24T08:39:50Z

@brson

The design is part of a long-going RFC process (including the lengthy thread for rust-lang/rfcs#1236) which tackled the issue of what safety covers in Rust (see e.g. my comment there).

This PR represents the consensus design balancing between those worried about panic safety problems as well as those primarily concerned with ergonomics. During the RFC process you raised concerns about introducing a new OIBIT, but these concerns were addressed by the RFC text and design at least to the level that the relevant teams made a consensus decision to land the feature experimentally, as in this PR.

It's vital that all team members abide by the consensus decisions we reached based on public discussion, even in cases where they personally disagree with the outcome. Consensus-based decision making is a fragile thing, and we all need to be operating openly and in good faith. Keep in mind that as always, there will be ample time to discuss this feature in the light of real-world experience on the tracking issue prior to stabilization.

If there is an immediately actionable tweak to the design -- such as the proposed renaming to RecoverSafe -- it's worth considering trying to land a tweaked version directly. The concerns you are raising here seem to be at least twofold -- the scope of "safety" in Rust and the "narrowing" of panic safety. The scoping issue I already mentioned (with relevant comment thread). The narrowing issue, I don't yet understand, but at this point discussion of the (de)merits of the feature in general (as opposed to minor design tweaks) should take place on the tracking issue.

aturon · 2015-11-24T08:41:15Z

@alexcrichton I've done an initial review pass, and made several small suggestions about documentation, naming, and a couple about impls. Can we use NoUnsafeCell to make Arc<i32> panic safe but Arc<RefCell<T>> not?

alexcrichton · 2015-12-05T00:35:01Z

Updated with comments addressed,

r? @aturon

bors · 2015-12-06T06:50:46Z

☔ The latest upstream changes (presumably #30187) made this pull request unmergeable. Please resolve the merge conflicts.

aturon · 2015-12-06T06:54:15Z

r=me after a rebase.

alexcrichton · 2015-12-08T14:13:19Z

@bors: r=aturon

bors · 2015-12-08T14:13:20Z

📌 Commit c39af12 has been approved by aturon

This commit is an implementation of [RFC 1236] and [RFC 1323] which rename the `thread::catch_panic` function to `panic::recover` while also replacing the `Send + 'static` bounds with a new `PanicSafe` bound. [RFC 1236]: rust-lang/rfcs#1236 [RFC 1323]: rust-lang/rfcs#1323 cc #27719

bors · 2015-12-08T18:23:56Z

💔 Test failed - auto-mac-64-nopt-t

alexcrichton · 2015-12-08T20:20:51Z

@bors: r=aturon 19892fd

bors · 2015-12-08T23:45:03Z

⌛ Testing commit 19892fd with merge e0c4379...

bors · 2015-12-09T00:31:37Z

💔 Test failed - auto-mac-64-nopt-t

This commit is an implementation of [RFC 1236] and [RFC 1323] which rename the `thread::catch_panic` function to `panic::recover` while also replacing the `Send + 'static` bounds with a new `PanicSafe` bound. [RFC 1236]: rust-lang/rfcs#1236 [RFC 1323]: rust-lang/rfcs#1323 cc rust-lang#27719

alexcrichton · 2015-12-09T15:20:27Z

@bors: r+ 0a13f1a

bors · 2015-12-09T16:57:47Z

⌛ Testing commit 0a13f1a with merge 6bf8cc5...

This commit is an implementation of [RFC 1236] and [RFC 1323] which rename the `thread::catch_panic` function to `panic::recover` while also replacing the `Send + 'static` bounds with a new `PanicSafe` bound. [RFC 1236]: rust-lang/rfcs#1236 [RFC 1323]: rust-lang/rfcs#1323 cc #27719

bors · 2015-12-09T18:42:11Z

☀️ Test successful - auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-64-debug-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-x-android-t, auto-linux-cross-opt, auto-linux-musl-64-opt, auto-mac-32-opt, auto-mac-64-nopt-t, auto-mac-64-opt, auto-win-gnu-32-nopt-t, auto-win-gnu-32-opt, auto-win-gnu-64-nopt-t, auto-win-gnu-64-opt, auto-win-msvc-32-opt, auto-win-msvc-64-opt

rust-highfive assigned brson Nov 19, 2015

steveklabnik reviewed Nov 19, 2015
View reviewed changes

alexcrichton force-pushed the panic-recover branch 2 times, most recently from 1865a8f to cb4141f Compare November 19, 2015 22:44

elinorbgr reviewed Nov 20, 2015
View reviewed changes

alexcrichton force-pushed the panic-recover branch 2 times, most recently from a1c9ffc to db1631c Compare November 20, 2015 17:11

brson reviewed Nov 24, 2015
View reviewed changes

brson removed their assignment Nov 24, 2015

aturon reviewed Nov 24, 2015
View reviewed changes

alexcrichton force-pushed the panic-recover branch 2 times, most recently from 1e48118 to fc1a2bf Compare December 5, 2015 00:34

rust-highfive assigned aturon Dec 5, 2015

alexcrichton force-pushed the panic-recover branch from fc1a2bf to eb7e365 Compare December 5, 2015 00:42

alexcrichton force-pushed the panic-recover branch from eb7e365 to c39af12 Compare December 8, 2015 14:13

alexcrichton force-pushed the panic-recover branch from c39af12 to 19892fd Compare December 8, 2015 20:20

alexcrichton force-pushed the panic-recover branch from 19892fd to 0a13f1a Compare December 9, 2015 15:19

bors merged commit 0a13f1a into rust-lang:master Dec 9, 2015

bors mentioned this pull request Dec 9, 2015

std: Remove deprecated functionality from 1.5 #30182

Merged

alexcrichton deleted the panic-recover branch December 10, 2015 23:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

std: Rename thread::catch_panic to panic::recover #29937

std: Rename thread::catch_panic to panic::recover #29937

alexcrichton commented Nov 19, 2015

rust-highfive commented Nov 19, 2015

steveklabnik Nov 19, 2015

aturon Nov 24, 2015

eefriedman commented Nov 20, 2015

elinorbgr Nov 20, 2015

alexcrichton Nov 20, 2015

alexcrichton commented Nov 20, 2015

brson Nov 24, 2015

huonw Nov 24, 2015

aturon Nov 24, 2015

brson commented Nov 24, 2015

aturon Nov 24, 2015

alexcrichton Dec 5, 2015

aturon commented Nov 24, 2015

aturon commented Nov 24, 2015

alexcrichton commented Dec 5, 2015

bors commented Dec 6, 2015

aturon commented Dec 6, 2015

alexcrichton commented Dec 8, 2015

bors commented Dec 8, 2015

bors commented Dec 8, 2015

alexcrichton commented Dec 8, 2015

bors commented Dec 8, 2015

bors commented Dec 9, 2015

alexcrichton commented Dec 9, 2015

bors commented Dec 9, 2015

bors commented Dec 9, 2015


		//! Panic support in the standard library

		#![unstable(feature = "std_panic", reason = "module recently added",

std: Rename thread::catch_panic to panic::recover #29937

std: Rename thread::catch_panic to panic::recover #29937

Conversation

alexcrichton commented Nov 19, 2015

rust-highfive commented Nov 19, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eefriedman commented Nov 20, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexcrichton commented Nov 20, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brson commented Nov 24, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aturon commented Nov 24, 2015

aturon commented Nov 24, 2015

alexcrichton commented Dec 5, 2015

bors commented Dec 6, 2015

aturon commented Dec 6, 2015

alexcrichton commented Dec 8, 2015

bors commented Dec 8, 2015

bors commented Dec 8, 2015

alexcrichton commented Dec 8, 2015

bors commented Dec 8, 2015

bors commented Dec 9, 2015

alexcrichton commented Dec 9, 2015

bors commented Dec 9, 2015

bors commented Dec 9, 2015