FIX: Correctly handle the case in Proc args binding when both typed positional varargs and block arg are present #1894

glyh · 2024-04-04T17:08:16Z

Continuation of #1893. Fixes #1892, maybe fixes #1849 .

…ositional varargs and block arg are present

andychu · 2024-04-04T17:29:11Z

OK cool, the tests ran!

http://travis-ci.oilshell.org/github-jobs/6593/

There are some failures, let me look at it a bit later

I can help interpret

Thanks for the PR!

glyh · 2024-04-04T19:44:55Z

I added a new spec test in spec/ysh-proc.test.sh but I'm not sure how should I inspect the information from a block.

andychu

Thanks for looking into this ! This looks very promising

I need to clone the branch and look at the before/after a little, but here are a couple comments

andychu · 2024-04-05T05:56:28Z

spec/ysh-proc.test.sh

+  for i, k, v in (c) {
+    echo "$i - $k - $v"
+  }
+  # TODO : figure out how to inspect blocks


We have pp line (x), which is meant to print value x in a deterministic way that won't change, specifically for spec tests (unlike the = operator)

$ pp line (^(echo hi)) (Command) <Command 0x7f39428c1870>

Although maybe this address 0x7 ... causes problems? That isn't stable

I guess you could use sed to remove it for now, hmm

andychu · 2024-04-05T06:01:42Z

ysh/func_proc.py

@@ -369,15 +351,31 @@ def _BindTyped(
        if rest:
            lval = LeftName(rest.name, rest.blame_tok)

-            rest_val = value.List(pos_args[num_params:])
-            mem.SetLocalName(lval, rest_val)
+            rest_val = pos_args[num_params:-1] if block_param else pos_args[num_params:]


Hmm why move if block_param after the rest section?

Wouldn't keeping it before that simplify this condition?

andychu · 2024-04-05T06:02:05Z

ysh/func_proc.py

        else:
-            if num_args > num_params:
+            if num_args > num_params + (1 if block_param else 0):


Likewise can we remove if block_param by handling it first?

I might have missed something

andychu · 2024-04-05T06:16:41Z

OK I played with it a bit more ... I do see the problem, and I see the logic behind your fix - thank you

I wonder if there are any other problems here, let me see

I guess I'm thinking about the larger issue

does it even make sense to treat the block as a positional arg? It's a weird quirk
does it cause any other problems?

I think my logic was:

var myblock = ^(echo hi)
cd (myblock)

looks nicer than

var myblock = ^(echo hi)
cd ( ; ; myblock)

But isn't that just cosmetic?

Why shouldn't it be

cd ( ; ; myblock )

? Is that more consistent and easier to remember for users?

andychu · 2024-04-05T06:19:50Z

Also I'm thinking about our docs:

https://www.oilshell.org/release/0.21.0/doc/proc-func.html

(The block param is passed as a typed, positional arg, although this detail usually doesn't matter.)

Clearly I did not think this through!

I think the design is a little weird

andychu · 2024-04-05T06:22:24Z

I think this came about in unifying

cd /tmp { echo hi }

cd /tmp (myblock)

But yeah now I wonder if it should be

cd /tmp ( ; ; myblock)

for consistency. I don't think this occurs much in real code. It's only for testing and metaprogramming and so forth.

There are also builtins like fopen and shvar ... hm

Another option is to change the signature ??? We could try to make it so there are 3 sections, not 4

proc p (w1, w2; t1, t2; named1, named2) {
}

Hm

glyh · 2024-04-05T10:58:29Z

It does still make sense, just that logically the block is the last passed typed arg rather than the first. That's why I need to switch the binding order. Block & typed positional args are still unified and code like this works:

proc doIt(;...args) {
  if (len(args) === 1) {
    eval (args[0])
  } else {
    var a = args[0]
    echo $a
    var argsRest = args[1:]
    doIt (...argsRest)
  }
}

# all of below work. 
doIt (1, 2, 3, 4, 5) {
  echo yeah
}

doIt (1, 2, 3, 4, 5, ^(echo yeah))

doIt {
  echo yeah
}

glyh · 2024-04-05T11:12:46Z

I don't really understand the cd /tmp ( ; ; myblock) syntax. I thought ; only occurs in definition sites.

andychu · 2024-04-06T01:38:31Z

So ; does have to be used at call sites in one case now:

call f(...args ; ...kwargs)

which is like Python's

f(*args, **kwargs)

We only have a single operator ..., so we have to use ; to disambiguate.

I think the argument for cd (; ; myblock) is to make a fully general wrapper

How do you wrap any proc?

Suppose you have:

proc p(w1, w2; t1, t2; n1, n2; block) {
}

How do you wrap it, like Python's f(*args, **kwargs)

I think you need

proc wrapper(...words; ...typed; ...named; block) {
  p @words (...typed ; ...named ; block)
}

This is more consistent than:

proc wrapper(...words; ...typed; ...named; block) {
  p @words (...typed ++ [ block ]; ...named)
}

@bar-g ran into this bug, and may have opinions

He wanted p @ALL_ARGS, but I think it's not desirable to unify them

I think p @words (...typed ; ...named ; block) makes sense

I think it's nice because the signature matches the call site

Thoughts?

andychu · 2024-04-06T01:39:10Z

I am not sure of the exact fallout of changing it, but I would like a PR that does cd /tmp ( ; ; block) to compare the two

Thanks for doing this -- it's very helpful in clarifying our design

glyh · 2024-04-06T02:21:28Z

I think it makes sense to have different treatment for different cases

# The followings are equivalent
p (...pos ++ [blk])
p (...pos ++ [blk];;)
p (...pos;) {
  # inside the blk
}
# The followings are equivalent
p (...pos; ...named; blk)
p (...(pos ++ [blk]); ...named)
p (...pos; ...named){
  # inside the blk
}

This PR doesn't conflict with those so it's no harm to merge them.

bar-g · 2024-04-08T10:57:38Z

sorry, only found this now, will take a day or two to answer

andychu · 2024-04-08T16:04:29Z

I am going to write something about this. I think we want to make call site and def site consistent

and we have two options in that case

proc p (words; typed; named; block) { ... }

cd ( ; ; block)

OR


proc p (words; typed; named) {  ...  }  # No Block section !!

cd (block)  # FIRST typed arg

Either of these consistent. But neither the OLD behavior or this NEW behavior is consistent. We still have a problem, even with the PR

I believe that an argument for the first style is ... should work APART from blocks

cd (...typed) {
  echo hi
}

cd (...typed ; ; ^(echo hi) )

should work consistently

It shouldn't be

cd (... [ typed ++ [^(echo hi)] ])

in other words

I think that works, but it's kinda crazy !!

andychu · 2024-04-08T16:05:42Z

Yeah I think that is the core argument

cd (... [ typed ++ [^(echo hi)] ])

is way too complex. We are appending one element to a list, and then splatting it

cd (...typed ; ; ^(echo hi) )

is more understandable

It's equivalent to

cd (...typed) {
  echo hi
}

(nevermind that cd doesn't take typed args now, it's just an example)

bar-g · 2024-04-12T09:18:30Z

Sorry, different from said, I will only be able to look at this next week.

andychu · 2024-04-13T02:36:22Z

Hm I played around with the other option -- p @words (...typed; ...named; block) and it appears to work fine

610e5e6

We still have eval (b) rather than eval (; ;b) though, because there's a difference between a value.Command that's positional (first section), and a block argument (fourth section)

So right now I think this works -- the main reason it would change is because of one of our 16-17 use cases from the blog/Zulip. We are trying to encode many idioms in this reflective interface

FIX: Correctly handle the case in Proc args binding when both typed p…

96501d0

…ositional varargs and block arg are present

glyh added 4 commits April 5, 2024 03:05

take block into account when throwing error message

8349658

add new spec test for varargs and block

80c1380

Make procs available when testing procs

8da55bc

we should use --upgrade when using block syntax

c42b453

andychu reviewed Apr 5, 2024

View reviewed changes

add more spec test for typed positional args and block

21e0341

remove redundant comments

cd6972c

glyh self-assigned this Apr 6, 2024

glyh added the bugfix Bug fix label Apr 8, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FIX: Correctly handle the case in Proc args binding when both typed positional varargs and block arg are present #1894

FIX: Correctly handle the case in Proc args binding when both typed positional varargs and block arg are present #1894

glyh commented Apr 4, 2024 •

edited

andychu commented Apr 4, 2024 •

edited

glyh commented Apr 4, 2024 •

edited

andychu left a comment

andychu Apr 5, 2024 •

edited

andychu Apr 5, 2024

andychu Apr 5, 2024

andychu commented Apr 5, 2024 •

edited

andychu commented Apr 5, 2024

andychu commented Apr 5, 2024

glyh commented Apr 5, 2024 •

edited

glyh commented Apr 5, 2024

andychu commented Apr 6, 2024

andychu commented Apr 6, 2024

glyh commented Apr 6, 2024 •

edited

bar-g commented Apr 8, 2024

andychu commented Apr 8, 2024

andychu commented Apr 8, 2024 •

edited

bar-g commented Apr 12, 2024

andychu commented Apr 13, 2024

FIX: Correctly handle the case in Proc args binding when both typed positional varargs and block arg are present #1894

Are you sure you want to change the base?

FIX: Correctly handle the case in Proc args binding when both typed positional varargs and block arg are present #1894

Conversation

glyh commented Apr 4, 2024 • edited

andychu commented Apr 4, 2024 • edited

glyh commented Apr 4, 2024 • edited

andychu left a comment

Choose a reason for hiding this comment

andychu Apr 5, 2024 • edited

Choose a reason for hiding this comment

andychu Apr 5, 2024

Choose a reason for hiding this comment

andychu Apr 5, 2024

Choose a reason for hiding this comment

andychu commented Apr 5, 2024 • edited

andychu commented Apr 5, 2024

andychu commented Apr 5, 2024

glyh commented Apr 5, 2024 • edited

glyh commented Apr 5, 2024

andychu commented Apr 6, 2024

How do you wrap any proc?

andychu commented Apr 6, 2024

glyh commented Apr 6, 2024 • edited

bar-g commented Apr 8, 2024

andychu commented Apr 8, 2024

andychu commented Apr 8, 2024 • edited

bar-g commented Apr 12, 2024

andychu commented Apr 13, 2024

glyh commented Apr 4, 2024 •

edited

andychu commented Apr 4, 2024 •

edited

glyh commented Apr 4, 2024 •

edited

andychu Apr 5, 2024 •

edited

andychu commented Apr 5, 2024 •

edited

glyh commented Apr 5, 2024 •

edited

glyh commented Apr 6, 2024 •

edited

andychu commented Apr 8, 2024 •

edited