[rfc][merge] ban assert statements

[Martin Pool]

I think we should ban assert statements from Bazaar: they cause bugs
and don't particularly help:

 * it makes the behaviour vary depending on whether bzr is run with -O
or not, therefore giving a chance for bugs that occur in one case or
the other, several of which have already occurred: assertions with
side effects, code which can't continue unless the assertion passes,
cases where we should give the user a proper message rather than an
assertion failure
 * it's not that much shorter than an explicit if/raise
 * it tends to lead to fuzzy thinking about whether the check is
actually needed or not, and whether it's an internal error or not
 * tends to cause look-before-you-leap patterns
 * it's unsafe if the check is needed to protect the integrity of the
user's data
 * it tends to give poor messages since the developer can get by with
no explanatory text at all
 * we can't rely on people always running with -O in normal use, so we
can't use it for tests that are actually expensive
 * expensive checks that help developers are better turned on from the
test suite or a -D flag
 * if used instead of self.assert*() in tests it makes them falsely pass with -O

This patch adds a test for them; it traps about 15 files that
currently use them.  If people agree with this in principle I'll post
a patch that changes them to if/raise with AssertionError ValueError
in most cases.  This will not magically make these errors more
understandable if they occur to a user, but I wouldn't be surprised if
I find some of the cases above.

16:15 <poolie> lifeless/spiv: what would you say to a source test that
forbids the use of the assert
               statement?
16:19 <spiv> poolie: I have mixed feelings about that
16:20 <spiv> Because I think that the assert statement is a sane thing
to use sometimes.  But it's very
             easy to misuse, so perhaps disallowing it entirely is the
best policy.
16:21 <spiv> So I'm +0.5 on it, I guess.
16:22 <poolie> can you give me a good case for committing it in mainline code?
16:23 <spiv> For a purely internal-to-bzrlib API, an assert that e.g.
"type(revision_id) is str" could be
             reasonable.  The sort of thing that would catch misuse of
an API within bzrlib.
16:24 <poolie> in C i'd certainly do that
16:24 <poolie> but the combination of not having a clear distinction
between debug and other builds
16:25 <poolie> means it just doesn't seem like a good tradeoff
16:25 <spiv> The thing that makes me lean towards banning them is plugins.
16:25 <poolie> all you really gain is slightly shorter syntax, right?
16:25 <poolie> mm?
16:25 <spiv> Because it's too easy for a plugin to accidentally trip
over an assert than was intended to
             catch a bzrlib-only mistake, and really that shouldn't be
a user-visible error.
16:26 <spiv> Right, you can explicitly use "raise AssertionError, ..."
instead, but it feels a bit funny
             to raise AssertionError.  Probably that's not a hard
habit to break.
16:29 <spiv> So I share your feeling regarding debug builds: asserts
are the sort of thing that should
             happen for developers only, to help them know when
they've misused an API or similar, to make
             development go faster.  They shouldn't be depended on to
preserve the integrity of the
             system, e.g. in the face of bad user input.
16:29 <spiv> Hmm, lag.
16:29 <spiv> But of course we don't have debug builds, so I guess we
never have a good time for an assert
             :)
16:30 <spiv> I guess I've talked myself into agreeing with you :)


-- 
Martin <http://launchpad.net/~mbp/>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 20080422-require-no-asserts.diff
Type: text/x-diff
Size: 4006 bytes
Desc: not available
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20080422/93e19f05/attachment-0002.bin