-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
@require -- like assert but throws ArgumentError #15495
base: master
Are you sure you want to change the base?
Conversation
+1 Seems like 32 bit Travis timed out, anyone have an idea why that might happen? |
@@ -49,3 +49,34 @@ macro assert(ex, msgs...) | |||
end | |||
:($(esc(ex)) ? $(nothing) : throw(Main.Base.AssertionError($msg))) | |||
end | |||
|
|||
|
|||
macro throw_argument_error(msg) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should not be a macro. It's much better to be a function. (and you don't want this to be inlined.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes a lot of sense.
Done: samoconnor@258aa74
I find this interesting since it would raise the average quality of error messages. Most of the time, people write things like For the name, one could also think of FWIW, this makes me think of
In their model, |
Interesting idea, I like the concept at least. Counter-argument is maybe, does The |
The So |
Swift uses exactly this verbiage for something similar. Assertions are completely removed in optimized builds, whereas preconditions are only removed in unchecked builds. Interestingly, Swift still manages to propagate the precondition knowledge to LLVM even when the check is removed. So I can see a case here, too, even if we don't have the infrastructure (yet) to exploit it. https://www.mikeash.com/pyblog/friday-qa-2016-03-04-swift-asserts.html |
JuliaLang#15495 (comment) Move @precondition out of error.jl to avoid bootstrap failure caused by trying to use @noinline in error.jl.
Done: samoconnor@9d75e4c
@nalimilan 😀 I originally used
@mbauman The use of |
9d75e4c
to
b8fa0ba
Compare
function split{T}(c::T, n::Int) @precondition n > 0 return SplitIterator{T}(c, n) end split("xxx", 0) ERROR: ArgumentError: split(::ASCIIString, ::Int64) requires n > 0 Make throw_argument_exception @noinline as suggested by yuyichao JuliaLang#15495 (comment) Move @precondition out of error.jl to avoid bootstrap failure caused by trying to use @noinline in error.jl.
Disclosure of bias: I spent a couple of years in Santa Barbara working for eiffel.com in the late '90s. And, I later implemented Design by Contract in Tcl. Some might see that as a sign of madness. |
It seems that, to implement an @contract function f(x)
@pre x>=0
@post result>=0
return sqrt(x)
end could be translated into function f(x)
pre(x>=0, "x>=0")
result = f_impl(x)
post(result>=0, "result>=0")
result
end
function f_impl(x)
return sqrt(x)
end I started implementing this in https://github.com/eschnett/Contracts.jl, but things got a bit tedious regarding interpreting the expressions representing the function and its various conditions, so I didn't continue. I'm sure there is a simpler way to implement this, in particular if one uses macros for the pre-/post-conditions (as is suggested here). I used function calls that need to be inspected in the |
PS: To show off my experience with contract languages I'm using the Sather http://www1.icsi.berkeley.edu/~sather/Publications/article.html keywords here. (Sather is a close descendant of Eiffel.) |
@eschnett, could If I was being bold, I would even argue that e.g. function f()
...
finally
...
end |
@samoconnor That's probably possible, but I think that would be much more expensive. I think one might be able to use the post(cond) do
... original function body ...
end where However, both approaches still require wrapping the whole function body. It might not be necessary to create a separate named function, but instead, a closure needs to be generated -- I don't think that's any easier nor more efficient. |
This strikes me as redundant and unnecessary, and overly complicated for the goal of pre populating an error message (and copy-pasting code is never a good sign). Backtrace collection is slow, this is collecting an extra one to add function name information to the error message that will already be in the backtrace. It's getting towards |
Does anyone know if there is a way to provide branch prediction hints to llvm from Julia? When I've implement contract checking systems in the past I've always used something like |
@tkelman In case you were commenting on my post just above yours: What I showed was intended to show an example of the code that an expansion of an |
@eschnett no I wasn't commenting on postconditions since this PR isn't doing that yet, I was commenting on what this PR currently implements. |
@tkelman You're right that an implementation should be found to avoid any performance impact on the normal case. But do we care about performance when an @samoconnor I have to admit I know very little about Eiffel (too bad for my French chauvinism...). Regarding your PR, what do you think of showing the passed value of the invalid argument in addition to the condition? For example, |
@samoconnor You can use http://llvm.org/docs/LangRef.html#llvm-expect-intrinsic . I tried this recently (see below for an incomplete sketch), but code quality was reduced (!), so I didn't investigate further. @generated function llvm_expect{I}(x::Union{Bool, IntTypes}, ::Type{Val{I}})
# I::Integer
lt = llvmtypes[x]
ival = isa(I, Bool) ? Int(I) : I
decls = """
declare $lt @llvm.expect.$lt($lt, $lt)
"""
instrs = """
%res = call $lt @llvm.expect.$lt($lt %0, $lt $ival)
ret $lt %res
"""
quote
:(Expr(:meta, :inline))
llvmcall(($decls, $instrs), $x, Tuple{$x}, x)
end
end
@inline llvm_assume(cond::Bool) =
llvmcall(("""
declare void @llvm.assume(i1)
""", """
call @llvm.assume(i1 %0)
ret void
"""), Void, Tuple{Bool}, cond) |
FWIW |
@eschnett thanks for sharing your experience with |
@nalimilan, I agree with your conclusion that performance is moot in the failure case. The only place that has any business I very much like the idea of including the value that caused the failure (and maybe also the other function argument values). Perhaps: If this PR is accepted and people start writing |
If you have a macro just for the precondition, then it cannot capture the function arguments. If you add a macro before the function (similar to |
@tkelman there is plenty of literature and practical experience on this topic. If you want to be better informed, a good place to start might be: Applying “Design by Contract, Meyer, IEEE Computer, 1992 |
@MikeInnes thanks for pointing that out. |
e67c3ae
to
1089f18
Compare
function split{T}(c::T, n::Int) @precondition n > 0 return SplitIterator{T}(c, n) end split("xxx", 0) ERROR: ArgumentError: split(::ASCIIString, ::Int64) requires n > 0 Make throw_argument_exception @noinline as suggested by yuyichao JuliaLang#15495 (comment) Move @precondition out of error.jl to avoid bootstrap failure caused by trying to use @noinline in error.jl.
412584e
to
0085bab
Compare
renamed |
macro require(precondition, msgs...) | ||
msg = isempty(msgs) ? string(precondition) : msgs[1] | ||
:(if ! $(esc(precondition)) precondition_error($msg) end) | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since macros can have optional arguments now @require
could be defined a bit shorter as
macro require(precondition, msg = string(precondition))
:(if ! $(esc(precondition)) precondition_error($(esc(msg)) end)
end
(+1 to having a @require
in Base.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thx for the tip @MichaelHatherly
0085bab
to
db5d46d
Compare
I still don't like this. I think experimenting with more advanced forms of design by contracts is better done out in packages for now, and this macro by itself is just slightly standardizing the error message which isn't worth the double collection of backtraces. Actually doing some interesting and worthwhile analysis with the precondition would require a different exception type that saves an Expr of the precondition, or does an outer macro annotation of the entire function declaration. edit: To be more constructive, I'd be fine with a version of this that made throwing a precondition violation very cheap, and deferred any string concatenation and/or backtrace collection until such an exception gets displayed. |
Yes, this should definitely go in a package for now. |
@StefanKarpinski my motivation for this was just to be able to use simple precondition asserts in a PR for I have code like this which is apparently not allowed because function batchsplit(c; min_batch_count=1, max_batch_size=100)
@assert min_batch_count > 0
@assert max_batch_size > 1 It seems unfortunate to have to write it out like this... (which is why I'm proposing function batchsplit(c; min_batch_count=1, max_batch_size=100)
if !(min_batch_count > 0)
throw(ArgumentError("batchsplit(c) requires min_batch_count > 0"))
end
if !(max_batch_size > 1)
throw(ArgumentError("batchsplit(c) requires min_batch_size > 1"))
end I'm happy to write the preconditions out long-hand if that is what you prefer. Would you accept a PR for a non-exported macro require(precondition, msg = string(precondition))
esc(:(if ! $precondition Base.precondition_error($msg) end))
end |
It sort of makes sense but I'm not sure that that the kind of error raised matters enough given our current exception system (which doesn't really care about exception types much), and it feels half-baked without a corresponding postcondition macro, which would need a mechanism to ensure covering all exit paths. Once we have something like |
Ok, so in the meantime, are happy for me to write |
Throwing ArgumentError with a descriptive message is the current idiomatic style of error handling for user-facing code.
|
Quick postcondition prototype, possibly only working on Lazy/MacroTools master: using MacroTools
import Lazy: tailcalls
macro post(ex)
@capture(shortdef(ex), cond_ -> f_(args__) = body_) ||
error("invalid @post syntax")
@gensym ret
body = tailcalls(body) do ex
:($ret = $ex;
@assert $cond;
$ret)
end
return esc(:($f($(args...)) = $body))
end
@post (n == 0) ->
function foo(x)
n = 0
rand(Bool) && (n += 1; return x^3)
x^2
end |
Can one access the function's return value with this approach? In the long run, you also want the original values of the function arguments available in the postcondition. |
You get access to whatever's in scope at the point of return, so both of those things are possible. In the above example you can just acess |
Something along these lines might work... function foo(x)
@require x > 0
@ensure n == 0
n = 0
rand(Bool) && (n += 1; @return x^3)
@return x^2
end
or it could expand to something like this... function foo(x)
@assert x > 0
@goto _start
@label _postcondition
@assert n == 0
return result
@label _start
n = 0
rand(Bool) && (n += 1; (result = x^3; goto _postcondition)
result = x^2; goto _postcondition
end |
Or how about an ensure macro that takes an explicit block of code and
|
@toivoh I initially had the same thought, however try blocks are way too slow for this. They would slow down the code by an order of magnitude. Edit: Fixed some typos. Something like this would be better - at least as a stop-gap: function ...(n::Integer)
@require n == 0
@ensure @result > 0, iseven(@result) begin
n *= 2
(n % 2 == 0) || (return n + 1)
return n
end
end which could expand to: function ...(n::Integer)
(n == 0) || throw(ContractViolation("required n == 0; got n = $n"))
@gensym ret
n *= 2
if !(n % 2 == 0)
ret = n + 1
else
ret = n
end
(ret > 0) || throw(ContractViolation("ensured @result > 0; got $ret"))
iseven(ret) || throw(ContractViolation("ensured iseven(@result); got $ret"))
return ret
end Of course, ideally you wouldn't have to wrap the function ...(n::Integer)
(n == 0) || throw(ContractViolation("required n == 0; got n = $n"))
ccall(:jl_ensure, :(@result > 0))
ccall(:jl_ensure, :(iseven(@result)))
n *= 2
(n % 2 == 0) || (return n + 1)
return n
end In this case, the expressions passed to The only remaining issue would be how to---if indeed it would be required---have the name of the enclosing function (and also presumably, the specific method) which these contract-statements were declared in, be shown in the error message. |
@toivoh Fixed numerous typographical errors. |
On Wed, Apr 13, 2016 at 6:18 AM, H-225 notifications@github.com wrote:
Ideally, you also capture the initial value of the function's arguments, as @def function f(x, y)
requires(x < y)
ensures(result > x)
x^2 + y^2
end -erik Erik Schnetter schnetter@gmail.com |
Well, I guess there's no point in checking for contact violations if the function exits through an exception. In that case it should be fine to just find all points in the code that could cause transfer of control out of the block. That would include |
@toivoh It would certainly be good if the |
@require
is the same as@assert
but it throwsArgumentError
and generates an error message of the form"ArgumentError: $method_signature requires $condition"
.Note: this arrises from a request not to use the
@assert
macro for precondition assertions: #15409 (comment) .Update: I prefer to call this
@require
(vs@precondition
) in the tradition of DBC. I originally thought that would clash with Requires.jl, but it turns out that is not the case: #15495 (comment) .