[RFC] Allow error definitions outside bzrlib.errors?

[Andrew Bennetts]

For a long time our policy has been that error definitions belong in
bzrlib.errors.  This has been very convenient, but we've also found that it's a
wasteful to have every invocation of Bazaar execute the definitions of over 300
error classes when most invocations will use virtually none of them.  The cost
isn't huge (roughly 20ms according to a quick-and-dirty benchmark on my laptop),
but it's large enough that we've become reluctant to keep adding to errors.py.

So, I propose we relax the policy a little.  I suggest we allow defining some
errors inside the modules that raise them, for errors that:

  a) raised only by one module, and
  b) typically the only places that explicitly catch that error are the
     immediate callers of that module.  (“explicitly” meaning caught via “except
     ExactErrorName” or similar, rather than catch-alls like the one found in
     run_bzr_catch_user_errors).

I think this will be useful enough to allow defining new useful exceptions in
many cases without bloating bzrlib.errors further, and without causing a
proliferation of "from moduleA import ErrorA; from moduleB import ErrorB" in
every module of bzrlib.  If an error is used very widely in bzrlib then perhaps
its definition should be moved to bzrlib.errors — and perhaps many errors that
are currently in bzrlib.errors should be moved to more specialised locations
Why not put DirstateCorrupt in dirstate.py, for instance?

----

Here's the example that prompted this: the bzrlib.inventory_delta is currently
returning generic BzrErrors in lieu of returning more specific exceptions
because we didn't want to pay the price of yet another definition in
bzrlib.errors.  But a more specific exception (or exceptions!) would certainly
be useful.  The cases in this module (ignoring really basic
AssertionErrors/TypeErrors) are:

  - failure to serialize a delta (presumably because the delta is not sane, or
    does not meet expectations about having/not having a versioned root, etc.)
  - failure to deserialize a delta (due to inconsistent serialized data, or
    processing an incomplete or corrupted serialization, or processing something
    that simply isn't an inventory delta)
  - deserializing an incompatible delta (such as encountering a tree reference
    when the caller has said it does not allow them).

Of these, the first is probably a sign of a bug in bzrlib, the second is
probably an error in the input and should be reported to the user, and the third
is particular kind of error in the input that should probably be mapped to
bzrlib.errors.IncompatibleRevision by the caller (because the caller presumably
has a repository object to construct IncompatibleRevision with).  Of these
cases, the one I most want a distinct exception for is the third, so that
StreamSink (the only non-test caller of the deserializer in my inventory-delta
branch) can catch it and make an IncompatibleRevision error from it.  But having
even a single InventoryDeltaError would improve the precision of the
inventory_delta unit tests significantly.

-Andrew.

p.s. A sort-of example that has already snuck into bzr.dev is _NeedMoreBytes in
     bzrlib.smart.protocol, but as the leading underscore indicates it's private
     to that module.  So this case would match the proposed policy, as
     bzrlib.smart.protocol is the only caller of methods that might raise that
     exception, and it is also the only module that catches it :)