Add set --function
#8145
Add set --function
#8145
Conversation
krobelus
left a comment
There was a problem hiding this comment.
Looks good, this is really nice!
We should try to make this the default behavior eventually - during the transition period we could print a deprecation warning when a statement set foo bar inside a function sets a global or universal var.
Wait, you mean requiring an explicit Anyway, that's for another time. I want to make this kind of scoping prominent. I believe that it's the much better model from But switching that is a much much much bigger change, so I would appreciate any ideas on how to do that - |
That problem can go away once we "merge" global and universal scopes maybe |
"Does this make my function a local function?" Maybe "--function-scoped", but that's a bit asymmetrical to what's already there. Also neither option really explains what happens when there isn't a function - |
|
So, looking over #565, there's a fair amount of talk over what I don't agree with that. I want Now, this PR currently uses the global scope at the top level, and that's probably not great - now suddenly the variable is available everywhere. Instead I'm going to make it the top level local scope. That also makes explaining what this does easy:
(this could be made shorter by explaining that it's "the top-most accessible local scope" or some such, but that is a very dense explanation) Of course that means it's not entirely the same as the "undefined scope" anymore, but tbh that's not great anyway. |
|
Okay, I have now made Note that this now means functions with > function set-my-thing --no-scope-shadowing
set -f $argv
end
> function foo
set -S bar
set-my-thing bar nana
set -S bar
end
$bar: set in local scope, unexported, with 1 elements
$bar[1]: |nana|(to be clear that's the I'm not entirely sure if I like that or not. They could already read local variables (that's the point of it!), and they could always set global variables, but now they can set local variables as well. I think Compare also #3540. |
This makes the "unspecified" scope available - the one that is used
when no variable exists.
It's either the enclosing function's topmost local scope, or global
scope if there is no function.
This removes the need to declare variables locally before use. E.g.:
```fish
set -l thing
if condition
set thing one
else
set thing two
end
```
could be written as
```fish
if condition
set -f thing one
else
set -f thing two
end
```
Note: Many scripts shipped with fish use workarounds like `and`/`or`
instead of `if`, so it isn't easy to find good examples.
Also, if there isn't an else-branch in that above, just with
```fish
if condition
set -f thing one
end
```
that means something different from setting it before! Now, if
`condition` isn't true, it would use a global (or universal) variable of
te same name!
Some more interesting parts:
Because it *is* a local scope, setting a variable `-f` and
`-l` in the toplevel of a function ends up the same:
```fish
function foo2
set -l foo bar
set -f foo baz # modifies the *same* variable!
end
```
but setting it locally inside a block creates a new local variable
that shadows the function-scoped variable:
```fish
function foo3
set -f foo bar
begin
set -l foo banana
# $foo is banana
end
# $foo is bar again
end
```
This is how local variables already work. "Local" is actually "block-scoped".
Also `set --show` will only show the closest local scope, so it won't
show a shadowed function-level variable. Again, this is how local
variables already work, and could be done as a separate change.
Fixes fish-shell#565
|
Alright, I added the docs, which kinda shows how convoluted all of this is. We now have:
This should really be simplified. The classic "local" is probably mostly obsolete, unless you really need a variable only valid in a block, like for: begin
set -lx PAGER cat
# do things that use $PAGER
endSo removing it, or changing the default meaning of We also can't really make the undefined scope local instead, because millions of config.fish have cruft like set -x PAGER cat(alternative: Make exported variables global by default?) Merging universal and global scope sounds tempting, but what if you e.g. want to try out a new value for a universal variable? |
|
Yeah this is getting fairly complex. I wonder how we can provide a simpler UI with fewer scopes. My main gripe with (top-level) local scope is that local variables are not inherited by functions by default. function f
echo "not visible inside function:" $foo
end
set --function foo bar # same with set --local
fI'm not sure if not inheriting locals is really useful, especially since as of now, function authors should declare their own variables with If we could make
Right, that wouldn't really work, we can't use locals for trying out values. |
Oh I heavily disagree with this! Not passing local variables everywhere is good because it encapsulates things. It leaves knowledge of these variables where it is, instead of spreading it all over the place. Your example here is a toy. In a real situation often the function would be defined elsewhere, even in a different file. So having random variables defined there would be quite confusing. And if you do need to pass a local variable, we have an option for that: Exporting them: function f
echo "not visible inside function:" $foo
end
set --function --export foo bar # same with set --local
fThe function will receive a local copy of the variable. |
ridiculousfish
left a comment
There was a problem hiding this comment.
This looks excellent to me. I agree with having top-level set -f set in the top-most local scope, not a global. I think this actually simplifies scoping, since users can now mostly ignore set -l.
Conceptually we should not think of "function-local" but just "function variables" to avoid confusion. A local variable is local to the innermost block; if that block is a function it is a function variable. Local variables become the "advanced" case.
One wrinkle is that set --show will still call function variables "local" but I think that's not a big deal.
Now in terms of function-scope, local is the odd one out.
|
Alright, merged. |
This makes the "unspecified" scope available - the one that is usedwhen no variable exists.
It's either the enclosing function's topmost local scope, or global(after deliberation, using global felt awkward because that would make the variable available everywhere, so we're just local)scope if there is no function.
This removes the need to declare variables locally before use. E.g.:
could be written as
Note: Many scripts shipped with fish use workarounds like
and/orinstead of
if, so it isn't easy to find good examples.Also, if there isn't an else-branch in that above, just with
that means something different from setting it before! Now, if
conditionisn't true, it would use a global (or universal) variable ofte same name!
Some more interesting parts:
Because it is a local scope, setting a variable
-fand-lin the toplevel of a function ends up the same:but setting it locally inside a block creates a new local variable
that shadows the function-scoped variable:
This is how local variables already work. "Local" is actually "block-scoped".
Also
set --showwill only show the closest local scope, so it won'tshow a shadowed function-level variable. Again, this is how local
variables already work, and could be done as a separate change.
Fixes #565
One of the tests explicitly checks one case mentioned by @ridiculousfish in #565 (comment):
This isn't an issue, because function scope is the outermost local scope of the function. There is no difference. It's not a new node.
TODOs: