Rename assert to enforce, add debug_assert

[huonw]

Currently assert! is used to enforce invariants, not just doing sanity checks that get removed in release builds (unlike traditional asserts). This changeset renames them to fail_unless (and fail_unless_eq to enforce_eq), and adds debug_assert and debug_assert_eq for assertions that are active unless --cfg ndebug.

Closes #12049.

General points:

• you probably don't want to look at the "files changed" tab or the "Rename uses of ..." commits; the other individual commits will be more interesting.
• debug_assert etc. are never automatically removed, since not even -O sets --cfg ndebug (I did not add any use of them to the codebase, and am happy to remove the addition if people think we should work out a better arrangement)
• This is very easy to rebase (since the renaming is entirely automated), and so can wait until other manual harder-to-rebase patches land (in particular, I'm thinking of @pcwalton's vector patch).

[bnoordhuis]

debug_assert_eq!() is rather a mouthful. Google uses ASSERT() and CHECK() for checks that should be done in debug and release builds respectively. How about assert!() and check!() macros?

[huonw]

Something like that is fine by me long term; but I think we need to have a (reasonably long) period without an assert! macro at all, because, if we do have one, habit will mean that people may then use the debugging assert when they actually want/need the check/enforce assert.

(I do think enforce sounds slightly better than check, but that's just, like, my opinion.)

[ghost]

logging uses cfg!(not(ndebug)).

[ghost]

I like enforce, it sounds more fail-y.

[sfackler]

I would prefer debug_assert to use cfg!(not(ndebug)) as well. That way it matches both the behavior of debug! and C-style assert (-D NDEBUG).

[alexcrichton]

I personally feel very strongly that we need a macro with the exact name of assert. The notion of an assert is very common amongst all languages and libraries, and I don't think that we should be completely removing the name "assert".

Right now we don't have a debug assertion, and I think that it's a problem. In my opinion the world should look like:

• There are two assertion macros, one called assert and one not called assert.
• Both macros are turned on by default
• You can selectively turn one of them off with a --cfg flag

I don't really have a strong preference as to which bucket assert falls into, but I very much want the name assert to stick around.

[sfackler]

I'd vote for assert being the disable-able one since that's how many other languages (C, Java, etc) behave. I agree with @huonw that if we did that, It'd probably be a good idea to have a small transitional period of having no macro called assert to make sure that downstream code knows to update. Maybe it could be temporarily featured gated or linted instead?

[lilyball]

Some languages allow you to disable asserts in non-debug builds, but not all. And in those languages that do, you're not required to disable them. I'm personally in favor of keeping assert how it is and adding debug_* asserts.

[brson]

Please let's consider this very carefully.

[huonw]

Updated to use ndebug. Another possibility (maybe in addition to debug_assert aka assert?) would be asserts that hook into the RUST_LOG level, and so can be dynamically activated.

e.g.

assert_level!(std::logging::DEBUG, super_expensive());
assert_level!(std::logging::WARN, reasonably_cheap());

[alexcrichton]

My favored course of action is that assert is always on and dassert can be turned of with --cfg ndebug

[lilyball]

I agree with @alexcrichton.

Also, asserts that are dynamically activated with RUST_LOG sounds kind of interesting, but also probably not a good idea. Turning on logging shouldn't change behavior.

[pnkfelix]

I agree with @alexcrichton's proposal:

My favored course of action is that assert is always on and dassert can be turned of with --cfg ndebug

except that I would prefer the name debug_assert! to dassert!. (A trivial locally-defined macro can wrap it and thus provide dassert locally.)

[thestinger]

@alexcrichton: At least in the world of C and Python, an assertion is not expected to be there in a release build. I don't see a need for Rust to deviate from the expectation of many users rather than giving it a less overloaded name like enforce! that's clearly not going to be disabled in a release build. There are many assertions in the Rust codebase only there to verify that the code is correct and not enforce invariants, and they are a significant contribution to the standard library's poor performance and code bloat. For example, there are assertions on every RefCell borrow. These shouldn't be there by default... and if assertions are enabled by default, they will just have to be removed completely from places like this.

If it's on by default in a release build, then it's not a debugging feature. Code will be built to depend on the failure being thrown and caught. It's definitely not what I think of as an assertion.

[thestinger]

@kballard: Which languages call it assert but don't optimize it out in a release build?

[lilyball]

@thestinger Well, even C's assert() is only compiled out if you define NDEBUG. Obj-C's NSAssert() similarly doesn't get compiled out unless you define NS_BLOCK_ASSERTIONS (which most projects don't do). Ruby's assert() never gets skipped AFAIK.

[cpeterso]

Eiffel uses ensure for its postcondition checks (and require for precondition checks).

Python will remove assert checks when run with the -O "optimize" flag, but AFAIU this flag is rarely used because it will also strip out docstrings, breaking Python modules that rely on docstring annotations at runtime.

[alexcrichton]

I personally always find it ambiguous whether assert is optimized away or not, and I've always found hazy answers as to whether languages expect it to be optimized away or not.

I would rather have a macro which clearly says in its name that it will be optimized out willy nilly, so you shouldn't rely on it. I don't personally think that enforce/assert is a good split because it's not clear which one gets optimized away and which one doesn't (I think it's the same problem with assert/check). The debug_assert/assert split is quite clear, but the debug version is a tad bit long. Using dassert/assert it's not super clear that dassert == debug assert.

(just my current thoughts)

[glaebhoerl]

@thestinger I only see assert! used in std::cell in unwrap() and in impl Drop for Ref/RefMut. Borrowing a RefCell uses fail!() explicitly, and unless I seriously misunderstood something, this is necessary to preserve memory safety (i.e. would be unsafe to optimize out).

[huonw]

If the code is correct (and my understanding is correct), the asserts in the destructors will never be triggered.

[Kimundi]

1+ for enforce!.

Having used the language for over a year, I still hesitate to actually use assert! for any sort of "always required" checking, because of preexisting conditioning that assert is just for optional, not semantic changing, checking.

In my opinion a name like enforce! would make the distinction quite clear, and reads like a stronger guarantee than assert!.

It seems logical to me to have, in the longterm, the split between always-on, semantic relevant enforce!, and an optional, debug-only assert!.

A bit nonsenical example:

fn foo(i: uint) -> uint {
    enforce!(i < 10, "The Algorithm only works for i values 0-9");
    /* complicated algorithm */
    assert!(result <= 1024); // This should always be true if the algorithm is correct
    result
}

[nikomatsakis]

+1 to @Kimundi's suggestion. There is long-standing precedent from many languages that assertions are for enforcing static properties you believe will never fail -- and hence it is safe to remove them. I see no reason to change the name assert.

The biggest question is if we should compile asserts out by default under -O. Coming from C, I'd assume that assertions are removed under -O but not -O -g, but I assume we'll want this to be configurable.

I know that the choice to have assertions always be compiled in was a conscious choice.

[nikomatsakis]

I see everything I said has already been said and with more precision. Oh well, I should always read the full comment threads. =) Anyway, +22 to @brson's point of "let's not land this without much consideration".

[huonw]

FWIW, modulo the name of assert!, @Kimundi's suggestion is exactly what this implements; although debug_assert! is currently only removed when explicitly compiled with --cfg ndebug.

(I just realised that I hadn't updated the PR description, which may've been a source of confusion; updated now.)

[ben0x539]

fail_unless!? :)

[emberian]

I'm in favor of this pull request as-is, but would also be in favor of assert/enforce.

That said, I'd prefer if it used not(debug) instead, and that -g is used to signal debug, with debug! also using -g. We can add a -C no-debug-info if necessary. Just using rustc won't compile it out, but rustc -O would, which is more along what I'd expect from a modern language.

[alexcrichton]

I have put this on the next meeting agenda.

[brson]

In the meeting this week we decided to do the following:

• Add a fail_unless! macro to existing crates that duplicates current assert! macro. We don't export this.
• Convert existing assert!s to fail_unless! to preserve existing logic
• Modify assert! to be disabled when ndebug is defined
• Warn people about the changing semantics.

[huonw]

Updated to use a publically exported fail_unless (I know there's not really a concensus...).

[huonw]

fail_unless is significantly longer than assert, and so the change pushes several lines over 100 chars and this doesn't pass tests. I'll update/shorten them when someone wants to give me an r+.

[brson]

@huonw has it been decided how assert interacts with tests after this change?

[ghost]

As a naive user, I'd expect debug_assert to be active in tests but not benchmarks.

[huonw]

@brson tests use fail_unless, with debug_assert remaining as a debug assertion (i.e. building tests with --cfg ndebug removes debug_assert but doesn't touch fail_unless, because testing such a configuration is useful).

[lilyball]

As a naive user, I want assert!() and assert_eq!() to function in tests. I shouldn't be calling anything debug_* for a test, and fail_unless!() seems wrong.

Did we get rid of assert!() as a general-purpose macro? If so, could we reintroduce it exported from the test module, and implicitly provide it when building with tests?

[huonw]

fail_unless seems reasonable for tests to me: "This test fails unless x". (It does go against convention a little.)

[thestinger]

I still think enforce is the best naming and it makes a lot of sense in a test. I don't think there would be an impression that it may be disabled with a strong naming like that.

[huonw]

I like enforce the most too.

Also, @kballard, if it's not clear, the removal of assert in this patch isn't necessarily meant to be permanent; just making sure we don't introduce (memory-safety) bugs from people accidentally using a disappearing assertion (out of habit, or from the behaviour of assert changing silently) when they need one that stays.

[pnkfelix]

accidental nomination...

[sfackler]

Ping?

[huonw]

I'm happy to update/rebase/change this, but I don't think we have a sensible resolution yet?

[pnkfelix]

So, just to put it out there, I've been floating this idea around: instead of allocating a macro name for the "always on" assert, one could follow Perl's example:

#[allow(deprecated_owned_vector)]
fn main() {
    ::std::os::args().len() == 2 || fail!("require exactly one argument");
    println!("arg: {}", ::std::os::args()[1]);
}

I had thought we would need to put in a special case into our "unused value" lint to deal with this, but apparently it works without any warnings right out of the box:

% rustc /tmp/die.rs
% ./die  hello
arg: hello
% ./die  hello there
task '<main>' failed at 'require exactly one argument', /tmp/die.rs:3
% 

(True, this requires the user to spell out what the condition was that they were checking. But that might be a pro, since one probably wants to control how much their code gets bloated by the output strings for this construct.)

Anyway, this pattern means we could just make assert! behave like debug_assert!, and stop worrying about what to name the other case (since it can be written very succinctly inline).

[glaebhoerl]

That's so cute I can't decide if it's a good idea or not.

[huonw]

I'm with @glaebhoerl; that seems cute and workable... but it's possibly a little too cute.

I guess what's now assert!(foo()) would become foo() || fail!()?

[huonw]

Oh, that raises a downside: assert!(foo()) will currently give an indication of what went wrong "assertion failed: foo()", but the foo() || fail!() form does not (or maybe it will encourage people to write informative failure messages, which might not be a bad thing).

[pnkfelix]

@huonw right: what is now assert!(foo()); would become foo() || fail!("foo()");

Though perhaps we could tweak rustc for the special case of <expr> || fail!(), I think I'd stick with what I said above: "one probably wants to control how much their code gets bloated by the output strings for this construct." This matters for code bloat; it may also matter for trying to prevent information leakage via side-channels (though really, the latter seems like a weak argument since information leakage should probably be prevented via higher level means).

[alexcrichton]

Closing due to inactivity.

I think that with our new RFC process that this should likely go through an RFC before getting another implementation. This appears to be a fairly contentious topic, but I would very much like to see this resolved for 1.0.

[SimonSapin]

@alexcrichton To make sure this does not slip through the cracks, should we keep an open issue for this with a "backward compat" flag, or whatever is appropriate to have it reconsidered before 1.0?

[huonw]

#12049.