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

[arith] Support empty arith. #725

Closed

Conversation

akinomyoga
Copy link
Collaborator

This fixes the following problem.

Actually this was a N-I feature rather than a bug. It is related to an empty string in arithmetic context. I'm not sure if Oil should support this by default or not, but this is a patch to support empty strings in arithmetic context.

In this implementation, I derived a class ArithParser from TdopParser to add a special rule for empty arithmetic expressions.

@andychu
Copy link
Contributor

andychu commented Apr 22, 2020

Hm does any program need $(( )) and (( ))? If not I'd rather keep it stricter. dash and yash don't support it. Although it's true they're in the minority, I can't think of any reason you would want $(( )) instead of say $(( 0 )).

I'm also confused about ${arr[@]::}. Doesn't that just give you an empty array always? What use is it?

I suppose it's equivalent to ${arr[@]:0:0}, and that is easier to read.

@andychu
Copy link
Contributor

andychu commented Apr 22, 2020

I think this falls under the common subset principle: https://github.com/oilshell/oil/wiki/Language-Design-Principles

If there is a trivial rewrite (that may improve your program), it may be OK to disagree with other shells. Generally speaking I think existing shells accept too much code without failing. (That's why there are many shopt -s strict_* options in Oil.)

@akinomyoga
Copy link
Collaborator Author

akinomyoga commented Apr 22, 2020

Thank you for comments! I don't think people will directly write something like $(()), but I assumed cases such as

let() { eval "(($*))"; }
## @fn process [param]
process() { let "$1"; }
process

Or,

## @fn func [extra_count]
func() {
  local -a extra_words=(1 2 3 4)
  eval "joined=\"abc\${extra_words[*]::$1}\""
}
func

I encountered a similar (but not exactly the same) case while testing ble.sh. Actually the exact ${arr[@]::} case will not appear in ble.sh, so there is no problem with ble.sh. Nevertheless, it is still possible some Bash scripts fall into this pattern. They can be easily patched to make it work without empty arithmetic expressions if we find the problem, but these patterns cannot be statically detected so we need to perform enough tests for scripts.

Another thing is that it may be more consistent with the treatment of empty variables in arithmetic expressions. The empty variables will be evaluated as 0 in arithmetic expressions.

$ osh -c 'var=; echo $((var))'
0
$ osh -c 'var=; echo $(($var))'
0
$ osh -c 'shopt -s eval_unsafe_arith; var=; echo $(($var))'
0

But I can understand this is too permissive in the view point of the language design. That is the reason that I wrote the following sentence in the first post.

I'm not sure if Oil should support this by default or not

Maybe we can support it under shopt -s eval_unsafe_arith or another option?

@akinomyoga
Copy link
Collaborator Author

I added shopt -s parse_empty_arith as an experiment. I can drop or rename this option anytime.

@andychu
Copy link
Contributor

andychu commented Apr 22, 2020

Hm I'd rather wait until we have evidence that people need it. It sounds like you don't need it right now.

This is similar to the unset thing -- let's stick with the simpler rules until someone complains about it. Otherwise we could be adding special cases for 10 years!

(BTW in general I'm interested in what the next thing is to make some tests pass in ble.sh. I guess you hit the FUNCNAME bug while testing? And I still want to add basic coverage support so we can see how many lines are hit when tests pass. If we can get 100 or 1000 lines passing that will be nice.)


But I think the rewrites to avoid empty arithmetic are always improvements. Keep in mind that you wrote one of the biggest shell programs in the world, so your knowledge of bash far exceeds the average user. Oil is supposed to be for the average user, not the most sophisticated user! :-)

I'm also trying to keep the code small, which is why I count lines on every release.

And super() doesn't translate to C++, etc. (If it were a a very valuable feature that wouldn't be an issue; I would make it translate myself.)

We're trying to avoid more global options too: https://github.com/oilshell/oil/wiki/Language-Design-Principles

@akinomyoga
Copy link
Collaborator Author

OK! Thank you!

@akinomyoga akinomyoga force-pushed the support-empty-arith branch from 57b464e to 2c1b487 Compare April 24, 2020 00:40
@andychu andychu closed this Mar 21, 2021
andychu pushed a commit that referenced this pull request Jun 26, 2024
The the missing length is interpreted as zero.

Even though

- It appears undocumented
- zsh doesn't agree

This came up on PR #725 a few years ago, for ble.sh

Discussion:

    https://oilshell.zulipchat.com/#narrow/stream/121540-oil-discuss/topic/.24.7Barr.5B.40.5D.3A.3A.7D.20in.20bash.20-.20is.20it.20documented.3F
momiji pushed a commit that referenced this pull request Jun 26, 2024
The the missing length is interpreted as zero.

Even though

- It appears undocumented
- zsh doesn't agree

This came up on PR #725 a few years ago, for ble.sh

Discussion:

    https://oilshell.zulipchat.com/#narrow/stream/121540-oil-discuss/topic/.24.7Barr.5B.40.5D.3A.3A.7D.20in.20bash.20-.20is.20it.20documented.3F
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