Make ub_check message clear that it's not an assert

[kornelski]

I've seen a user assume that their unsound code was safe, because ub_check prevented the program from performing the unsafe operation.

This PR makes the panic message clearer that ub_check is a bug detector, not run-time safety protection.

[rustbot]

r? @cuviper

rustbot has assigned @cuviper.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[Noratrieb]

r? @saethlin

[saethlin]

It's good to have experience reports, but these checks have been around for 6 releases and this is the first time I've seen this response. Everyone else seems clear on the idea that "violating an unsafe precondition" is something you should fix your code to not do. If it's that combination of words that is hard to understand, I'd rather adjust those in the diagnostic than shout at our users or emit text that is redundant to everyone but an extremely small minority.

[kornelski]

I've calmed down the shouty part.

The current "unsafe precondition(s) violated" part is very clear that a precondition has been violated, but on its own is not very clear what the consequences of violating the precondition are. It's not obvious whether this check indicates UB, or prevents UB. Compare:

• In the regular array[index] there's a safety precondition that the index must be in bounds. There's code that checks the precondition, and panics when the precondition is violated. The panic makes it safe, and you can keep the code as-is.

• In array.get_unchecked(index) there's a safety precondition that the index must be in bounds. There's code that checks the precondition, and panics when the precondition is violated. The panic doesn't make it safe, and you must fix the code.

On the surface both safety checks are similar and catch the same error, but they indicate very different things. Evidently, the wording difference between "unsafe precondition" and other preconditions that prevent unsafety isn't clear enough.

So I still think the message would benefit from giving more context. This message is only aimed at developers, and is only meant to be used in dev builds, so I don't see a problem with making it as long as needed.

[rustbot]

The Miri subtree was changed

cc @rust-lang/miri

[saethlin]

Hm. I think I'm more positive on this now, but really my opinion on these tends to change as a result of directly watching people run into them.

Thank you for your input on this little feature.

@bors r+ rollup

[bors]

📌 Commit ca28827 has been approved by saethlin

It is now in the queue for this repository.