Skip to content
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

each macro improper behavior? #1400

Closed
llmII opened this issue Feb 11, 2024 · 8 comments
Closed

each macro improper behavior? #1400

llmII opened this issue Feb 11, 2024 · 8 comments

Comments

@llmII
Copy link
Contributor

llmII commented Feb 11, 2024

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
Copy link
Member

pepe commented Feb 12, 2024

Have you tried to flush stdout after pp?

@llmII
Copy link
Contributor Author

llmII commented Feb 12, 2024

@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
Copy link
Member

pepe commented Feb 12, 2024

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

@sogaiu
Copy link
Contributor

sogaiu commented Feb 12, 2024

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
Copy link
Contributor Author

llmII commented Feb 12, 2024

@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
Copy link
Contributor

sogaiu commented Feb 13, 2024

I haven't really thought much about the details here, but I've submitted #1402 anyway.

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

bakpakin added a commit that referenced this issue Feb 15, 2024
@sogaiu
Copy link
Contributor

sogaiu commented Feb 15, 2024

@llmII Perhaps this issue can be closed?

@llmII
Copy link
Contributor Author

llmII commented Feb 15, 2024

Fixed in #1402.

@llmII llmII closed this as completed Feb 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants