julia
julia copied to clipboard
Add `@check` macro for non-disable-able `@assert`
I've found that sometimes folks use @assert
for its simple syntax and clear error message in cases that are inappropriate for asserts (e.g. checking that data is consistent rather than program logic is consistent). I think it would be great to provide an alternative with an equally simple syntax and clear error messages that will never be disabled. At the same time, we can use the opportunity of the macro to outline the string creation to improve performance. E.g. with this PR,
function hv_cat(rows::Tuple{Vararg{Int}}, xs::Int...)
nr = length(rows)
nc = rows[1]
for i = 2:nr
@audit nc == rows[i]
end
Base.hvcat_fill!(Matrix{Int}(undef, nr, nc), xs)
end
matches the "fast" performance of https://github.com/JuliaLang/julia/issues/41307 for me (~23ns, instead of ~80ns for the "slow" performance). (~~I made the same tweaks to @assert
here, so @assert
would work as well, although that would be an incorrect usage of it here~~ nevermind, took that out due to bootstrap issues. Which incidentally is why there's code duplication from @assert
and prepare_error
).
I also experimented with using code from FastCaptures to outline the whole logic check + error (as suggested by @KristofferC in https://github.com/JuliaLang/julia/issues/29688#issuecomment-434860725) in https://github.com/JuliaLang/julia/commit/6a6b9accad2c14545ac7a6816f5a3f840e451659, but in the hv_cat
example it was the same speed as not outlining at all. Perhaps I made a mistake (I am not very experienced with macros), though from @code_warntype
and @macroexpand
it looked like it was doing what I wanted.
@kleinschmidt jokingly suggested the name @audit
when I was calling it @throw_unless
and I ended up liking it so I figured I'd use it here.
closes: https://github.com/JuliaLang/julia/issues/10614
Regarding the name, maybe @require
?
The ArgCheck.jl package has the @check
macro, which seems similar.
@require
is already taken by Requires.jl. How about @ensure
?
Regarding the name, maybe
@require
?
I thought that could be confusing since Base.require
is very different, but that's not a public API anyway so maybe it's fine? @require
does get the point across well.
The ArgCheck.jl package has the
@check
macro, which seems similar.
True, it does. One difference is the outlining feature, but I'm sure that could be added to ArgCheck.jl. One reason I think it would be good to have something like this in Base is that @assert
is in Base and it would be nice to have an equally available alternative to help push users away from incorrect use of @assert
s. I think sometimes @assert
is used out of convenience but one-off code has a way of sticking around...
True, it does. One difference is the outlining feature, but I'm sure that could be added to ArgCheck.jl. One reason I think it would be good to have something like this in Base is that
@assert
is in Base and it would be nice to have an equally available alternative to help push users away from incorrect use of@assert
s.
I agree. I would love it if the ArgCheck package could be deprecated and Base would have that functionality instead.
One thing ArgCheck does is put energy into creating good error messages, a bit like @test
.
Observe that while @assert
only puts compile time information into the error, @test
and @check
provide information about runtime values:
julia> using Test, ArgCheck
julia> x = NaN
NaN
julia> @assert isfinite(x)
ERROR: AssertionError: isfinite(x)
julia> @test isfinite(x)
Test Failed at REPL[28]:1
Expression: isfinite(x)
ERROR: There was an error during testing
julia> @check isfinite(x)
ERROR: CheckError: isfinite(x) must hold. Got
x => NaN
julia> @assert x ≈ 2
ERROR: AssertionError: x ≈ 2
julia> @test x ≈ 2
Test Failed at REPL[32]:1
Expression: x ≈ 2
Evaluated: NaN ≈ 2
ERROR: There was an error during testing
julia> @check x ≈ 2
ERROR: CheckError: x ≈ 2 must hold. Got
≈ => isapprox
x => NaN
@check
does seem like the best name for this.
I started working on tests and realized the outlined function in my code only works if the msg you pass is a string, not an exception or even arbitrary code like @assert
allows (and evaluates when triggered). I think this is due to eval’ing it, but I did want to generate the function in global scope at macro expansion time. Maybe that is the wrong approach though. I’d appreciate any pointers.
I started working on tests and realized the outlined function in my code only works if the msg you pass is a string, not an exception or even arbitrary code like
@assert
allows (and evaluates when triggered). I think this is due to eval’ing it, but I did want to generate the function in global scope at macro expansion time. Maybe that is the wrong approach though. I’d appreciate any pointers.
You can take a look at ArgCheck.jl. It does outline the error generation. It only inlines the throw, which would be trivial to outline as well.
@macroexpand @argcheck x > 1 DimensionMismatch
begin
#= /home/jan/.julia/packages/ArgCheck/5xEDR/src/checks.jl:239 =#
var"#1###calle#274" = (>)
var"#2###arg#272" = x
var"#3###arg#273" = 1
#= /home/jan/.julia/packages/ArgCheck/5xEDR/src/checks.jl:240 =#
if var"#1###calle#274"(var"#2###arg#272", var"#3###arg#273")
#= /home/jan/.julia/packages/ArgCheck/5xEDR/src/checks.jl:241 =#
ArgCheck.nothing
else
#= /home/jan/.julia/packages/ArgCheck/5xEDR/src/checks.jl:243 =#
ArgCheck.throw(ArgCheck.build_error(ArgCheck.CallErrorInfo($(QuoteNode(:(x > 1))), ArgCheck.ArgCheckFlavor(), $(QuoteNode(Any[:>, :x, 1])), [var"#1###calle#274", var"#2###arg#272", var"#3###arg#273"], (DimensionMismatch,))))
end
end
Ok, I removed the outlining exceptions bit because I couldn't figure out how to do it quickly (despite the nice example provided by ArgCheck.jl), but I figure that can be done in a future PR since it doesn't change the semantics. I also renamed it to @check
, and added NEWS, docs, and tests (by copying @assert
).
Bump. Would be nice to get this in for 1.8 or to decide we don’t want it (or want it if …).
Bump 🤞
Bump @KristofferC @StefanKarpinski
It would be great to get this into 1.8.
Bump
Why not copy code from ArgCheck.jl which provides strictly more information and also is battle-tested? We can optimize this more later using the same logic as https://github.com/JuliaLang/julia/pull/41342#issuecomment-1002369980
Just because this is simpler and having a nice alternative to @assert
to use and to point to in code reviews is more important to me than performance (outlining etc) here. In terms of battle-tested this is just @assert
which seems plenty battle-tested. But if the fancier implementation will help get something merged then I’ll do it :).
Quoting your comment in Zulip
I sometimes want things in the stdlib because there’s something else similar there and I don’t want that to be favored by virtue of not needing dependencies.
I agree with your observation. However, this PR as-is seems to make Base.@check
favorable over ArgChecks.@check
while the latter makes debugging easier and thus makes the ecosystem more robust.
That said, I don't know enough about the principle on which the core devs operate and so I can't guess which way can be more easily accepted.
My take here is that some package exporting a name can't be considered to block Base from exporting the same name. @check
seems like the most obvious name for this; people who use ArgCheck
can/should import ArgCheck: @check
. The name @ensure
is ok too though. We should do something.
What about @assume
as an alternative to @check
? According to juliahub, that is still free and IMO has a much stronger association with not being removed than @check
does.
I don't see how @assume
is associated with not being removed. If anything, I'd think that @assume x > 1
is saying "I promise that x is greater than 1, you don't need to check it"
It's just a different suggestion to avoid conflicts with the ecosystem :shrug: In Hypothesis (and my soon-to-be-released port of it), assume
is a verb to designate conditions that MUST be true, otherwise an error is thrown (which is handled by the framework, but that's an irrelevant detail). It's not ever removed.
I'm fine with @ensure
too, I just want to avoid having conflicts between what an established package means with @check
and what Base means.
avoid having conflicts between what an established package means with @check and what Base means
there's no conflict here with meaning; semantically, this PR's @check
is identical in meaning to ArgCheck's.
Maybe @jw3126 would be willing to upstream ArgCheck’s @check
macro to Base? It is fuller featured though more complicated than this PR which just duplicates @assert
with a new name. Then ArgCheck could just re-export it on Julia versions where Base has it.
I would love to have ArgCheck
in Base. However, I have experienced quite a few PRs to julia in the past, where the following happened:
- Somebody put work into a PR
- The PR then it did not get the attention of the right people and would neither be merged nor rejected, see for instance https://github.com/JuliaLang/julia/pull/33495#issuecomment-1541409518
Because of this, I am hesitant to put work into julialang PRs. I am happy to do the work, but only if triage or some core dev states that we want it and promises that the PR either gets merged or rejected for good technical reasons.
Why can't the answer here be to just use ArgCheck.jl? It looks great, you can get updates whenever etc.
- People abuse
@assert
since it’s “easier” since you don’t need a new dependency - If we start turning off
@assert
sometimes (as we have every right to do) it’s easier to fix code with a drop-in replacement that doesn’t need a new dependency to do so
BTW, ArgCheck hasn't had any changes since June 2022, so the slow release cadence of Base actually seems totally fine at this stage of maturity.
Twocents: I would prefer @assert
be made the non-disabled version, then add a new @debug_assert
that can be turned off sometimes. Reasoning:
-
@assert
is currently used a lot of places where it is assumed to always be turned on, even though the warning reserves the right to change that behavior (this is a problem in the Python ecosystem too). Introducing something new for the behavior of https://github.com/JuliaLang/julia/pull/37874 is much less likely to break things, even if those cases aren't technically doing the correct thing. - The difference between the
assert
anddebug_assert
is pretty self explanatory, and I think it is pretty unlikely for someone to mistakenly usedebug_assert!
to enforce code correctness. I don't findcheck
vs.assert
to be as immediately clear. - There is already some prior teaching available for shared knowledge - Rust uses
assert
anddebug_assert
and does a pretty good job of explaining when either should be used. Kotlin is the only language I know of to useassert
andcheck
terminology (alsorequire
), but I don't think it does as good a job explaining when either should be used.
I think doing this would also make https://github.com/JuliaLang/julia/pull/37874 more palpable, since some of the more recent objections are assert-related.
@assert
is currently used a lot of places where it is assumed to always be turned on, even though the warning reserves the right to change that behavior (this is a problem in the Python ecosystem too). Introducing something new for the behavior of RFC: implement proper debug mode for packages (with support for re-precompilation) #37874 is much less likely to break things, even if those cases aren't technically doing the correct thing.
I agree with that.
Another choice is to use a level
parameter to the macro and consider a default choice somewhere like for logging
macro logmsg(level, exs...) logmsg_code((@_sourceinfo)..., esc(level), exs...) end
macro debug(exs...) logmsg_code((@_sourceinfo)..., :Debug, exs...) end
macro info(exs...) logmsg_code((@_sourceinfo)..., :Info, exs...) end
macro warn(exs...) logmsg_code((@_sourceinfo)..., :Warn, exs...) end
macro error(exs...) logmsg_code((@_sourceinfo)..., :Error, exs...) end
the macro may be in base. differents behaviors may even be handled in different module via multidispatch. That may require a dispatch on singleton-struct-based type hierarchy like iterators trait or some type lift of bitstype via Val.