Deprecate assert function in favor of at-assert macro

[mzaffalon]

fixes first half of #8856

[jakebolewski]

👎 this is used in a lot of code. I don't get what we gain here for all the code churn.

[mzaffalon]

I am not sure I understand what you mean.

[jakebolewski]

Every time someone deprecates something, that means broken code in 2 release cycles or piles of deprecation warnings to go through and fix.

[ivarne]

@jakebolewski Am I correct that your objection is to the deprecation of assert() in favor of @assert (essentially the intro issue #8856) , not to the quality of a PR from a third time contributer.

[jakebolewski]

Of course, sorry for the implied tone (my anger has been honed by fixing too many deprecation warnings).

[IainNZ]

At a quick search of Github for assert in Julia code, @assert is a lot more popular, and it does open up the possibility of allowing @asserts to be removed in an "optimized" mode.

[IainNZ]

This is what I'm basing that on:
https://github.com/search?l=Julia&q=assert&type=Code&utf8=%E2%9C%93

[jakebolewski]

Just because one is used more, doesn't mean that they can't coexist.

[IainNZ]

Two ways to do same thing, where one is inflexible and has quite a different meaning to the C version (in my mind at least, given I think of assert has not affecting release builds), doesn't seem like a good long term idea.
I'm not interested in burning much energy on fighting for this, but I do feel that in a "could do it all again" world we wouldn't have two ways to do the same thing - so why not change now?

[StefanKarpinski]

+:100:. Having two such similar things in Base is annoying and pointless. The deprecation objection is, imo, kind of lazy and defeatist. Sure, dealing with deprecations is tedious, but the alternative is to settle for leaving things permanently in a broken, crappy state just because it's annoying to fix them. The only legitimate argument for not deprecating something we know is lousy is that we're not sure what would be better. In this case that argument doesn't apply.

[StefanKarpinski]

The export is probably not necessary here since the @deprecate macro automatically exports.

[jakebolewski]

It is not annoying and pointless. One is much more efficient than the other because it doesn't have to allocate.

[StefanKarpinski]

A better solution would be to figure out how to avoid allocation in the macro version which is used far more often.

[jakebolewski]

sure, but you are advocating removing one version when there is no alternative in place.

[StefanKarpinski]

The performance argument is a very valid one, so unfortunately I guess we'll have to wait on this.

[IainNZ]

I'm not sure if I mentioned it in my initial issue, but I don't think @assert should be doing anything but checking its argument is true. Anything fancier should be using @test.

[IainNZ]

I really see no point to @assert unless it compiles away under some condition but not others.

[ScottPJones]

Is @assert at the moment always being compiled? Could it be changed to only generate code for the julia-debug version (which is what C/C++ programmers would expect)?
If that were the case, I wouldn't think the performance argument for keeping assert is really that compelling.

[IainNZ]

It is always being compiled, which was my suggestion here #8856 (comment)

[jakebolewski]

@IainNZ I'm not dead set against the change you point out, only deprecating something without providing a compelling enough alternative.

[carnaval]

We should have a way to disable assert statements but I don't think the debug build is a good way. People don't usually keep around two versions of the executable. A command line argument is easier. It leaves the question open of already compiled code obviously.

[ScottPJones]

Have the generated code always test a global flag, and generate nothing at all if it wasn't already compiled. State of this flag may need to go in cached precompiled file.

[jakebolewski]

Related: #10614. @ScottPJones what @carnaval is saying is that you can never enforce that all the assertions are turned on or off, it depends on the phasing of when code is precompiled and what global flags were or were not turned on at the time.

[ScottPJones]

@jakebolewski Yes, that is why I said, if it does compile it in, it should still have a run-time check of the global flag, before actually executing the assertion test (which could be expensive).
It is also why any global flags that affect how code is compiled need to be saved in any precompiled cached code, and checked to see if the cached code is actually what you want to run (it might be a good idea to cache keep separate caches for some things, if you might be jumping back and forth between say running a no-inlined build for coverage purposes, and a debugging version with assertions enabled)

[mzaffalon]

@carnaval

People don't usually keep around two versions of the executable.

Who is people? Developer? As a normal user, I am perfectly happy to rely on whatever decision regarding assertions the developer has taken. I am already relying on the developer for decision regarding the algorithm.

I have a limited understanding of assertions, so I am probably wrong, but shouldn't there be assertions that have to be run no matter what (and it is up to the developer to make sure they do not impact performance) and debug assertions that are turned off in high-performance code?

Please do ignore this comment if it doesn't make any sense...

[tkelman]

The discussion in #10614 (and I think at least one other earlier issue that I can't seem to find) went pretty much that same direction. There are some safety critical assertions you never want to disable and some check-only assertions that you'd want to turn off in "release" code somehow. But anyway I don't think you need to solve that issue here.

I'm in favor of deprecating the assert function since it's almost completely redundant. It's worth quantifying how serious of a difference there is with respect to

One is much more efficient than the other because it doesn't have to allocate.

[ScottPJones]

Tests that you always want performed should be done as cond && throw(AppropriateError(args))
@assert cond should just be for sanity check code, that may be expensive, which should not be needed in production code.

[mzaffalon]

So for instance in https://github.com/JuliaLang/julia/blob/master/base/inference.jl#L766, the line should be

n != length(types) && throw(ArgumentError("Dimensions do not match"))

[carnaval]

@michele-zaffalon I'm just saying that since we can compile julia code on the fly independently of the julia executable it does not make sense to tie a codegen option (assertions-enabled) with the julia build version (debug).

As for this example, I think in that case the argument types array is always generated inside inference so this assertion should not trigger even on broken AST. However, if your point is that it's so cheap compared to what's being done there that we might as well just want to leave it in anyway, you're right.

[eschnett]

Or maybe

n == length(types) || throw(ArgumentError("Dimensions do not match"))

since this is closer to an assert, and states the condition that will hold in the code below.

[ScottPJones]

@eschnett Yes (although for that particular case, maybe you should use a DimensionMismatch() error?)
I looked, and in Base, the use of cond && throw and cond || throw are fairly equal (172 vs. 212), somewhat more for the later, but in the registered packages, || is used a lot more (115 vs. 721).
I personally always use &&, simply because to me, it is more like if cond ; throw(error) ; end

[ararslan]

Superseded by #26194.