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

Defining a local variable in a function can remove export-bit outside of the function #2611

Closed
faho opened this Issue Dec 14, 2015 · 5 comments

Comments

Projects
None yet
2 participants
@faho
Copy link
Member

faho commented Dec 14, 2015

Like @krader1961 says in #2601 (comment):

Note that the set -l LC_ALL C removes the exported attribute if that var was already exported. You can see this for yourself:

$ set -x LC_ALL en_US.UTF-8
$ env | grep LC_ALL
LC_ALL=en_US.UTF-8
$ function wtf
    set -l LC_ALL C
    env
end
$ wtf | grep LC_ALL
$ echo LC_ALL
en_US.UTF-8
$ env | grep LC_ALL

Notice that after invoking wtf the LC_ALL var is still defined but is no longer in the subprocess environment.

@faho faho added the bug label Dec 14, 2015

@krader1961

This comment has been minimized.

Copy link
Contributor

krader1961 commented Dec 15, 2015

Should making a local change without explicitly saying the new value should be exported result in the var not being exported to any external command? If the answer is yes then external commands won't get the setting in effect for the shell (which is a big deal for LANG or the LC_* vars). In which case we can implicitly inherit the export attribute. Alternatively, we can define the semantics to be that the pre-existing value is still exported to external commands and the set -l only affects the shell code executed within the scope of that function.

@faho

This comment has been minimized.

Copy link
Member

faho commented Dec 15, 2015

Should making a local change without explicitly saying the new value should be exported result in the var not being exported to any external command?

The relevant bit from the set documentation:

  1. If a variable is not explicitly set to be either exported or unexported and has never before been defined, the variable will not be exported.

That's a yes.

In which case we can implicitly inherit the export attribute.

That's basically the previous rule:

  1. If a variable is not explicitly set to be exported or not exported, but has been previously defined, the previous exporting rule for the variable is kept.

The question here being of course whether a variable by the same name in a different scope counts as "previously defined". I'd be okay with interpreting it as such since for overrides like LC_* this would be what you want. Of course the docs should be adjusted to explicitly mention this.

However, the main problem I see here is that defining a variable in a smaller scope alters variables in a bigger scope - meaning that a function can break stuff outside of it (and quite unintentionally).

@faho

This comment has been minimized.

Copy link
Member

faho commented Jun 21, 2017

Okay, the issue here is the following:

When a scope ends (i.e. var_stack_t::pop() is called), we currently only recompute the export-array when the ending scope has exported variables:

    if (top->new_scope) {  //!OCLINT(collapsible if statements)
        if (top->exportv || local_scope_exports(top->next.get())) {
            this->mark_changed_exported();
        }
    }

This is wrong in this case. The array needs to be recomputed, not because the dying scope has exported variables, but because it has variables that it removes from exporting.

TBD is how we solve that - do we just always recompute the export-array when a scope ends? Presumably that has a performance impact. Alternatively, we could set a scope's "exportv" bool if the node changes something about exports, not just when it has exported variables. I've made that change locally and all tests still pass.

faho added a commit to faho/fish-shell that referenced this issue Jun 21, 2017

faho added a commit to faho/fish-shell that referenced this issue Jun 21, 2017

@faho

This comment has been minimized.

Copy link
Member

faho commented Jun 21, 2017

we could set a scope's "exportv" bool if the node changes something about exports, not just when it has exported variables. I've made that change locally and all tests still pass.

Okay, I have now included this in #4149.

krader1961 added a commit to krader1961/fish-shell that referenced this issue Jul 20, 2017

krader1961 added a commit to krader1961/fish-shell that referenced this issue Jul 20, 2017

krader1961 added a commit that referenced this issue Jul 21, 2017

@krader1961

This comment has been minimized.

Copy link
Contributor

krader1961 commented Jul 21, 2017

The fix by @faho has been merged into the git major branch for the upcoming fish 3.0.0 release.

@krader1961 krader1961 closed this Jul 21, 2017

@krader1961 krader1961 modified the milestones: fish-3.0, fish-future Jul 21, 2017

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