janet-lang / janet

A dynamic language and bytecode vm
https://janet-lang.org
MIT License
3.52k stars 227 forks source link

Variadic argument is not preserved in nested short-fn #1123

Closed primo-ppcg closed 1 year ago

primo-ppcg commented 1 year ago

When short-fns are nested, the variadic argument to the outer function is not preserved.

A minimal example that demonstrates the issue:

( |( |(pp $) $&) 1 2)

Expected: (1 2) Actual: (2)

It appears as though the first value is somehow dropped.

The following equivalent forms both return the correct result:

( |( |(pp $) [$0 $1]) 1 2) # $0 and $1 both preserved
( |( (fn [i] (pp i)) $&) 1 2) # $& preserved without inner short-fn

The above is obviously a trivial example. I noticed the behavior when using a map within a reduce.

sogaiu commented 1 year ago

Is this discussion related?

primo-ppcg commented 1 year ago

Is this discussion related?

A slightly different case. They were attempting to use a positional argument from the outer function within the inner, which can't work for reasons pointed out in the accepted answer.

The Janet compiler handles nested short-fns fine, with exception to the variadic argument, that somehow loses it's first member.

sogaiu commented 1 year ago

Thanks for taking a look and explaining.

Though I try not to use more than one shortfn in this type of situation (it's too confusing to me), losing the first member doesn't sound good.

bakpakin commented 1 year ago

The macro expansion for |(|(pp $) $&) is as follows:

(fn [$0 & $&] ((fn [$0] (pp $0)) $&))

The inner parameter is captured by the outer short-fn when determing the outer functions parameter list. Nothing is "lost", it is just a bit ambiguous to the macro what is actually wanted here - the short-fn macro works by just scanning through it's body and looking for "$" parameters to pull into it's function signature, so it sees the inner parameter and doesn't know that this is shadowed.

bakpakin commented 1 year ago

A similar example that is much simpler about the short-fn macro not know exactly which parameters are capture is the following:

(def f |(let [$ 10] $))

Should the function f have one parameter ($) or no parameters? It is ambiguous to me, and one might expect it to have no parameters, but that requires the macro to be cleverer, and at that point would be better built into the compiler.

primo-ppcg commented 1 year ago

(fn [$0 & $&] ((fn [$0] (pp $0)) $&))

The behavior makes perfect sense when expanded. Thanks for checking.

Assuming the inner function would be expanded first (I'm unsure if that's the case), replacing the positional arguments with generated symbols should prevent the double discovery. I also don't know if gensym has a significant overhead that would make this undesirable.

bakpakin commented 1 year ago

The gensym overhead is fine - however that fix, while reasonable, doesn't address the second example I posted. Here is such a patch:

diff --git a/src/boot/boot.janet b/src/boot/boot.janet
index c120abb0..0ccda788 100644
--- a/src/boot/boot.janet
+++ b/src/boot/boot.janet
@@ -2209,6 +2209,7 @@
   (defn saw-special-arg
     [num]
     (set max-param-seen (max max-param-seen num)))
+  (def prefix (gensym))
   (defn on-binding
     [x]
     (if (string/has-prefix? '$ x)
@@ -2216,22 +2217,24 @@
         (= '$ x)
         (do
           (saw-special-arg 0)
-          '$0)
+          (symbol prefix '$0))
         (= '$& x)
         (do
           (set vararg true)
-          x)
+          (symbol prefix x))
         :else
         (do
           (def num (scan-number (string/slice x 1)))
           (if (nat? num)
-            (saw-special-arg num))
-          x))
+            (do
+              (saw-special-arg num)
+              (symbol prefix x))
+            x)))
       x))
   (def expanded (macex arg on-binding))
   (def name-splice (if name [name] []))
-  (def fn-args (seq [i :range [0 (+ 1 max-param-seen)]] (symbol '$ i)))
-  ~(fn ,;name-splice [,;fn-args ,;(if vararg ['& '$&] [])] ,expanded))
+  (def fn-args (seq [i :range [0 (+ 1 max-param-seen)]] (symbol prefix '$ i)))
+  ~(fn ,;name-splice [,;fn-args ,;(if vararg ['& (symbol prefix '$&)] [])] ,expanded))

 ###
 ###
primo-ppcg commented 1 year ago

that fix, while reasonable, doesn't address the second example I posted

I'm not sure there's one fix that would work for both cases. However, I do think it's reasonable to allow the macro to be confused in the latter case. Using identifiers that mimic positional arguments should probably be avoided anyway, and certainly within a short-fn.