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

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

Open
silicon-salamander opened this issue Jun 28, 2024 · 6 comments

Comments

@silicon-salamander
Copy link

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

andychu commented Jun 28, 2024

Hmmm I just reproduced this ...

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

@andychu
Copy link
Contributor

andychu commented Jun 28, 2024

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

@silicon-salamander
Copy link
Author

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

andychu commented Jun 28, 2024

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 pushed a commit that referenced this issue Jun 29, 2024
This is #2012

I'm not sure if can prevent this, but we want a better idiom.  Something
like:

    read --line-raw
    read --line-unbuffered
    read --line-unbuf
    read --line-u
andychu pushed a commit that referenced this issue Jun 29, 2024
And --with-eol

This addresses the dynamic scope IFS issue in #2012.

Documented as the recommended idiom.

TODO: we also want buffered reading of lines:

    for line in <> {
      echo $line
    }

    for i, line in <> {
      echo "$i $line"
    }
@andychu
Copy link
Contributor

andychu commented Jul 3, 2024

I implemented

  • read --raw-line for unbuffered read
  • for line in <> { echo $line } for buffered read

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

Documented here

9911231

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 andychu changed the title YSH procs can't read from stdin pitfall - IFS= read x doesn't set x (read --raw-line or for <> loop instead) Jul 3, 2024
@andychu andychu removed the bug label Jul 3, 2024
@andychu
Copy link
Contributor

andychu commented Jul 3, 2024

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants