oilshell / oil

Oils is our upgrade path from bash to a better language and runtime. It's also for Python and JavaScript users who avoid shell!
http://www.oilshell.org/
Other
2.78k stars 150 forks source link

pitfall - IFS= read x doesn't set x (read --raw-line or for <> loop instead) #2012

Open SylvAbdomen opened 4 days ago

SylvAbdomen commented 4 days ago

I am running release build 0.22.0 of oils-for-unix on Arch Linux. If I run this code with ysh, I only get blank lines printed to the screen. It should pass the lines that the while loop prints to stdout.

proc p {
    var line = ""
    while IFS= read -r line {
        write $line
    }
}

var counter = 0
while true {
    write $counter
    setvar counter += 1
    sleep 1s
} | p

If instead I make the proc a bash style function I get the expected output.

function f {
    var line = ""
    while IFS= read -r line {
        write $line
    }
}

var counter = 0
while true {
    write $counter
    setvar counter += 1
    sleep 1s
} | f

# Output:
# 0
# 1
# 2
# 3
# etc...

Am I reading from stdin correctly in the proc or is this a bug?

andychu commented 4 days ago

Hmmm I just reproduced this ...

Let me look into this, thanks for the clear report!

andychu commented 4 days ago

Hm funny that it works in proc without IFS= (though removing IFS= does change the behavior, even for one read arg!)


But I think there's still a bug here. I think it is probably related to lack of dynamic scope in procs

Our replacement for dynamic scope is shvar IFS= { read -r line }, but that is actually problematic in loops, since it's a command

I'll look into it a little more ... hm

SylvAbdomen commented 4 days ago

Interesting! Thanks for looking in to this. I didn't know about shvar. For now I can work around the issue with a loop like this.

proc p {
    var line = ""
    while true {
        try { shvar IFS= { read -r line } }
        if (_status !== 0) {
            break
        }
        write $line
    }
}

I just recently started learning YSH, and it's been great so far. Love the project!

andychu commented 4 days ago

OK wow, now I see the problem...

It's because

  1. IFS= anycommand creates a new stack frame in shell. That's how "temp bindings" are implemented in all shells

  2. In OSH shell functions, read -r line uses dynamic scope. This means it often creates a global variable, unless you declare with local or var inside the function

$ f() { read -r zz; echo "inside f: $zz"; }; f <<< 'stdin'; echo "global: $zz"
inside f: stdin
global: stdin

Actually I was confused about this for a long time ... I thought that read -r line would logically modify the calling stack frame! But it doesn't -- it looks up the whole stack, and if nothing is there, it creates a global

  1. YSH proc don't have dynamic scope. We got rid of that because it's unfamiliar to say Python and JS programmers

So basically YSH creates the line variable in the temp binding, and it gets immediately thrown away when read returns !!


So bottom line:

I will add

And then let me think about what we can do about this IFS= read pitfall

It will also affect mapfile and any other builtins which set variables


Hm hm

andychu commented 6 hours ago

I implemented

So you don't have to use the old shell idioms

Documented here

https://github.com/oilshell/oil/commit/9911231b0cb4dd95b95367ee399a4cdfc167f47f

I guess I will turn this bug into "do something about IFS= read -r` in YSH ... that is confusing

Maybe we need a lint rule or something

andychu commented 6 hours ago

Related to