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

Revisit array warnings/errors with shopt --set strict_array #2168

Open
akinomyoga opened this issue Dec 3, 2024 · 2 comments
Open

Revisit array warnings/errors with shopt --set strict_array #2168

akinomyoga opened this issue Dec 3, 2024 · 2 comments

Comments

@akinomyoga
Copy link
Collaborator

Originally posted by @andychu in #2165 (comment)

Another thing we can do is make a hard error when shopt --set strict_array, which we already have

There is an e_strict() pattern that we sometimes use for that

Though that increases the test matrix, so maybe we can do that in another pass

I don't have the capacity to handle this in parallel with the currently ongoing bug fixes, refactoring, and SparseArray implementation of arrays.

Let's create an issue before forgetting about this.

@akinomyoga
Copy link
Collaborator Author

The above is for ${a[-1]} and $((a[-1])). The following is about test -v a[-1].

Originally posted by @andychu in #2166 (comment)

Hm the other change was just a warning, and this one is an error

$ bash -c 'test -v a[-1]; echo status=$?'
bash: line 1: a: bad array subscript
status=1

I think this argues for the e_strict() style then ... we can match bash EXACTLY if we want, but then OSH should have shopt -s strict_array, which is CONSISTENT

i.e. this part would not change, but the previous $((a[-1])) and ${a[-1]} should change to be consistent, I think

There is also a[-1]=v. In addition, each of them has codes for BashArray, BashAssoc, and SparseArray.

  • Are there any other places?
  • How does Bash behave?
  • What would be the default osh behavior for each, and how would it be changed under strict_array?

@andychu
Copy link
Contributor

andychu commented Dec 3, 2024

OK yes, we can do this after SparseArray

I think my basic viewpoint is

  • bash is inconsistent about errors. But sometimes it is easier to just copy bash, since it makes the spec tests pass. And because sometimes scripts RELY on the inconsistency.
    • if bash gives a warning, give a warning. if it is a fatal error, then fatal error.
  • However, shopt --set strict_array should make ALL index of bounds a fatal error. That is more consistent and easier to explain/document. I think it's just a better experience for the user

So as another pass, we just grep IndexOutOfBounds */*.py, and make sure that if a fatal error is NOT thrown, then we throw it with strict_array


I can probably do this as long as the style is consistent, and it looks good so far

I think it is helping to put more stuff in bash_impl.py and have error codes

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

No branches or pull requests

2 participants