Julia: audit usages of `@assert`

[StefanKarpinski]

Assertions should only be used when it is believed to be logically impossible for the assertion to fail. The uses of @assert should be audited to make sure that this is the case. There seem to be many situations in the Julia code base where @assert is used to check for error conditions. These should be converted to do check() || error(...) instead.

[petershintech]

In fact, if possible, please be specific about the Exception type you want to throw rather than just calling error("..."). So please
check() || throw(SpecificError(...))

[aaronlws95]

Hi, I'm new here but I'd like to help if possible. I was looking at the code and I found that for Base.has_offset_axes mostly uses @assert !has_offset_axes(...) for checks but I also found has_offset_axes(a) && throw(ArgumentError("range must be indexed starting with 1")). Is there a reason for the difference?

[mbauman]

I don't think there's a meaningful reason there — both idioms could probably be unified into an ensure_non_offset_axes function that does the check and throws something like ArgumentError("offset arrays are not supported but got an array with axes $(axes(A))"). I think that'd make for a nice improvement.

[aaronlws95]

Adding the function ensure_non_offset_axes(A...) = has_offset_axes(A...) && ArgumentError("offset arrays are not supported but got an array with index other than 1") should work I think. $(axes(A)) won't work since has_offset_axes takes a list of array objects but doesn't tell you which is the faulty one.

Also, there are a fair amount of occurrences of @assert !has_offset_axes(...) and the documentation also recommends using it. Just want to check the consensus, if it's okay I'll go ahead and refactor.

[petershintech]

Hi, I'm new here but I'd like to help if possible. I was looking at the code and I found that for Base.has_offset_axes mostly uses @assert !has_offset_axes(...) for checks but I also found has_offset_axes(a) && throw(ArgumentError("range must be indexed starting with 1")). Is there a reason for the difference?

I think if a function is exported from a module, it needs to raise an exception so that an external program calling the function can handle it. However, the function is only used internally in the module, an @assert statement would be sufficient enough. Any standard practice for Julia programming?

[petershintech]

Adding the function ensure_non_offset_axes(A...) = has_offset_axes(A...) && ArgumentError("offset arrays are not supported but got an array with index other than 1") should work I think.

In the Julia code base, I can see more than 300 lines calling has_offset_axes().
How about defining a new exception type for the checks, rather than just raising a general ArgumentError, so that programmers can handle the specific exception if they want?

[JeffBezanson]

As Stefan said, an assertion should only be used for conditions believed to be logically impossible, not for checking arguments passed by an external caller. It looks to me like that documentation should be changed. Whether an assertion is the right thing to use is context specific, so each use needs to be audited.

[aaronlws95]

Hi, I updated the !has_offset_axes checks in the base folder. Please see if it is ok to extend it to cases in stdlib and update documentation.

[aaronlws95]

Updated has_offset_axes asserts in the rest of stdlib and documentation.

[Moelf]

what do we do with things like this?
https://github.com/JuliaLang/julia/blob/040a3e5a71632190a13b6bb9a5706c0cde3d6e0f/base/file.jl#L814-L817

[mbauman]

We can leave it alone — that's a case where it's logically impossible for the assertion to be reached.

[StefanKarpinski]

It is, in effect, a correct assertion: it asserts that it should never be reached. It could be rewritten as error("this shouldn't happen") or something like that.