Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add @check macro for non-disable-able @assert #41342

Open
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

ericphanson
Copy link
Contributor

@ericphanson ericphanson commented Jun 24, 2021

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 #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 #29688 (comment)) in 6a6b9ac, 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: #10614

base/error.jl Outdated Show resolved Hide resolved
@ararslan ararslan added error handling Handling of exceptions by Julia or the user needs compat annotation Add !!! compat "Julia x.y" to the docstring needs news A NEWS entry is required for this change needs tests Unit tests are required for this change labels Jun 24, 2021
@simeonschaub
Copy link
Member

Regarding the name, maybe @require?

@jw3126
Copy link
Contributor

jw3126 commented Jun 24, 2021

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

@jpsamaroo
Copy link
Member

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

@ericphanson
Copy link
Contributor Author

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

@jw3126
Copy link
Contributor

jw3126 commented Jun 24, 2021

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

@StefanKarpinski
Copy link
Sponsor Member

@check does seem like the best name for this.

@ericphanson
Copy link
Contributor Author

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.

@jw3126
Copy link
Contributor

jw3126 commented Jun 26, 2021

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

base/error.jl Outdated
msg = prepare_error(ex, msgs...)
fn = gensym("audit")

@eval @noinline function $(fn)()
Copy link
Sponsor Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps these should all be put into some submodule to not pollute the namespace of e.g. Base too much?

@ericphanson ericphanson changed the title Add @audit macro for non-disable-able @assert with outlined exceptions Add @check macro for non-disable-able @assert Dec 29, 2021
@ericphanson
Copy link
Contributor Author

ericphanson commented Dec 29, 2021

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

@DilumAluthge DilumAluthge requested review from KristofferC and StefanKarpinski and removed request for StefanKarpinski and KristofferC February 3, 2022 04:48
@ericphanson
Copy link
Contributor Author

Bump 🤞

@DilumAluthge
Copy link
Member

Bump @KristofferC @StefanKarpinski

@DilumAluthge
Copy link
Member

It would be great to get this into 1.8.

@ericphanson
Copy link
Contributor Author

Bump

@tkf
Copy link
Member

tkf commented Mar 16, 2022

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 #41342 (comment)

@ericphanson
Copy link
Contributor Author

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

@tkf
Copy link
Member

tkf commented Mar 16, 2022

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.

@StefanKarpinski
Copy link
Sponsor Member

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.

@Seelengrab
Copy link
Contributor

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.

@MasonProtter
Copy link
Contributor

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"

@Seelengrab
Copy link
Contributor

It's just a different suggestion to avoid conflicts with the ecosystem 🤷 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.

@ericphanson
Copy link
Contributor Author

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
Copy link
Contributor Author

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.

@jw3126
Copy link
Contributor

jw3126 commented Feb 16, 2024

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:

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.

@KristofferC
Copy link
Sponsor Member

KristofferC commented Feb 16, 2024

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

@ericphanson
Copy link
Contributor Author

  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
Copy link
Contributor Author

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.

@tgross35
Copy link

tgross35 commented Mar 21, 2024

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 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.
  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 #37874 more palpable, since some of the more recent objections are assert-related.

@o314
Copy link
Contributor

o314 commented Mar 21, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
error handling Handling of exceptions by Julia or the user
Projects
None yet
Development

Successfully merging this pull request may close these issues.

omit assert under --optimize; introduce@check macro?