Draft patch to make `(lambda x [{: a}] a) (x)` warn that we needed a
table before checking if a is nil or not.
The error message format should be discussed.
Given the function:
(λ f [{: a :b [x y z]}]
(+ a x y z))
Ex previous code:
local function f(_1_)
local _arg_2_ = _1_
local a = _arg_2_["a"] -- <= Lua error, cant destructure non-table
value!
local _arg_3_ = _arg_2_["b"]
local x = _arg_3_[1]
local y = _arg_3_[2]
local z = _arg_3_[3]
_G.assert((nil ~= z), "Missing argument z on hotpot-compile-
string:1")
_G.assert((nil ~= y), "Missing argument y on hotpot-compile-
string:1")
_G.assert((nil ~= x), "Missing argument x on hotpot-compile-
string:1")
_G.assert((nil ~= a), "Missing argument a on hotpot-compile-
string:1")
return (a + x + y + z)
end
Ex new code:
local function f(_1_)
_G.assert(("table" == type(_1_)), "Argument 1.1 ({:a a :b [x y
z]}) must be a table on des-lam.fnl:1") -- <= check type for nicer error
local _local_3_ = _1_
local a = _local_3_["a"]
local _2_ = _local_3_["b"]
_G.assert((nil ~= a), "Missing argument a on des-lam.fnl:1")
_G.assert(("table" == type(_2_)), "Argument 1.2 ([x y z]) must be
a table on des-lam.fnl:1")
local _local_4_ = _2_
local x = _local_4_[1]
local y = _local_4_[2]
local z = _local_4_[3]
_G.assert((nil ~= x), "Missing argument x on des-lam.fnl:1")
_G.assert((nil ~= y), "Missing argument y on des-lam.fnl:1")
_G.assert((nil ~= z), "Missing argument z on des-lam.fnl:1")
return (a + x + y + z)
end
Oliver Marriott (1):
Add table type checking to lambda argument destructuring
src/fennel/macros.fnl | 73 ++++++++++++++++++++++++++++++++++---------
test/core.fnl | 20 ++++++++++++
2 files changed, 79 insertions(+), 14 deletions(-)
--
2.38.5
fennel/patches/.build.yml: SUCCESS in 1m3s [Add table type checking to lambda argument destructuring][0] from [~rktjmp][1] [0]: https://lists.sr.ht/~technomancy/fennel/patches/43719 [1]: mailto:signup@omarriott.com ✓ #1043145 SUCCESS fennel/patches/.build.yml https://builds.sr.ht/~technomancy/job/1043145
~rktjmp <rktjmp@git.sr.ht> writes:
I think this is an ok-ish benchmark? It generates a random argument list with a mix of plain symbols and nested tables. There are two test styles.
Thanks for taking the time to write up this benchmark. I agree the performance cost is negligible now that we can see the numbers. The benchmark actually also caught a bug which had slipped in with regard to 1-arg do forms, so I pushed a fix for that: https://git.sr.ht/~technomancy/fennel/commit/2d6fb50b3ddfded29ebcf5ec7f8462c0c72a1cd2
One just calls whatever lambda is in the fennel binary and outputs nothing, for
use with hyperfine etc.
The other embeds both macros in one file and outputs some run data,
might be biased because call order is not randomised, requires
`luarocks install luv` to run for the hrtimer.
Examples run with lua 5.4.2.
Benchmark 1: https://0x0.st/HLWb.txt
Running classic vs new generated code, new is slower by a bit, as expected.
Benchmark 1: lua bench-classic.lua
Time (mean ± σ): 73.8 ms ± 2.7 ms [User: 71.2 ms, System: 18.0 ms]
Range (min … max): 69.9 ms … 86.6 ms 39 runs
Benchmark 2: lua bench-new.lua
Time (mean ± σ): 75.6 ms ± 2.1 ms [User: 74.2 ms, System: 16.7 ms]
Range (min … max): 72.0 ms … 83.5 ms 38 runs
Summary
lua bench-classic.lua ran
1.03 ± 0.05 times faster than lua bench-new.lua
Benchmark 2: https://0x0.st/HLWc.txt
Running both with some timing, generally the old code is faster, as
expected, it has
less checks. But I wouldn't say the new method is particularly slow. This is
actually one of the worst results I could get, most of the results
were closer to 0.0.. to 0.1 delta.
The new method is *faster* often enough that I think it's basically
noise (or the test
is faulty!).
Runs: 100
total_new_microsec 418.644us
total_old_microsec 377.391us
avg_new_microsec 4.18644us
avg_old_microsec 3.77391us
avg_delta 0.41253us slower
Oliver Marriott <hello@omarriott.com> writes:
The ast-source function should be used here on the second line; it knows how to get the metatable when necessary or use the original node when not.
Within the macro scope, pairs is actually utils.stablepairs, so I think it's probably fine to use that here. The order won't be random at least.
I think that notation of 2.3 is likely to be misunderstood. To me the most intuitive interpretation is that it's the third element of the second argument; I would assume the depth in that case is 2 because there are 2 numbers. The other hesitation is just that the implementation is a fair bit longer and more difficult to follow than the old one, but given the fact that it has to mutate the original arglist in order to add the checks in, maybe that's unavoidable. It is a little unfortunate because we're introducing an additional gensym local for these tables which is somewhat redundant; destructuring already binds this to a gensym before trying to pick it apart, but as a macro, lambda doesn't have access to that. From this perspective, it seems that the weird thing about this is that it's a problem with destructuring, not really a problem with lambda itself. But fn/lambda is a distinction where the programmer is saying "I am asking for better safety at the cost of additional overhead" whereas general destructuring does not have any such distinction, so it's not really possible to put this where it feels like it "belongs" if that makes sense. So this is probably fine to put it here in lambda. -Phil
Copy & paste the following snippet into your terminal to import this patchset into git:
curl -s https://lists.sr.ht/~technomancy/fennel/patches/43719/mbox | git am -3Learn more about email & git
From: Oliver Marriott <hello@omarriott.com>
Given `(λ f [{: x}] x)`, calling `f` with any non-table value
(including nil) would crash when attempting to destructure the argument
before performing nil checks. These errors would be more opaque than
expected from a lambda function.
Now we check whether the argument is a table first and provide a "must
be a table error" error of similar quality to the "missing argument"
errors, before performing any nil checks as before.
This is nice but I have a bad feeling that it more than doubles the
performance penalty of using lambda. However, I haven't done even the
slightest bit of performance analysis on the impact of using
lambda. Maybe I'm worrying over nothing.
The two messages:
runtime error: attempt to index local '_arg_2_' (a number value)
runtime error: Argument 1.1 ({:a a :b [x y z]}) must be a table on scratch.fnl:1
I think "attempt to index local (a number value)" is already *pretty*
close to a good error here; if you've written a nontrivial amount of
Fennel, you're certainly going to be familiar with it. If we could make
Lua say something like "attempt to index local '[x y z]' (a number value)"
that would maybe be perfect, but obviously that's not viable. I wonder
if we could do something close, like what about:
runtime error: attempt to index local '_x_y_z_' (a number value)
If we could improve the naming of the local variables, that would help
in other contexts too, not just lambda.
So, not explicitly saying no to this patch, but just throwing out some
thoughts.
-Phil
--- src/fennel/macros.fnl | 73 ++++++++++++++++++++++++++++++++++--------- test/core.fnl | 20 ++++++++++++ 2 files changed, 79 insertions(+), 14 deletions(-) diff --git a/src/fennel/macros.fnl b/src/fennel/macros.fnl index 9b645d1..6a63f47 100644 --- a/src/fennel/macros.fnl +++ b/src/fennel/macros.fnl @@ -328,22 +328,67 @@ nil, unless that argument's name begins with a question mark." (utils.kv-table? (. args metadata-position)))) arity-check-position (- 4 (if has-internal-name? 0 1) (if has-metadata? 0 1)) - empty-body? (< args-len arity-check-position)] - (fn check! [a] - (if (table? a) - (each [_ a (pairs a)] (check! a)) - (let [as (tostring a)] - (and (not (as:match "^?")) (not= as "&") (not= as "_") - (not= as "...") (not= as "&as"))) - (table.insert args arity-check-position - `(_G.assert (not= nil ,a) - ,(: "Missing argument %s on %s:%s" :format - (tostring a) - (or a.filename :unknown) - (or a.line "?")))))) + empty-body? (< args-len arity-check-position) + checks []] + + (fn wants-not-nil? [sym] + (let [as (tostring sym)] + (and (not (as:match "^?")) (not= as "&") (not= as "_") + (not= as "...") (not= as "&as")))) + + (fn location [x] + (let [via (if (table? x) (getmetatable x) x)] + {:name (view x) + :filename (or via.filename :unknown) + :line (or via.line :?)})) + + (fn check! [input-set input-key output-actions {: arg-n : arg-depth}] + ;; Recursively extract any destructing from the input set, replacing the + ;; hole with a stand in symbol to be type checked, then check the + ;; destructure itself for nils or further destructing. + ;; + ;; Mutates input-set in place and appends to output-actions + (let [next (. input-set input-key)] + (if (and (sym? next) (wants-not-nil? next)) + (let [{: name : filename : line} (location next)] + (table.insert output-actions + `(_G.assert (not= nil ,next) + ,(: "Missing argument %s on %s:%s" :format + name filename line)))) + (table? next) + (let [t-sym (gensym) + {: name : filename : line} (location next)] + (tset input-set input-key t-sym) + ;; REVIEW: this generates errors like + ;; (λ x [a [[[b]]]) (x 1 [[]]) + ;; Argument 2.3 ([b]) must be a table on ... + ;; where 2 is the argument number and 3 is the depth of destructing + ;; into that argument. + ;; + ;; Unsure how how useful the depth is, or how + ;; noisy outputting the expected table shape might end up being. + ;; but including the arg-n probably *is* useful, and once you do + ;; that and have some deeper destructing, maybe it is worth + ;; including the depth. + ;; + ;; Probably the depth should be omitted when depth = 1 + (table.insert output-actions `(_G.assert (= :table (type ,t-sym)) + ,(: "Argument %s.%s (%s) must be a table on %s:%s" :format + arg-n arg-depth name filename line))) + (table.insert output-actions `(local ,next ,t-sym)) + ;; REVIEW: I assume you *could* just pairs this but ensuring the + ;; order matches as given shape is probably broadly useful. + (if (sequence? next) + (each [i a (ipairs next)] + (check! next i output-actions {: arg-n :arg-depth (+ 1 arg-depth)})) + (each [k v (pairs next)] + (check! next k output-actions {: arg-n :arg-depth (+ 1 arg-depth)}))))))) (assert (= :table (type arglist)) "expected arg list") - (each [_ a (ipairs arglist)] (check! a)) + (each [i a (ipairs arglist)] + (check! arglist i checks {:arg-n i :arg-depth 1})) + (for [i (length checks) 1 -1] + (table.insert args arity-check-position (. checks i))) (if empty-body? (table.insert args (sym :nil))) `(fn ,(unpack args)))) diff --git a/test/core.fnl b/test/core.fnl index a54bdb7..68dc1b4 100644 --- a/test/core.fnl +++ b/test/core.fnl @@ -73,6 +73,9 @@ (== ((lambda [x ...] (+ x 2)) 4) 6) (== ((lambda [x _ y] (+ x y)) 4 5 6) 10) (== ((lambda [x] (+ x 2)) 4) 6) + (== ((lambda [?x] (+ (or ?x 1) 2))) 3) + (== ((lambda [x [y]] (+ x y)) 1 [2]) 3) + (== ((lambda [x {:y [z]}] (+ x z)) 1 {:y [2]}) 3) (== (if (= nil ((fn [a]) 1)) :pass :fail) "pass") (== (if (= nil ((lambda [a]) 1)) :lambda-works) "lambda-works") @@ -80,6 +83,23 @@ (== (let [(ok e) (pcall (lambda [x] (+ x 2)))] (string.match e "Missing argument x")) "Missing argument x") + (== (let [(ok e) (pcall (lambda [x [y]] (+ x y)) 1)] + (string.match e "Argument .+ must be a table")) + "Argument 2.1 ([y]) must be a table") + (== (let [(ok e) (pcall (lambda [x [[y]]] (+ x y)) 1 [])] + (string.match e "Argument .+ must be a table")) + "Argument 2.2 ([y]) must be a table") + (== (let [(ok e) (pcall (lambda [x {: y}] (+ x y)) 1)] + (string.match e "Argument .+ must be a table")) + "Argument 2.1 ({:y y}) must be a table") + (== (let [(ok e) (pcall (lambda [x {:y {: z}}] (+ x z)) 1 {})] + (string.match e "Argument .+ must be a table")) + "Argument 2.2 ({:z z}) must be a table") + (== (let [(ok e) (pcall (lambda [x [y [{:z {: p}}]]] (+ x y p)) + 1 [2 [{:z :whoops}]])] + (string.match e "Argument .+ must be a table")) + "Argument 2.4 ({:p p}) must be a table") + (== (let [(ok val) (pcall (λ [?x] (+ (or ?x 1) 8)))] (and ok val)) 9) (== (let [add (fn [x y z] (+ x y z)) f2 (partial add 1 2)] (f2 6)) 9) (== (let [add (fn [x y] (+ x y)) add2 (partial add)] (add2 99 2)) 101) -- 2.38.5
builds.sr.ht <builds@sr.ht>fennel/patches/.build.yml: SUCCESS in 1m3s [Add table type checking to lambda argument destructuring][0] from [~rktjmp][1] [0]: https://lists.sr.ht/~technomancy/fennel/patches/43719 [1]: mailto:signup@omarriott.com ✓ #1043145 SUCCESS fennel/patches/.build.yml https://builds.sr.ht/~technomancy/job/1043145
~rktjmp <rktjmp@git.sr.ht> writes: