[YSH] setglobal d.key mutates local instead of global

[bar-g]

Initial results of the tests:

false:	 setglobal can set shadowed global,	empty		global
false:	 setglobal can set shadowed global,	populated	global
true:	 setglobal can set un-shadowed global,	empty		global

Tests:

proc set-shadowed-global {

	var global_var = {}
	
	setglobal global_var['setglobal'] = 'done'
}




setvar global_var = {}

set-shadowed-global
... echo
		"$[ global_var ===  { 'setglobal': 'done' } ]:	"
		'setglobal can set shadowed global,	empty		global'
;


setvar global_var = { 'key': 'value' }

set-shadowed-global
... echo
		"$[ global_var ===  { 'key': 'value', 'setglobal': 'done' } ]:	"
		'setglobal can set shadowed global,	populated	global'
;



proc set-unshadowed-global {
	
	setglobal global_var['setglobal'] = 'done'
}


setvar global_var = {}

set-unshadowed-global
... echo
		"$[ global_var ===  { 'setglobal': 'done' } ]:	"
		'setglobal can set un-shadowed global,	empty		global'
;

[bar-g]

[Results moved into description.]

[andychu]

What tests do you think are undesired behavior here?

The first piece of code looks wrong

proc set-shadowed-global {

	var global_var = {}
	
	setglobal global_var['setglobal'] = 'done'
}

Because global_var is a local.

The rule should be simple: inside a proc, use setvar for locals, and setglobal for globals.

This code seems to be violating that simple rule

[bar-g]

The code is using the setglobal keyword to set the global.

However, it (wrongly) does not do so if there is a local variable defined that happens to have the same name (global is shadowed), instead of just (correctly) setting the global.

I think the fact that the example does not produces a failure even means that setglobal is actually (badly, wrong) setting the local variable instead.

[bar-g]

A setglobal could fail if there is a local but no global of the specified name.
It could likewise make sense to check that setvar fails if there is no local defined but a global of the same name.

[andychu]

OK thank you, that's a pretty serious oversight

I added a failing test case here - 7cd9edb

And I can see in the code where it's ignoring scope_e.Global

---

This bug report was hard to understand. I think it will help to use this format:

1. Subject: summary of what's wrong. "Failing Tests for setglobal" doesn't describe the issue -- "setglobal d.key mutates local instead of global" does
2. Example code - you had this, though it's long, and ideally would be reduced to something like my spec test
3. The output - you had this
4. What you expected to happen -- this is missing

---

The output of the tests looks like this:

$ test/spec.sh ysh-scope -r 10 -v
        oils_failures_allowed: 1

ysh-scope.test.sh
case    line    osh
 10     328     FAIL    setglobal d[key] inside proc should mutate global (bug #1841)
case: 10

[osh stdout]
Expected 'BEFORE mutate\n(Dict)   {}\nhi from mutate\n(Dict)   {"key":"mutated","key2":"mutated"}\nAFTER mutate\n(Dict)   {"key":"mutated","key2":"mutated"}\n'
Got      'BEFORE mutate\n(Dict)   {}\nhi from mutate\n(Dict)   {"key":"mutated","key2":"mutated"}\nAFTER mutate\n(Dict)   {}\n'

osh stdout:
BEFORE mutate
(Dict)   {}
hi from mutate
(Dict)   {"key":"mutated","key2":"mutated"}
AFTER mutate
(Dict)   {}

You're definitely welcome to add such tests

---

The other 2 issues you mention aren't clear either, though I suspect the core issue is that we don't do any dynamic checks differently at the top level vs. inside a proc. There are different static checks though

The doc for this is

• https://www.oilshell.org/release/0.20.0/doc/variables.html

But I haven't touched it in a couple years, so there are probably some things missing

Luckily this setglobal issue is more of "missing code" rather than "wrong code"

[bar-g]

I'll try to show how I attempted to make this issue and the other short and telling.

Subject: summary of what's wrong. "Failing Tests for setglobal" doesn't describe the issue -- "setglobal d.key mutates local instead of global" does

Hm, I first hadn't grasped yet that set global it's actually setting a local. I just found that it's not setting the global in my code (but don't just want to say "it doesn't work"). I mostly don't know what is actually wrong at first, and it could be multiple and different things that cause the more complex application test to fail.

Example code - you had this, though it's long, and ideally would be reduced to something like my spec test

Hm, I tried YSH's ... feature to make one-liner tests that produce telling output, but now I see that additionally making use of its line commenting feature could be a good idea.

Here is what it's telling:

false:	 setglobal can set shadowed global,	empty		global
^        ^                                      ^
|        |                                      |
|        |                                      * Data/condition/context (after the `,`).
|        * Assumption that is expected.
* Tests are expected to pass (assumption true).

[andychu]

Released - https://www.oilshell.org/blog/2024/06/release-0.22.0.html