janet-lang / janet

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

`each` macro improper behavior? #1400

Closed llmII closed 7 months ago

llmII commented 7 months ago

To illustrate the issue:

(defn lines-from-buf [buf]
  (when-let [idx (string/find "\n" buf)]
    (yield (buffer/slice buf 0 idx))
    (buffer/blit buf buf 0 (+ 1 idx))
    (buffer/popn buf (+ 1 idx))
    (lines-from-buf buf)))
(each line (coro (lines-from-buf @"a\nb\nc\nd\n\ne")) (print line))
(each line (coro (lines-from-buf @"abc\ndef\ngeh\nd\n\ne")) (print line))

(defn lines-from-buf [buf &opt res]
  (default res @"")
  (when-let [idx (string/find "\n" buf)]
    (yield (-> res
               (buffer/clear)
               (buffer/blit buf 0 0 idx)))
    (pp (buffer "RES " res " BUF " buf))
    (buffer/blit buf buf 0 (+ 1 idx))
    (buffer/popn buf (+ 1 idx))
    (lines-from-buf buf res)))
(each line (coro (lines-from-buf @"a\nb\nc\nd\n\ne")) (print line))
(each line (coro (lines-from-buf @"abc\ndef\ngeh\nd\n\ne")) (print line))

The expected output, which is yielded by the first lines-from-buf is as follows:

repl:7:> (each line (coro (lines-from-buf @"a\nb\nc\nd\n\ne")) (print line))
a
b
c
d

nil
repl:8:> (each line (coro (lines-from-buf @"abc\ndef\ngeh\nd\n\ne")) (print line))
abc
def
geh
d

nil

The second function yields and sets res correctly (looking at the output from pp) but each never sees the first yielded value seemingly as shown below:

repl:20:> (each line (coro (lines-from-buf @"a\nb\nc\nd\n\ne")) (print line))
@"RES a BUF a\nb\nc\nd\n\ne"
b
@"RES b BUF b\nc\nd\n\ne"
c
@"RES c BUF c\nd\n\ne"
d
@"RES d BUF d\n\ne"

@"RES  BUF \ne"

nil
repl:21:> (each line (coro (lines-from-buf @"abc\ndef\ngeh\nd\n\ne")) (print line))
@"RES abc BUF abc\ndef\ngeh\nd\n\ne"
def
@"RES def BUF def\ngeh\nd\n\ne"
geh
@"RES geh BUF geh\nd\n\ne"
d
@"RES d BUF d\n\ne"

@"RES  BUF \ne"

nil

I think the macro expands to the incorrect form as illustrated with:

(macex '(each line (coro (lines-from-buf @"abc\ndef\ngeh\nd\n\ne")) (print line)))

# output below but <function *> has been rewrote to just the *
(do
  (def _000072 (fiber/new (fn [] (lines-from-buf @"abc\ndef\ngeh\nd\n\ne")) :yi))
  (var _000071 (next _000072 nil))
  (while (not= nil _000071)
    (def line (in _000072 _000071))
    (set _000071 (next _000072 _000071))
    (print line)))

Why is print line executed after the set instead of before? Invoking next on a fiber could (and does as shown here) modify what is returned when what is returned is a reference type consumed throughout the entirety of the fiber's lifecycle.

I would think putting the user provided form (in this case (print line)) before the set would do better in all cases.

pepe commented 7 months ago

Have you tried to flush stdout after pp?

llmII commented 7 months ago

@pepe Just in case, I did just now try that. There is no difference if one calls (:flush stdout) directly after pp in the second implementation.

pepe commented 7 months ago

Thanks. TBH, I just woke up and saw the combination of print and order :-), so my suggestion went.

sogaiu commented 7 months ago

FWIW, make test doesn't show errors for:

diff --git a/src/boot/boot.janet b/src/boot/boot.janet
index 46fcabae..e3b5ee9e 100644
--- a/src/boot/boot.janet
+++ b/src/boot/boot.janet
@@ -442,8 +442,8 @@
               :each ~(,in ,ds ,k)
               :keys k
               :pairs ~[,k (,in ,ds ,k)]))
-         (set ,k (,next ,ds ,k))
-         ,;body))))
+         ,;body
+         (set ,k (,next ,ds ,k))))))

 (defn- iterate-template
   [binding expr body]
llmII commented 7 months ago

@sogaiu If that works with make test and works with the second example of lines-from-buf from the original post in this thread, it'd be awesome if you submitted the PR to get that change.

sogaiu commented 7 months ago

I haven't really thought much about the details here, but I've submitted https://github.com/janet-lang/janet/pull/1402 anyway.

FWIW, I did run some more tests across some of my repositories and those didn't turn up anything.

sogaiu commented 7 months ago

@llmII Perhaps this issue can be closed?

llmII commented 7 months ago

Fixed in #1402.