Please stop using `error` and `ErrorException` in packages (and Base)

[dfdx]

@foobar_lv2

For both these cases, it is easy to imagine situations where I might want to catch the exception:

Yep, and in these rare cases you still can catch ErrorException.

[jlapeyre]

EDIT: I only fixed typos since the first version of this post. But, I see this bumps the post to the most recent. Unfortunately, there does not seem to be a toggle to prevent this (because “none of our paying customers ever asked for this”… fair enough)

I have some sympathy. Recently I used a Python wrapper of a MySQL library. Probably for lack of time, the author coarse-grained the possible exceptions and included a catchall. For whatever reason the error strings were not useful. I was quite frustrated. Of course, it took no knowledge of Python or MySQL for me to fix it in the Python source (I should give it back some day.)

Even if the error is not caught, using the correct exception can more consistently and reliably convey information than a string alone can; a string constructed however the author saw fit.

Regarding @assert, it’s clear that the ultimate arbiters here understand that (from c2.com)

An assertion is a boolean expression at a specific point in a program which will be true unless there is a bug in the program.

It should be possible to easily disable assertions to test production code. Exceptions should never be used in place of assertions. They play different roles and should not be confused.

But, @assert will not get this clearly defined role till at least v1.1. If I read the discussions (several threads) correctly, care was taken so that @assert can be used correctly in the future without breaking v1.0 compatibility. (Many popular languages did not have proper assertions at the time of first stable release.)

[Tamas_Papp]

@foobar_lv2

the package that depends on your package might provide a workaround, so that the end-user does not need to be bothered

IMO such workarounds are just trouble in the long run. They are usually procedural (not declarative), and are prone to bit rot much more than “usual” code. I recognize that sometimes they are necessary, but the goal should be to get rid of them as quickly as possible.

@foobar_lv2

A package that uses differentials might want to try multiple strategies; e.g. try symbolic before AD, but only if symbolic (1) works and (2) the expressions stay reasonably small; so your user, who is a package author, may reasonably want to catch this.

Again, if a package advertises that it does symbolic AD, why would it not work? That would be a bug. Using the exception handling system to work around bugs is sometimes necessary, but should be considered the durian of code smells.

[Tamas_Papp]

@dfdx

for line in lines
        res = tryparse(line)
        if res != nothing
            ...
        else
            failed = true
            break 
        end
    end
end

I would probably code this as

function process_file(file)
    lines = ...
    for line in lines
        res = tryparse(line)
        res ≡ nothing && return true
        ...
    end
    false
end

which I consider rather simple, but I see your point. Some languages do encourage conditions for control flow, and probably Common Lisp was the language that took it to the extreme with restarts, eg see Practical Common Lisp for a nice example. I was kind of missing that when I started using Julia, but now I consider these misfeatures, as they lead to code which require a lot of non-local convoluted reasoning.

[Liso]

@Tamas_Papp

I am curious what your use case is for enforcing a fine-grained distinction.

I humbly think it is case of every more complex code with longer support and/or with bigger team.

@Tamas_Papp

There are already

julia> length(subtypes(Exception))
58

in Base alone.

I am not julian yet so maybe I see it wrong but I see as problem:

length(filter(isabstract, subtypes(Exception) )) == 1 

so there is clearly no hierarchy here…

[oxinabox]

Here is one in the wild:

https://github.com/malmaud/TensorFlow.jl/blob/459b5450917be40d2fa14bfd988d7877332bc62f/src/shape_inference.jl#L44-L65

If the shapes are incompatible, then it throws with useful error message.
A try_unify couldn’t give a message saying what failed, just that something did.
(While I like that pattern for simple cases, I think for complicated cases it only can go so far.)

That exception can be thrown at a bunch of different times, basically anytime the shape inference is invoked.
It is caught, at one place:
https://github.com/malmaud/TensorFlow.jl/blob/9a2b5a1b81481eca197f16ec38e8017c7e72ac5f/src/run.jl#L142-L154
Which is a function called during run to actually check the inputs match the placeholders.
If they do not then it throws a new richer error message that also says which input failed to match.

[samoconnor]

@oxinabox

I suggest that ErrorException should be treated as if it were an uncatchable exception type.

Here is a related (out of date) attempt to implement uncatchable exceptions:

WIP: Uncatchable fatal errors

[taqtiqa-mark]

Hi, I’m trying to understand the best practice in situations such as where StatsBase.jl, uses error(...) liberally.

Is this type of usage of error(...)frowned upon?
If so what would best julia practice look like?
How would you write a test for this change so that if someone reverted it, there would be some indication?

Appreciate any hints. tips and suggestions.

[Tamas_Papp]

@taqtiqa-mark

Is this type of usage of error(...) frowned upon?

In this particular case, IMO MethodError would work fine and defining a method saying there is no method is kind of redundant. I am aware that some packages do this though, presumably to help the user.

@taqtiqa-mark

If so what would best julia practice look like?

Here, I would recommend just not defining a method.

@taqtiqa-mark

How would you write a test for this change so that if someone reverted it, there would be some indication?

See Test.@test_throws.

Note that these are just my stylistic preferences. Some older Julia packages were written at a time when best practices for Julia code were still being explored, and may reflect a style people no longer use, or not that widely. Make sure you check with the maintainers before making pure style PRs, are not universally encouraged (benefit vs cost of reviewing etc).

[taqtiqa-mark]

Thanks @Tamas_Papp, appreciate you sharing your thoughts and suggestions.