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

[builtin/assign] Do not remove existing arrays by 'declare -a array'. #731

Merged

Conversation

akinomyoga
Copy link
Collaborator

When declare -a array is performed on existing arrays, the array becomes empty. In this PR, the arrays are initialized only when the variable contains another type of value. This also fixes the same problem for associative arrays.

# Normal variables are not removed

$ bash -c 'var=hello; declare -r var; declare -p var'
declare -r var="hello"
$ osh -c 'var=hello; declare -r var; declare -p var'
declare -r var=hello

# Arrays and associative arrays are removed in Oil

$ bash -c 'arr=(1 2 3); declare -a arr; declare -p arr'
declare -a arr=([0]="1" [1]="2" [2]="3")
$ osh -c 'arr=(1 2 3); declare -a arr; declare -p arr'
declare -a arr=()

@akinomyoga
Copy link
Collaborator Author

akinomyoga commented Apr 23, 2020

With the patches #728..#731, now the line editor works (yet many parts of ble.sh including UTF-8 support, syntax highlighting, command execution, command history, visual bells, vim mode, completions, etc. are not working).

@andychu
Copy link
Contributor

andychu commented Apr 23, 2020

Hm I'm curious where does ble.sh need to say declare -a arr on a variable that already exists? Is this because you're changing the type of existing variables?

I tend to use "declare/local" to initialize once, and then mutate it later with a=1 (no keyword), I guess like C and JavaScript.

Same with local -a arr; a=other; local -a arr, although I guess this change fixes that too.


I feel like this falls mostly under:

https://www.oilshell.org/release/0.8.pre4/doc/known-differences.html#values-are-tagged-with-types-not-cells

@akinomyoga
Copy link
Collaborator Author

Hm I'm curious where does ble.sh need to say declare -a arr on a variable that already exists?

Actually, a problem occurred with associative arrays in ble.sh but not with arrays. When one wants to add a value to associative arrays which may or may not exist already, one needs to initialize an associative array in the case it doesn't exist already.

function user-setting {
  local dict_name=${1}_dict
  declare -gA "$dict_name"
  eval "$dict_name[\$2]=\$3"
}
user-setting hello aaa bbb

Of course, maybe one can rewrite it in the form like is-assoc-array "$dict_name" || declare -gA "$dict_name". But all the other shells supporting arrays do the equivalent thing with simply declare -a arr or declare -A dict.

Is this because you're changing the type of existing variables?

No, this PR does not affect when the type of existing variable changes. This PR only changes the behavior when the type is unchanged, i.e., when declare -a arr is performed on existing arrays and when declare -A dict is performed on existing associative arrays. The current Oil removes the existing arrays with declare -a arr which is inconsistent with the behavior of declare var which does not remove existing scalar variables or array variables.

Note that in Bash, the conversion from scalars to arrays or associative arrays are allowed, but the opposite direction and the conversion between associative arrays and arrays are not allowed. In the allowed cases, the value is always preserved in some way. When a scalar is converted to an array (associative array), the original value becomes the first element (the value for the key '0').

Same with local -a arr; a=other; local -a arr, although I guess this change fixes that too.

The above case will not be affected by the PR because it changes the type.

Copy link
Contributor

@andychu andychu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK I see. Yeah I guess it makes sense. I kind of don't like how it now depends on the old value, but I can't think of a better way to do it.

I guess it could be behind an option. But the reason I have var in Oil is so we can design our own semantics, so declare can retain compatible semantics.

I think it could be cleaner if _ReconcileTypes now depends on the old values. But since the tests pass we can leave it for now and refactor later.

@andychu andychu merged commit eba9532 into oils-for-unix:master Apr 23, 2020
@akinomyoga akinomyoga deleted the fix-declare-overwriting-arrays branch April 23, 2020 23:25
@akinomyoga
Copy link
Collaborator Author

Thank you!

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

Successfully merging this pull request may close these issues.

2 participants