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.

Sign up for GitHub

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

Jump to bottom

[word_eval] Implement 'shopt -s compat_array'. #728

Merged
merged 7 commits into from
Apr 23, 2020
Merged

[word_eval] Implement 'shopt -s compat_array'. #728

merged 7 commits into from
Apr 23, 2020

Conversation

akinomyoga
Copy link
Collaborator

Ref #726

  • Fix a problem that the scalar form cannot be used in arithmetic expressions (9fc20e7 Test c2365f5 Fix):
$ osh -c '$((BASH_LINENO))'
  $((BASH_LINENO))
     ^~~~~~~~~~~
[ -c flag ]:1: fatal: Expected a value convertible to integer, got value.MaybeStrArray

  • Implement shopt -s compat_array to allow $array (Test bf9f332, Fix 6704770)

  • Disallow ${#BASH_SOURCE}, ${BASH_SOURCE:1:1}, ${BASH_SOURCE#X}, ${BASH_SOURCE-X}, etc. by default (Fix b1a16c2). In this implementation, everything with additional suffix/prefix are disallowed.

@akinomyoga akinomyoga mentioned this pull request Apr 23, 2020
Merged
andychu
andychu reviewed Apr 23, 2020
View reviewed changes
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.

Great thanks for doing this!

It doesn't look like too big a change which I'm happy about.

osh/sh_expr_eval.py
if val.tag_() in (value_e.MaybeStrArray, value_e.AssocArray) and lval.tag_() == lvalue_e.Named:
named_lval = cast(lvalue__Named, lval)
if word_eval.CheckCompatArray(named_lval.name, self.exec_opts):
val = word_eval.ResolveCompatArray(val)
Copy link
Contributor

Choose a reason for hiding this comment

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

Would this part be made obsolete if we made BASH_LINENO etc. immutable?

I guess that would make Oil technically incompatible, but I've been thinking about that. Also it makes sense to tighten up things like PATH=(a b c) and HOME=(a b c) (they shouldn't be arrays)

This doesn't have to be done in this commit; I'm just curious / brainstorming the semantics

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

As you have already written in another comment, this change also affects the other mutable variables when shopt -s compat_array is set.

osh/word_eval.py Outdated
@@ -61,6 +61,26 @@
# For compatibility, ${BASH_SOURCE} and ${BASH_SOURCE[@]} are both valid.
# ${FUNCNAME} and ${BASH_LINENO} are also the same type of of special variables.
_STRING_AND_ARRAY = ['BASH_SOURCE', 'FUNCNAME', 'BASH_LINENO']
def CheckCompatArray(var_name, opts, compat_introspect=True):
Copy link
Contributor

Choose a reason for hiding this comment

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

I would name compat_introspect something more descriptive like is_plain_var or is_plain_var_sub

( The difference between SimpleVarSub and BracedVarSub is one thing I want to get rid of with #604 )

Copy link
Collaborator Author

@akinomyoga akinomyoga Apr 23, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I renamed it to is_plain_var_sub b311fb3.

@andychu
Copy link
Contributor

andychu commented Apr 23, 2020

Ah never mind about my lvalue comment. I think this works now?

$ a=(1 0 0); (( a++ )); echo ${a[@]}
2 0 0

If so then I think we should add things like that as test cases. I don't think the tests are hitting some code paths.

@akinomyoga
Copy link
Collaborator Author

akinomyoga commented Apr 23, 2020 •
edited
Loading

I think this works now?

$ a=(1 0 0); (( a++ )); echo ${a[@]}

I added a test case 4662002 to find that it actually doesn't work. Currently, it mutates the array to a scalar. I'll fix it later.

@akinomyoga akinomyoga force-pushed the support-compat-array branch from 5ab54ad to b311fb3 Compare April 23, 2020 22:54
andychu
andychu reviewed Apr 23, 2020
View reviewed changes
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 think the continuous build might fail but let's see what happens ... e.g. the test/spec.sh numbers should generally be adjusted to keep it green.

spec/ble-features.test.sh
case ${SH##*/} in (osh) shopt -s compat_array;; esac
case ${SH##*/} in (zsh) setopt KSH_ARRAYS;; esac
arr=(foo bar baz)
echo "$arr"
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should also test ${arr} since that's a different code path. (as mentioned I want to collapse those)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I added the test for ${arr} in another PR #733.

@andychu andychu merged commit b16e087 into oils-for-unix:master Apr 23, 2020
@andychu
Copy link
Contributor

andychu commented Apr 23, 2020

Oh I also had that other comment about testing ${arr} too, maybe you can sneak it in the next commit if you're touching this...

In any case I'm interested to see what works in ble.sh is, so if there is something I can try let me know. Should I just switch to the osh branch and try to run it with osh from head?

We can put instructions here too:

https://github.com/oilshell/oil/wiki/Running-ble.sh-With-Oil

@akinomyoga
Copy link
Collaborator Author

Oh, you have already merged it. Thank you!

OK I think the continuous build might fail but let's see what happens ...

I have actually fixed it now. I'll submit it in another PR.

In any case I'm interested to see what works in ble.sh is, so if there is something I can try let me know.

Yes, now it is ready so I'm going to prepare something that can be tried out.

We can put instructions here too:

OK.

@akinomyoga
Copy link
Collaborator Author

We can put instructions here too:

I added a description in the Wiki page. You can follow the instructions Update ble.osh and Try read -e

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

@andychu andychu andychu left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@akinomyoga @andychu