For-loops and variable change events

[faho]

While working on reducing the overhead of running for-loops, I stumbled upon one weird fact:

> function foo --on-variable foo; echo $argv $foo; end
> for foo in 1 2 3; end
VARIABLE SET FOO
VARIABLE SET FOO
VARIABLE SET FOO
VARIABLE SET FOO

See how that calls the event handler four times? That's because this called parser::set_var_and_fire to set the variable initially, which pins it to the outer scope.

That's probably useless.

However, I'm not sure the events here have a use in general. The reason is that for sets the variable as a local to the enclosing block, so no variable handler would ever be able to touch it?

Do we want to just remove the superfluous handler or the event firing here in general?

(or do we even want to abolish events for local variables entirely?)

Is there a use here I'm not seeing?

[krobelus]

Yeah I think this is a bug. I expect to get events only when a global/universal variable is changed.
The point of local variables is to keep everything about a variable private. Callers should never know that the local foo existed.

[floam]

I would expect local variables not to post these events.

[faho]

Turning off variable handlers for local variables is a possibility. Do we have any idea if someone's trying to use that?

The only idea I can come up with is to have a thing run simply more often.

[floam]

Actually I'm not sure if I was right to suggest local variables should not fire events. There are probably valid use cases where a global variable has an on-variable-change handler, and a person would expect it to still work even if they make a local copy for the sake of not messing things up, are about to export it or something, etc.

[faho]

a person would expect it to still work even if they make a local copy for the sake of not messing things up, are about to export it or something, etc.

Okay, please look at one of our fine, artisinal code samples:

> function foo --on-variable foo; echo $argv Value of foo: $foo; end
> set -g foo global
VARIABLE SET foo Value of foo: global
> set -l foo local1
VARIABLE SET foo Value of foo: global
> set foo local2
VARIABLE SET foo Value of foo: global
> set foo local3
VARIABLE SET foo Value of foo: global
> set foo local4
VARIABLE SET foo Value of foo: global
# ...

As you see, the variable handler is triggered, but there's no way to tell that anything happened. As far as it is concerned, what happened was set -g foo global, again and again and again. set --show foo will only show it as global.

So there is no way to react to the actual value of the variable, so I do not see what use the variable handler has.

Even if you e.g. said that it should show that the variable was shadowed, there is

1. no way to find out what the shadowing value is
2. no way to act to influence it
3. no way to tell what the difference between the first, second and third shadowing call is - it is repeated again and again

So, what I would like here is a concrete example of how you would use this for anything.

[floam]

function foo_watcher --no-scope-shadowing --on-variable foo;
    echo "$argv="
    set --show foo
end

> set -g foo bar
VARIABLE SET foo=
$foo: set in global scope, unexported, with 1 elements
$foo[1]: |bar|=

> set -l foo MOOCOW
VARIABLE SET foo=
$foo: set in local scope, unexported, with 1 elements
$foo[1]: |MOOCOW|
$foo: set in global scope, unexported, with 1 elements
$foo[1]: |bar|

> set foo jar
VARIABLE SET foo=
$foo: set in local scope, unexported, with 1 elements
$foo[1]: |jar|
$foo: set in global scope, unexported, with 1 elements
$foo[1]: |bar|
$foo: set in univ

[faho]

That's --no-scope-shadowing, which is cheating by definition, and I don't believe something we have to bend over backwards to support.

[floam]

which is cheating by definition, and I don't believe something we have to bend over backwards to support.

It works right now, doesn't it?

[faho]

Let me put it this way:

If we break that, and make everything else better, that's a good deal. "--no-scope-shadowing" is a hack made to support something like psub, but without actually implementing the needed things, or something. (edit: first mention I can find is 88efc73, implemented for .. Instead of, you know, just implementing it as a builtin in one literal line of c++)

My question remains: What do you need this for? Why would you need this handler? What's the use case? A concrete example, not an abstract "look ma, no handlebars" please.

Because if there is no real use, we can simply document "variable handlers are only triggered for global and universal variables" and be done. If there's no real need, there's no real breakage, so it's all fine.

[floam]

I'm not really sure how no-scope-shadowing is "cheating" here. If scope shadowing is on that would make what you were asking how to do not possible. Because of the scope shadowing. I think turning off scope-shadowing is exactly the way to do it.

[faho]

Okay, let's not get bogged down into "is it cheating or not".

There are two questions here:

1. Is the first weird trigger for for-loops useful? The answer is very very very likely "heck no".
2. Is the handler for local variables useful at all?

The second question here asks "useful". It's not about if you can theoretically finagle something together with "--no-scope-shadowing" and a prayer, it's about actual use. I have no problem simpy adding a note here that variable handlers only work for global/universal variables if there's no actual use in doing it for locals, especially because that requires --no-scope-shadowing, so it looks broken.

[floam]

Okay, so a realistic example could be __fish_reconstruct_path - it's not a crazy idea that someone would have wanted to design it such that if the user does set -l fish_user_paths /tmp/bin, /tmp/bin is appended to a locally exported PATH by the handler rather than -gx PATH. That way, when they are out of scope they'd go back to their previous $PATH.

There are a lot of things a person might do with a monitored variable in any scope.

[floam]

(edit: first mention I can find is 88efc73, implemented for .. Instead of, you know, just implementing it as a builtin in one literal line of c++)

It's much older than that. (The language has never had actual closures.)

ee94424

[faho]

it's not a crazy idea that someone would have wanted to design it such that if the user does set -l fish_user_paths /tmp/bin, /tmp/bin is appended to a locally exported PATH by the handler rather than -gx PATH.

Yeah, that makes sense.

---

Okay, think I'm gonna remove the duplicate event handler for for-loops and call it a day.

[krobelus]

Okay, so a realistic example could be __fish_reconstruct_path

I don't think that example works.
If you do something like this, then __fish_reconstruct_path will only ever see the global value, even when the local value changed:

set -g user_paths /global/bin

function __fish_reconstruct_path --on-variable user_paths
    echo adding $user_paths to \$PATH
    # ...
end

begin
    set -l user_paths /local/bin
    # run some command assuming that /local/bin is in $PATH - but it's not!
end

The problem is that __fish_reconstruct_path only receives the global value.
Which makes sense. It's a global function, so it can only read global state.

So I don't think events on locals are useful.
The only way I can imagine them being useful if we can tie an --on-variable function to a function in a specific scope.

[faho]

If you do something like this, then __fish_reconstruct_path will only ever see the global value, even when the local value changed:

Not if you use --no-scope-shadowing on the event handler.

We currently don't, but it is something at least something semi-reasonable you might want to do.

[krobelus]

My bad, I missed that. Ok then it's probably reasonable to keep, time will tell if it's useful in practise.

Removing the event for for's implicit initialization sounds fine, the user needn't know about this.

[faho]

Pretty sure the for-loop thing existed since #4376, i.e. fish 3.0.