Open tkf opened 4 years ago
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.
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
?
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.
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 https://github.com/JuliaLang/julia/issues/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.)
(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?
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
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!
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?
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###")
.
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 https://github.com/simeonschaub/OptionalArgChecks.jl/pull/8 but it wasn't clear.)
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 :upside_down_face:
It came up elsewhere recently: https://github.com/JuliaLang/julia/issues/33248#issuecomment-570092961 https://github.com/MikeInnes/Effects.jl
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
@skipargcheck
to @unsafe_skipargcheck
@unsafe_skipargcheck
to ArgCheck.ArgCheckLabel
If so I can provide PRs.I would be ok with that
IIUC, the effect of
@skipargcheck
is dynamically scoped: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