julia icon indicating copy to clipboard operation
julia copied to clipboard

Add `@check` macro for non-disable-able `@assert`

Open ericphanson opened this issue 3 years ago • 27 comments

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

ericphanson avatar Jun 24 '21 02:06 ericphanson

Regarding the name, maybe @require?

simeonschaub avatar Jun 24 '21 10:06 simeonschaub

The ArgCheck.jl package has the @check macro, which seems similar.

jw3126 avatar Jun 24 '21 11:06 jw3126

@require is already taken by Requires.jl. How about @ensure?

jpsamaroo avatar Jun 24 '21 11:06 jpsamaroo

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 @asserts. I think sometimes @assert is used out of convenience but one-off code has a way of sticking around...

ericphanson avatar Jun 24 '21 11:06 ericphanson

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 @asserts.

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

jw3126 avatar Jun 24 '21 13:06 jw3126

@check does seem like the best name for this.

StefanKarpinski avatar Jun 24 '21 15:06 StefanKarpinski

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.

ericphanson avatar Jun 25 '21 23:06 ericphanson

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

jw3126 avatar Jun 26 '21 04:06 jw3126

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).

ericphanson avatar Dec 29 '21 02:12 ericphanson

Bump. Would be nice to get this in for 1.8 or to decide we don’t want it (or want it if …).

ericphanson avatar Feb 03 '22 04:02 ericphanson

Bump 🤞

ericphanson avatar Feb 13 '22 21:02 ericphanson

Bump @KristofferC @StefanKarpinski

DilumAluthge avatar Feb 13 '22 21:02 DilumAluthge

It would be great to get this into 1.8.

DilumAluthge avatar Feb 13 '22 21:02 DilumAluthge

Bump

ericphanson avatar Mar 16 '22 02:03 ericphanson

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

tkf avatar Mar 16 '22 03:03 tkf

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 :).

ericphanson avatar Mar 16 '22 04:03 ericphanson

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.

tkf avatar Mar 16 '22 06:03 tkf

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.

StefanKarpinski avatar Feb 15 '24 14:02 StefanKarpinski

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.

Seelengrab avatar Feb 15 '24 14:02 Seelengrab

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"

MasonProtter avatar Feb 15 '24 14:02 MasonProtter

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.

Seelengrab avatar Feb 15 '24 14:02 Seelengrab

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.

ericphanson avatar Feb 15 '24 15:02 ericphanson

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.

ericphanson avatar Feb 15 '24 22:02 ericphanson

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.

jw3126 avatar Feb 16 '24 07:02 jw3126

Why can't the answer here be to just use ArgCheck.jl? It looks great, you can get updates whenever etc.

KristofferC avatar Feb 16 '24 10:02 KristofferC

  1. People abuse @assert since it’s “easier” since you don’t need a new dependency
  2. 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

ericphanson avatar Feb 16 '24 11:02 ericphanson

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.

ericphanson avatar Feb 16 '24 13:02 ericphanson

Twocents: I would prefer @assert be made the non-disabled version, then add a new @debug_assert that can be turned off sometimes. Reasoning:

  1. @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.
  2. The difference between the assert and debug_assert is pretty self explanatory, and I think it is pretty unlikely for someone to mistakenly use debug_assert! to enforce code correctness. I don't find check vs. assert to be as immediately clear.
  3. There is already some prior teaching available for shared knowledge - Rust uses assert and debug_assert and does a pretty good job of explaining when either should be used. Kotlin is the only language I know of to use assert and check terminology (also require), 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.

tgross35 avatar Mar 21 '24 22:03 tgross35

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.

o314 avatar Mar 21 '24 23:03 o314