Warn OptionalArgChecks.jl's scoping rule?

[tkf]

IIUC, the effect of @skipargcheck is dynamically scoped:

using OptionalArgChecks
@noinline f(x) = @argcheck x > 0
@noinline g(x) = f(x)
h(x) = @skipargcheck g(x)
h(-1)  # no error

That is to say, @skipargcheck is effective no matter how far away the function using @argcheck is in the call chain. This is very different from @inbounds as it works only with inlined calls (i.e., @inbounds scoping is more-or-less lexical). On the other hand, there is no way for a callee to reliably throw an exception when using @argcheck. I understand that exception is not used as "output" in Julia most of the time. However, you can find in some places exception type is crucial (e.g., NLsolve.jl, Optim.jl).

I do think OptionalArgChecks.jl is a very interesting approach to optional error handling. But, if my understanding of its scope is correct, I think it's better to have a warning on its scoping rule. Ideally, I think it's better to have a separate macro for enabling OptionalArgChecks.jl.

cc @simeonschaub

[simeonschaub]

Making @skipargcheck recursive was definitely a conscious design decision, but I agree that this may not always be desired. It would be quite trivial though to add a keyword argument to turn off this behavior.

[jw3126]

I thought of OptionalArgChecks.@skipargcheck as trading safety for speed and I was aware of the scoping. I did not think of it as a way to manipulate control flow. But you are right @tkf this might be undesirable.

How about we mimic the design of @inbounds, @propagate_inbounds, @boundscheck? E.g. do not recurse @skip label unless a function is marked with @propagate_skip label?

[jw3126]

How about we mimic the design of @inbounds, @propagate_inbounds, @boundscheck? E.g. do not recurse @skip label unless a function is marked with @propagate_skip label?

Maybe this is not such a good idea. For skipping debug / logging / profiling code recursive is what you want and marking every function @propagate_skip debug ... is cumbersome.

[tkf]

I think @propagate_skip is actually the way to go. It's cumbersome but I can't think of a better way to annotate this clearly (which could be due to my lack of imagination).

Skipping argument checks kind of reminds me of the "chain of custody" error handling proposal JuliaLang/julia#7026 and I wonder if there is a unified syntax to neatly handle both cases. But the chain of custody is the "opposite" of optional argument check as it's about thrown error, not about error-to-be-thrown.

(BTW, if we go with @propagate_skip route I think it's implementable with normal dispatch + macro. It's a set of "boring" technologies but something stupid-simple may be better for fundamental thing like error handling.)

[jw3126]

(BTW, if we go with @propagate_skip route I think it's implementable with normal dispatch + macro. It's a set of "boring" technologies but something stupid-simple may be better for fundamental thing like error handling.)

You mean just the @propagate_skip or the whole familiy @skip, @mark, @propagate_skip is implementable with "boring" tech? If it is the whole family that would be great. Can you provide a gist in that case?

[tkf]

Here is a short snippet of what I was thinking:

module Unsafeables  # TODO: a better name?

abstract type UnsafeableFunction <: Function end

abstract type Caller <: Function end
struct SafeCaller <: Caller end
struct UnsafeCaller <: Caller end

(f::UnsafeableFunction)(args...; kwargs...) = f(SafeCaller(), args...; kwargs...)

(caller::Caller)(f::UnsafeableFunction, args...; kwargs...) = f(caller, args...; kwargs...)
# Fallback for normal functions:
(caller::Caller)(f, args...; kwargs...) = f(args...; kwargs...)

unsafe(f, args...; kwargs...) = UnsafeCaller()(f, args...; kwargs...)

struct F1 <: UnsafeableFunction end
const f1 = F1()

struct F2 <: UnsafeableFunction end
const f2 = F2()

"""
    f1(x)
    f2(x)

Argument checking of these functions can be skipped by `unsafe(f1, x)`, etc.
"""
(f1, f2)

function f1(caller, x)
    if !(caller isa UnsafeCaller)
        x > 0 || throw(ArgumentError("Require x > 0; got x = $x"))
    end
    return caller(f2, x)
end

function f2(caller, x)
    if !(caller isa UnsafeCaller)
        x != 0 || throw(ArgumentError("Require x != 0; got x = $x"))
    end
    return 1 / x
end

end

using Test
using UnPack: @unpack  # or Parameters.jl
@testset begin
    @unpack f1, f2, unsafe = Unsafeables
    @test f1(2) == 0.5
    @test_throws ArgumentError f1(-2)
    @test_throws ArgumentError f2(0.0)
    @test unsafe(f1, -2) == -0.5
    @test unsafe(f1, 0.0) == Inf
end

Above code for f1 and f2 should be straight-forward to generate. That is, with an annotation (say) @skippable, a function definition like this

@skippable function f(x)
    @argcheck x > 0
    ...
end

is lowered to

function f(caller, x)
    if !(caller isa UnsafeCaller)
        @argcheck x > 0
    end
    ...
end

i.e., @skippable just inserts the first caller argument and wrap @argcheck with if !(caller isa UnsafeCaller) block.

Above @skippable is a non-propagating version. For @propagate_skip, it can transform all call AST f(...) to caller(f, ...) outside of @argcheck; i.e.,

@propagate_skip function f(x)
    @argcheck x > 0
    ...
    g(x)
    ...
end

is lowered to

function f(caller, x)
    if !(caller isa UnsafeCaller)
        @argcheck x > 0
    end
    ...
    caller(g, x)  # `g` may or may not be an `UnsafeableFunction`
    ...
end

Since caller(g, x) just falls back to g(x) for non-UnsafeableFunction, @propagate_skip can just operate at syntax level.

I think that it is nice that selectively propagating "unsafeness" is straight-forward with this approach:

function f(caller, x)
    if !(caller isa UnsafeCaller)
        @argcheck x > 0
    end
    ...
    y = a_third_party_function_not_documented_well(x)
    z = caller(a_function_with_well_documented_domain, x)
    ...
end

which can have a syntax sugar like

@skippable function f(x)
    @argcheck x > 0
    ...
    y = a_third_party_function_not_documented_well(x)
    z = @mayskip a_function_with_well_documented_domain(x)
    ...
end

[simeonschaub]

I did think about a similar approach as well, but I didn't really like having to sprinkle @skippable on every function. In most of the cases I also wanted the recursive behaviour, since you can't propagate @skip through Base functions like this, for example. But I also see, how this is probably a much more robust solution and doesn't come with all the caveats that come with using IRTools, so why not support both!

[tkf]

I think it shouldn't be difficult to generalize this to a trait-ish-based approach so that it works with the functions you don't define. But I'm a bit ambivalent with this respect. If you don't define the function with "unsafe path" in mind, it's unlikely that the contract of the function is rigorously defined and enforced across all implementations. I think it's fair to say that the API definitions for @inbounds and @boundscheck are well-described compared to the other Base APIs.

I guess it's the usual tension between being succinct and explicit. I just think, for a widely-used "foundation" package like ArgCheck, it's better to play on the safe side. I know it's ugly but maybe using something like @unsafe_skipargcheck for the recursive version would make sense?

[jw3126]

Here is a short snippet of what I was thinking:

That snipped does look interesting. Sometimes I did use add hoc variants of this passing Unsafe() objects around. But it would be great to have a package that deals with this systematically.

But I also see, how this is probably a much more robust solution and doesn't come with all the caveats that come with using IRTools, so why not support both!

Yes, I also think that both approaches are useful. It would be good to have @tkf s approach in an extra (close to?) zero dependencies package.

I know it's ugly but maybe using something like @unsafe_skipargcheck for the recursive version would make sense?

I think renaming the current variant to @unsafe_skipargcheck is a great idea. While we are at it, we should also obfuscate the label to something like Symbol("###ArgCheck.argcheck###").

[tkf]

It would be good to have @tkf s approach in an extra (close to?) zero dependencies package.

If you need it sometime soon, please feel free to package it up :) I don't need this for code I'm writing right now, so it may take sometime until I do this.

(Also, I think it needs a bit more careful design. For example, reading it again, I find (f::UnsafeableFunction)(args...; kwargs...) = f(SafeCaller(), args...; kwargs...) a bit scary as it could invoke an infinite recursion. It's probably a bit better to lower @skippable function f(x) to function (caller::Caller)(::typeof(f), x) to avoid recursion.)

While we are at it, we should also obfuscate the label to something like Symbol("###ArgCheck.argcheck###").

Is this for avoiding name crash? If so, can label be arbitrary thing that can be put in the type parameter? Then defining a type (say) ArgCheck.ArgCheckLabel sounds like a cleaner solution. (Sorry if it was already discussed. I looked at simeonschaub/OptionalArgChecks.jl#8 but it wasn't clear.)

[tkf]

Or... Create a full-blown algebraic effects system (see, e.g., https://overreacted.io/algebraic-effects-for-the-rest-of-us/) and implement optional argument checking on top of that 🙃

It came up elsewhere recently:
JuliaLang/julia#33248 (comment)
https://github.com/MikeInnes/Effects.jl

[jw3126]

I don't know enough about algebraic effects to judge whether they are a good long term solution to the problem. But short term, we need something different.

@simeonschaub Are you okay with

1. Changing the name @skipargcheck to @unsafe_skipargcheck
2. Changing the label of @unsafe_skipargcheck to ArgCheck.ArgCheckLabel
   If so I can provide PRs.

[simeonschaub]

I would be ok with that