Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

rationalize uses of error() and throw() #5694

Closed
JeffBezanson opened this issue Feb 6, 2014 · 13 comments
Closed

rationalize uses of error() and throw() #5694

JeffBezanson opened this issue Feb 6, 2014 · 13 comments
Labels
needs decision A decision on this change is needed

Comments

@JeffBezanson
Copy link
Member

Currently all of these do the same thing:

error(BoundsError)
error(BoundsError())
throw(BoundsError())

We're not consistent about which of these should be used. It's unnecessarily confusing. Some background:

  • One idea was that error would be for signaling exceptional conditions, and throw would just be control flow.
  • error(string) wraps the string in an ErrorException, so that we generally throw things of type Exception. This is not strictly required though, and one could throw a string directly.
  • throw is a low-level, non-generic function.
  • another motivation for something like error is that throw(SomeException("message")) is generally too verbose

If we were going to use throw in the control-flow sense, it would probably need 2 arguments (throw(label, value)), so we don't necessarily need 2 names to disambiguate.

My preferred idiom is throw(SomeException()) since it clearly makes an exception object and then throws it. The only downside is verbosity.

Wrapping a string in an ErrorException obviously doesn't add any interesting information, so maybe throw("message") should be considered acceptable. Then error could be an alias or deprecation for throw.

@ssfrr
Copy link
Contributor

ssfrr commented Feb 6, 2014

It may be worth looking at Lumberjack.jl, which seems like a very sane approach to logging (though I'm not sold on the cutesy names). I like the approach that debug(), info(), warning(), and error() all take a string and append it to the log at the given level. error() also throws an ErrorException with the given string (basically like Base.error). In my mind that gives a more clear conceptual framework to the relationship between error() and throw().

As a side note - When I started using the logging module in python instead of scattering my code with prints it was a huge win. Being able to change the log level of the program rather then commenting in and out log messages all the time is something people overlook the value of. Not sure what the criteria are for when things get pulled into Base, but i think that encouraging this sort of logging is a good thing and has basically zero overhead when writing code.

@jakebolewski
Copy link
Member

+1 for throw(SomeException("msg")). I feel it is much more explict and easier to understand what is going on. I hope that it also encourages people to write more descriptive error objects. Sometimes the only way you can tell what went wrong is to match against a ErrorException's message string which is annoying. I feel that we should follow Python's lead here, error() seems like a vestige from matlab.

@jiahao
Copy link
Member

jiahao commented Feb 6, 2014

fwiw, I've put in some effort to systematize the linalg codebase to use throw(SomeException(...)) except when the exception doesn't currently take a message field when I think it would be useful to.

@johnmyleswhite
Copy link
Member

Another +1 for throw.

I also like the idea of pulling a logging framework into Base once the frameworks become more mature.

@JeffBezanson
Copy link
Member Author

I find errors are often hard to classify. For example I just noticed

    m==n || throw(DimensionMismatch("multiplicative identity defined only for square matrices"))

While most DimensionMismatches arise for two things with different shapes that should be the same, this one is for non-squareness. In still other cases the exact shape doesn't have to match, but the number of elements does. It's not clear if there should be a NotSquare exception type. These kinds of concerns add lots of cognitive overhead when trying to be diligent and handle all error cases. I think this is why people (myself included) like to fall back on the ease-of-use of error().

I'm not sure what the solution is, but there should probably be relatively few exception types so you don't have to think too much about which one to use.

@jiahao
Copy link
Member

jiahao commented Feb 6, 2014

I'll hazard a guess here. I think the current error vs Exception situation is unsatisfactory is because a) we don't really do much with Exceptions yet, and b) few of the existing Exceptions encapsulate information that is actually that useful for a caller to catch and act upon; quite a few Exception types do little other than wrap an error message in a string, and aren't really that much more useful than error(somestring). Sometimes the subtype of Exception is semantically useful: LinAlg itself contains three Exceptions which do nothing but wrap error codes from LAPACK, ARPACK and CHOLMOD. However, the current situation does presume that the user is cogent enough to realize that the errors come from the underlying libraries and will look up the error codes themselves.

To carry over an idea from JuliaLang/LinearAlgebra.jl#45, I wanted DimensionMismatch to display a standardized error message. Ideally the process throwing the exception could call some constructor that was given the identifiers of the variables responsible, and one would create a DimensionMismatch that would capture some semantic information about the sizes of the variables passed to the constructor, so that in principle, an error handler could work out what happened, and if the decision is made to output to console, the error message would be standardized, e.g. "Dimensions of A (20,15) and B (14, 10) are incompatible for [whatever]".

@jiahao
Copy link
Member

jiahao commented Feb 6, 2014

If DimensionMismatch applied to a single object is wrong, then maybe this is an instance of the square-rectangle problem.

@jakebolewski
Copy link
Member

I agree that error messages are hard to classify, and having lots of error types is sometimes just as confusing. But I think your example highlights what I was trying to get at before. If you were consuming the above code and it threw an error, isn't DimensionMismatch much more informative about what went wrong than ErrorException even though it is an imperfect description of the problem? You could include an informative string in the error message, but what if the informative string included (ideally) specific information as jiaho stated. Then if you want to act on the error message you have to resort to pattern matching against the error string to figure out what went wrong. I'm all for simplicity, but Python for instance has ~50 exception objects in the core language alone.

@JeffBezanson
Copy link
Member Author

Ultimately I agree we will have to bite the bullet and remove error in favor of throwing specific exception types.

@nalimilan
Copy link
Member

One approach would be to recommend using very specific exceptions, and make them inherit from more general ones when applicable. For example, NotSquare would inherit from DimensionMismatch, which allows callers to choose the most appropriate level of specificity for their use case.

Ideally, I think every error message should be generated automatically by combining the exception type and arguments, just like suggested in @jiahao's comment #5694 (comment) above. This could even be enforced by only allowing error messages to be printed via arguments to a specific exception type.

@kmsquire
Copy link
Member

kmsquire commented Feb 6, 2014

Is there an issue already for catching specific exception types? A quick search didn't find one. Obviously, we can use isa to test the type, as is shown in the manual, but I would love to see something less verbose.

(Better non-dispatch-based pattern matching is something I'm still wanting in the core language... cf. https://github.com/kmsquire/Match.jl, #3146, #5410)

@jiahao jiahao mentioned this issue Apr 3, 2014
@jakebolewski
Copy link
Member

The error(::Exception) methods have been deprecated in 0.4-dev in favor of throw, so this issue seems like it can be closed now.

@hayd
Copy link
Member

hayd commented Jun 2, 2015

To clarify, the consensus is that any error(...) calls in base should be made more specific?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs decision A decision on this change is needed
Projects
None yet
Development

No branches or pull requests

8 participants