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 assignment where var name is dynamic doesn't work #640

Closed
andychu opened this issue Mar 8, 2020 · 4 comments
Closed

arith assignment where var name is dynamic doesn't work #640

andychu opened this issue Mar 8, 2020 · 4 comments

Comments

@andychu
Copy link
Contributor

andychu commented Mar 8, 2020

From #620, ble.sh uses this in several places. All shells support it

@andychu
Copy link
Contributor Author

andychu commented Mar 10, 2020

Actually I realized there is an asymmetry as @akinomyoga pointed out on #648 .

A variable name is another type of arithmetic expression, like 1+2.

As mentioned, there's only one level of evaluation, not recursive evaluation.

(( x = ${prefix}x ))   # invalid integer constant in Oil.  NOT a variable name.

So then if we have this:

(( ${prefix}x = x ))

Then it seems inconsistent to treat it as a variable name. It's possible to do but it's a little trickier than I thought it would be. I have half a patch now.

We would need to get rid of the arith_expr.VarRef variant (which is arguably more consistent) and LooksLikeArithVar, etc.

The main reason I wanted to do this is to at least be able to PARSE ble.sh (#653). I think it's the last thing that doesn't parse.

@andychu
Copy link
Contributor Author

andychu commented Mar 10, 2020

I'm leaving this aside for now, but there is a partial implementation on branch dev/andy-20.

https://github.com/oilshell/oil/commits/dev/andy-20

Half of it is that I'm unsure about the semantics, and half of it is that it's a bigger change than I thought.


For now cases 52-55 in spec/arith.test.sh will fail ...

@andychu
Copy link
Contributor Author

andychu commented Mar 10, 2020

On the other hand, one reason I wanted to fix this was because it also addresses another "smell" in the code:

  • sh_lhs_expr_t is shared between arithmetic and shell assignments, which violates the "LST" style
  • I would be better to have arith_expr.Word -> lhs_expr, and sh_lhs_expr -> lhs_expr, which is the "IR" mentioned in consider an IR for words and ${} #604
    • although the IR would have to be constructed at runtime rather than parse time if we have these dynamic variable names

Also, a workaround is

(( ${prefix}x = 1 + 2 ))     # can be rewritten
eval "${prefix}x"=$(( 1 + 2 ))

(( ${prefix}x[i] = 1 + 2 ))    # can be rewritten
eval "${prefix}x[i]"=$(( 1 + 2 ))

andychu pushed a commit that referenced this issue Mar 12, 2020
Addresses #640 for ble.sh.  But this option could be used in other
cases where we're thinking about expanding the Oil language.
@andychu
Copy link
Contributor Author

andychu commented Mar 21, 2020

#670

andychu pushed a commit that referenced this issue Apr 5, 2020
Allows (( ${prefix}_x = y )), (( ${prefix}_x[1] = y )), etc.

- Addresses issue #640
- Related to ble.sh #653
@andychu andychu closed this as completed Apr 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant