-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
RFC: Abort by default #1759
RFC: Abort by default #1759
Conversation
The use-case for unwinding is very narrow (long lived servers one doesn't want to restart, I've never heard anything else). Furthermore, in a futures-based world—our main story for servers—threads are just used to run other tasks and thus dont really encapsulate something concrete and known to the programmer. |
Also, this is good for pedagogy—instead of "two ways to do things but one is bad so please don't", there's just one. Finally, if we wait to do this once std can be transparently rebuilt, we are hardly burdening those that want unwinding. |
Silently switching the default is a non-starter IMHO. While it's true that libraries should not depend on the panic strategy, it is clearly valid for applications (i.e., binary crates as well as library crates that are intrinsically tied to an application for organizational purposes) to do so. And since unwinding has been the only option for a long time and has always been the default, most existing crates won't be explicitly setting On the other hand, defaulting to |
Edit: I'm wrong. Code can very well rely on the specified panicking strategy, but not a particular default strategy. |
Okay, why do unwinding panics even exist then, if they are completely useless and aborting is certainly a more performant compilation option? |
quickcheck would break as a result of this change. |
Could you elaborate on that?
Edit: See edit note from previous comment. |
I'd like to see a valid, idiomatic example of code which relies on the panicking strategy. |
@ticki I don't buy the "compilation option" argument. It's a good heuristic, and most compiler flags indeed should not alter observable behavior, but there are plenty that necessarily affect whether a program works. Even if we restrict ourselves to the I don't want to spend a lot of words defending the use of panic=unwind and Finally, even if it were the baddest practice ever, that doesn't give us the right to break code using it. The stability promise is emphatically not "if you write code we like, we'll try to not break it". Transmutes between fn item types and fn types are infinitely more suspect and weird than unwinding, and even that got a several release cycle long compatibility lint! |
It's completely irrelevant whether it's good code. It's irrelevant whether it would be easy to avoid the issue if you knew the change was coming. All that matters is this sequence of events:
|
Even
It isn't just because it's a bad thing to do. It is because it relies on unspecified behavior, and therefore cannot be considered a breakage in the strictest sense. |
Eh, the same argument could be applied to telling LLVM that transmuting |
Every program on earth contains bugs. "Sorry, your program was buggy, we broke it" is not a nice thing to say, and not a defensible position.
What's unspecified? Please don't get all language lawyers. Virtually every resource that mentions the topic of panics also describes unwinding, including many official ones. There is no binding, formal standard, true, but that doesn't give the Rust project the license to declare anything it wants unspecified. It is, de facto, specified, and there is not even a really good reason to change it (unlike with some breaking changes that have happened in the past). Just switching the default of newly generated UB is very different. For starters, the Nomicon is quite clear that transmuting &T to &mut T is evil. I'm also much more willing to call some dark corner of unsafe code unspecified or UB than I am with prominent aspects of core language features. |
And, I don't buy the "if there's no behavior difference between unwinding and abort, why not just use abort then?" argument, because the fact that there is no difference in behavior in the ideal case doesn't mean there can't be in a realistic environment. Long running programs aren't supposed to panic, but they might do, and it's totally valid for those to prefer unwinding to avoid crashes. |
You're misunderstanding me here. If you rely on implemention defined behavior, you cannot be sure it actually stays that way, and hence you cannot rely on the behavior for correctness.
That's a good point. I suppose a sensible alternative is to simply make |
And the change proposed here will suddenly, silently, with no warning (no release notes don't count) make it so that people who prefer this mode of operation suddenly won't have it any more. Even if you don't want to call it a breaking change, that's a dick move. That alone would be enough for me to oppose it. |
What do you think about this idea ^ |
I've been advocating it since my very first comment! (The "libraries adopt the strategy of the dependee" part has always been true.) |
There's one exception to this: I think an abort-library should be able to link to a panic-runtime, since they can interact flawlessly. |
We appear to be on the same page now, but I do have one more rant in the pipeline about the justification of changing the default, sorry:
It's no more implementation defined than |
This, too, is already true: |
|
It definitely isn't. I can't link against panicking runtimes when I have |
@ticki That's weird. What exactly do you mean by "runtime"? |
@rkruppe The converse: Linking aborting libraries against panicking applications. |
It might originate due to another compiler flag. In any case, I think it is irrelevant for this discussion. |
I thought that in the panic=abort RFC, there was a way to specify if your crate relied on either behavior, and to fail compilation if you tried to, for example, compile a If that was there, QuickCheck and other crates could specify that they rely on unwinding, and things would be peachy. |
@steveklabnik No, changing the default in the compiler still wouldn't be peachy, because there will most likely be many crates which should set that flag but don't. These would still be broken, and the above argument about whether it's okay to break people's application when they should have known better still applies (to a lesser extent). |
|
||
## Correctness | ||
|
||
You often see people abusing `std::panic::catch_unwind` for exception handling. Forcing the user to explicitly opt in to unwinding will make it harder to unintentionally misuse it. |
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.
I don't think I have ever actually seen this in practice. Where does this pop up often?
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.
At least one person has done this:
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.
On /r/rust people regularly talk about how "nice it is to have exceptions".
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.
"Forcing the user to explicitly opt in to unwinding will make it harder to unintentionally misuse it."
At least one person has done this
It was an intentional misuse though. Cf. https://www.reddit.com/r/rust/comments/53ft4m/my_experience_rewriting_enjarify_in_rust/d7ss1vy
From the docs of
Woah... that trait reassured me a lot when I saw the name, and now a lot less. This is very subpar by Rust's standard's and if |
Your user does not care about whatever eventual progress is being made; they care that the thing they are trying to do doesn't work. Most bugs are not likely-and-not-crazy.
What language is that written in then? We never intend our code to be broken and panic, and yet it sometimes does.
Again, how much code actually cares about that distinction? There was quite a lot of discussion around whether |
In Rust, I wonder how much rarer those issues are than unwinding itself? On a different note, I am probably in a rare group that believes auditing all panic points is quiet feasible.
I am hoping that for such a small program, the feasibility of auditing all panic points is less a controversial opinion. |
I don't want to be moving the goal posts, because we were talking about webserver-like stuff, but in other domains the client is non-human with different timing preferences. |
I think my entire opinion rests on these assumptions:
|
Even if the immediate consumer is non-human, there is (unless some very significant advances in AI research have been made recently that I'm not aware of :P) a human waiting on the result of that computation somewhere down the line. For those assumptions:
|
I guess I've never had much need to index arrays or use RefCell and friends---I can't recall panicking outside of |
If stability is what is blocking this, Rust can be declared immutable forever. The default is unspecified and you should not rely on the behavior unless you manually specify another strategy. If we have to make sure no unspecified behavior changes, what's the point of keeping it unspecified? The whole point of a thing being unspecified is that it can be changed and you should not rely on it's behavior, and I'm tired of the "we cannot change this because somebody might assume that it is like this, despite not being specified." altitude. It keeps Rust from improving. If your code is broken, it's broken, and I'd go all the way and say that broken code should not be silent, because that simply enforces it's status quo. Safe crashing is perfectly legitimate in order to inform the programmer that the code is broken. Programmer errors happens, yes, but if you rely on unwinding, you should specify that your crate is unwinding. The first thing that comes to my mind is the filling drop future proofing RFC. It changed unspecified behavior, which broke some crates, and I think that it broke broken crates would be a poor reason to oppose it. You could argue that this was due to drop flags being in the pipeline, but ultimately it did change unspecified behavior. I'd also like to emphasize that this does not introduce any form of unsafety. The worst case scenario is that a program might stop with an error message. One thing I think is especially dangerous is crates which do not specify |
@ticki You are right that breaking some programs somewhere is inevitable while making changes. Even the seemingly most harmless changes, like adding a trait implementation, can break code. But precisely because almost any change is breaking, there must be more nuance to the decision than simply "this could break some program somewhere", and thus changes are weighted by criteria such as (this list is probably not complete):
This proposal scores okay on the first point (mostly, just add (At this point I would also like to stress again that "set However, I do concede that this may be a good change with manageable impact, and so it may not be ruled out by the stability policy. So much for the process of breaking changes. Let's return to the justification. A key point of your argument is that unwinding-as-default is unspecified. I disagree. I wrote here previously on why I believe this. To briefly recap:
Consequently, I think it is entirely reasonable for a user to look at all the messaging from the Rust project and determine that, since unwinding is obviously the default, they don't need to go through the hassle of adding some lines to their This is in stark contrast to filling drop, which to the best of my knowledge was never documented very much (drop flags were probably mentioned, but the details of filling, not really — this was the dark ages where nobody even tried to formalize guidelines for unsafe code). The Nomicon does mention (and IIRC has mentioned since that chapter was written) that drop flags as a whole are slated for removal. Moreover, rust-lang/rust#23535, which I assume you mean by "filling drop future proofing", was merged in March 2015, before Rust 1.0 (= the start of the stability policy). |
Quick note: the libs team discussed this RFC a bit, and everyone felt strongly that we cannot make the change as written -- we consider this a massive breaking change in behavior. A couple of notes about the formalities here:
Now, all of the above is separate from the question of something like having It's also disappointing that the RFC gives no discussion or weight to the fact that unwinding is ever useful or desirable. If we want to have a meaningful debate about the right defaults going forward, we need to take a clear-eyed look at the pros/cons on both sides. |
I'll also weigh in on my brief thoughts. The bottom line is that this cannot be done as proposed because it is a massive breaking change to the long expected Rust semantics. There is code that relies on unwinding in Rust - for example, the standard test runner, which nearly every project depends on, does not work without unwinding. Such code works today on all existing platforms and forcing it to break would be incredibly user-hostile and counter to the spirit of Rust's stability guarantees. |
Is it reasonable for Cargo to eventually require a panic strategy for binaries? FWIW, cargo is pre-1.0, and libraries/crates.io wouldn't be affected so there's no cascading breakage. |
@aturon 😢 Anyway, I'll update the RFC.
Yeah, I should add that. |
What value does that provide to the end user? Why will developers say "thank you for breaking my project until I added more stuff to my Cargo.toml"? How will this work logistically? The panic strategy is attached to a compilation profile, not the crate itself. Is this proposal really going to be to make people add this to every project they work on? [profile.dev]
panic = "unwind"
[profile.release]
panic = "unwind"
[profile.test]
panic = "unwind"
[profile.bench]
panic = "unwind"
[profile.doc]
panic = "unwind"
No. The number returned when you run |
It might make sense to allow specification of the panic strategy to be placed on the crate itself. How that would be applied to various pieces (testing, bench, doc, and binary/library generation) would need to be specified. Consider that at the moment those wanting to disable unwinding need a similar amount of boilerplate.
It could encourage the author to consider "does the functionality of my crate rely on unwinding", where at the moment that choice is mostly unconsidered. That said, I don't think only specifying this on the final crate is very useful, as it requires the consumer crate to completely understand their complete dependencies. |
A possibly doable way of making this change would be to do it in stages: for the next say six months, cargo prints a warning if a crate does not specify a panic strategy. So basically all crates would end up specifying one of the two. After that period, you flip the switch and the default becomes abort. It's not pretty, but it would probably work well enough to cover all crates anyone cares about. I'll leave the argument about should we do this at all to others. |
I cannot but wonder whether we would be breaking Cargo or Rust. What I mean is that if we switch the default panicking strategy in rustc to Anyhow, this is just a thought, I've still have not made my mind about any of this although for new projects a |
@gnzlbg As has been pointed out by several people multiple times, weaseling ourselves out on basis of "exact words" goes completely against the spirit of the stability policy. If a project works before updating and doesn't work afterward, that's bad, period. |
@rfcbot fcp close Like many others, I think that this change isn't sufficiently well-motivated and not worth the chance of perturbing existing code. The discussion in the @rust-lang/libs team team seems to have come to a similar conclusion. Moreover, my sense is that the major arguments which are going to be made have already been made. Therefore, I propose that we go to FCP with the intention of closing this RFC. (If you feel I'm being too hasty here, though, please do object!) Major points raised in the conversation thus far:
|
@nikomatsakis The RFC is updated such that there is no breaking changes. Could you reverse the FCP? I believe all of the criticism expressed in this thread is no longer relevant to the new text. |
FCP proposed with disposition to close. Review requested from: No concerns currently listed. |
@ticki Considering the changes you just made pivot the RFC, it might be better to start fresh and open a new pull request. |
Superseded by #1765 |
@rfcbot fcp cancel |
@nikomatsakis FCP proposal cancelled. |
This is a long shot, but let's see...
Rendered