if-let false branch, bindings out of scope

[primo-ppcg]

I've found that forms such as this:

(if-let [a true b false] b a)

are invalid, as the binding of a is out of scope in the false branch. To paraphrase a community example:

(if-let [root (math/sqrt 49)
         _ (even? root)]
  (printf "%d is even" root)
  (printf "%d is odd" root))

I understand that the bindings are short-circuiting, which is good and correct, but at very least the first binding should be in scope, right?

I would expect the initial if-let above to expand to something like the following:

(if (def a true) (if (def b false) b a) a)

but instead, it expands as (gensyms renamed for brevity):

(do (def $A true) (if $A (do (def a $A) (do (def $B false) (if $B (do (def b $B) b) a))) a))

I was able to trace the current implemenation all the way back to the Janet rename on Sep 6, 2018. I suspect the implementation was necessary at the time (perhaps def didn't support destructuring?), but at present, the following appears to be sufficient:

diff --git a/src/boot/boot.janet b/src/boot/boot.janet
@@ -663,30 +663,16 @@
 (defmacro if-let
   ``Make multiple bindings, and if all are truthy,
   evaluate the `tru` form. If any are false or nil, evaluate
   the `fal` form. Bindings have the same syntax as the `let` macro.``
   [bindings tru &opt fal]
   (def len (length bindings))
   (if (= 0 len) (error "expected at least 1 binding"))
   (if (odd? len) (error "expected an even number of bindings"))
   (defn aux [i]
     (if (>= i len)
       tru
       (do
         (def bl (in bindings i))
         (def br (in bindings (+ 1 i)))
-        (def atm (idempotent? bl))
-        (def sym (if atm bl (gensym)))
-        (if atm
-          # Simple binding
-          (tuple 'do
-                 (tuple 'def sym br)
-                 (tuple 'if sym (aux (+ 2 i)) fal))
-          # Destructured binding
-          (tuple 'do
-                 (tuple 'def sym br)
-                 (tuple 'if sym
-                        (tuple 'do
-                               (tuple 'def bl sym)
-                               (aux (+ 2 i)))
-                        fal))))))
+        (tuple 'if (tuple 'def bl br) (aux (+ 2 i)) fal))))
   (aux 0))

At very least, it passes all tests, although coverage does not appear to be complete: https://github.com/janet-lang/janet/blob/master/test/suite-boot.janet#L125-L129

But as far as I have been able to determine, there is no logical difference between this and the current implementation for any sort or number of bindings, except that it resolves the title issue. It's also significantly faster (~40% for two var bindings).

As perhaps an interesting point of discussion, the following is true for the current master:

(if-let [[a b] []] :t :f)
# ->
:t

One might debate if this is correct behavior.

[CosmicToast]

I've noticed this before and ultimately settled on "it doesn't matter".

Pretend you have 50 if-let bindings in a row. The false path has no way of knowing where in the chain the failure occurred. Which means that depending on any of the bindings being available is inherently error-prone, which is typically the converse of the intention.

i.e the reasoning is to have the entire set of if-let bindings be pseudo-atomic, if any of them fail, they all can be considered to have failed, and the false branch is essentially error propagation. This is in part because you usually don't assign "true", in such bindings, but rather (operation-that-could-fail).

There could be space for a combined macro ala (let-iflet [always bindings] [if-let bindings] :t :f) which just translates into (let [always bindings] (if-let [if-let bindings] :t :f)), but yeah, I ultimately don't see much benefit out of making the symbols visible since you can never depend on any of them in a way that matters (besides littering the false path with a bunch of ifs or whens, which you could have done to begin with instead of the if-let.

[primo-ppcg]

Reasonable, and certainly not difficult to work around; let if-let as you mention is fine.

However, I do still think that the macro should be reworked, especially considering that it is the basis implementation for many others (when-with, if-with, when-let, and used in about 10 others).

I ultimately don't see much benefit out of making the symbols visible since you can never depend on any of them in a way that matters

If doing so would slow the implementation or make it overly complicated, I would absolutely agree with you. It does the opposite.

[bakpakin]

Yes, def did not used to support destructuring - that looks like a reasonable simplification to me.

[primo-ppcg]

For future reference, this issue is incompatible with #1191, and was chosen as the victim.