Add RFC for disablable assertions

[pcwalton]

No description provided.

[sfackler]

cc rust-lang/rust#12108

[SimonSapin]

IMO we need both disablable assertions for checks that help debugging but are not critical (example), and non-disablable assertions for checks that are critical for memory safety (example) or other thing like security or not accidentally losing user data.

[pcwalton]

Right, that's the difference between assert! and enforce!.

[SimonSapin]

Yeah, +1 to the RFC as is. (I don’t care too much about naming.) My message above was arguing against an earlier decision from the Rust team to have only disablable asserts.

[nikomatsakis]

I am basically in favor. We probably ought to provide enforce_eq! as well (and whatever other variations on assert! we have today...)

[bill-myers]

Note that there's a further distinction, between an assert! that still evaluates the expression in debug builds but doesn't check its value, and one that completely removes it, which obviously makes a difference when it has side effects.

This RFC says that "assert! will compile to nothing": I wonder whether this is an intentional decision.

I would recommend always evaluating the expression, since not doing so breaks the intuitive principle that one may remove variables with a single use right after a single initialization without affecting program behavior, and in general risks introducing bugs in release builds.

For assertion expressions that the compiler can't optimize away, a construct like if !cfg!(ndebug) { assert!(super_complex_function()) } should be encouraged, which makes the effect clear.

[SimonSapin]

@bill-myers, evaluating expressions in disabled asserts defeats the purpose of disabling them to avoid the performance cost on evaluation. Also, I believe there’s precedent of removing side effects for disabled asserts in other languages.

[sfackler]

Side effects from disabled asserts are removed in C, C++, Python and Java.

[bill-myers]

It doesn't defeat the purpose, because removing the check will make the expression dead code removed by the optimizer in the vast majority of real world asserts (most asserts are things like a + b <= c or functions that are inlined).

There's the issue that you would be relying a bit on the optimizer, but on the other hand removing the expression altogether means that testing done on debug builds is not valid for release builds since an assert may change the behavior, which seems worse.

It's also simpler, since assert! behaves like a normal function.

[sfackler]

In all languages I am aware of, assert is implemented as either a macro or is actually a keyword to make it very intentionally less like a normal function. It's not uncommon at all in my experience to have expensive correctness checks in asserts with the knowledge that they'll be guaranteed to disappear in production builds.

[pcwalton]

No, it is not the "vast majority" of cases. It is very common to have procedure calls (for example, format string construction) in assertions, which cannot be optimized away.

[bill-myers]

So, I think the ideal solution would be to have a side effect analysis, and output a compiler error when the compiler cannot prove that the body of an assert! has no side effects rather than silently letting the code live as my earlier suggestion would.

Note that this also ideally requires to add side effect annotations on functions, to avoid the "implicit API guarantee" issue that global inference would cause.

I don't know if it's worth doing all that, but an assert that removes any expression is fundamentally unsafe, like allowing threads to access shared memory without locks or atomics, not requiring variable declaration and other widespread language misfeatures.

[cgaebel]

It's not "fundamentally unsafe". If you want your side effects to stick around in a release build, use enforce.

I might maintain some debugging state in my asserts, which would have side effects that I don't want in release builds. It's also a common pattern to have a function which checks the invariants of a data structure, and outputs the structure if it fails. I only want this running in debug builds, so I'll do something like assert!(self.check_invariants()). Since it can output something (on failure), this wouldn't be allowed.

[bill-myers]

For that, you would use a "debug_print" or "debug_log" function not marked as having side effects, or preferably either return an Err(s) object where s is the error output, and have an assert_ok!() function that prints the error value on failure, or even have an assert_property!(data, func) that does assert!(func(data)) and prints data on failure.

[bstrie]

I think that discussion of an effect system is out of scope for this RFC. Even if it's possible for program behavior to change as a result of eliding asserts, the elision would not itself allow Rust programs to violate safety:

let x;
assert!({ x = 1; true });
println!("{}", x);

When compiled with asserts disabled, this contrived code would cause a compilation error.

+1 to this RFC. Simple to understand and solves an important need. We can't let people be afraid to liberally sprinkle their code with assertions.

[bstrie]

One suggestion: instead of a --cfg ndebug flag, would we rather just have a --release flag that not only disables assertions, but also defaults to either --opt-level=2 or --opt-level=3 ?

[pcwalton]

Decision in meeting: Keep assert! the way it is and add debug_assert! for a disablable one. Rationale: Having assert! be disablable could break tests.

meeting discussion link

[nikomatsakis]

Also: not a breaking change.

[sfackler]

Should this get merged and implemented?

[alexcrichton]

This needs to get updated to reflect what we came up with during the meeting.

[sfackler]

The implementing PR is ready to go whenever this gets updated and merged: rust-lang/rust#13789.