hash['val'] += 1 produces invald hash.[]("val") += 1

Question

hash['val'] += 1 produces invald hash.[]("val") += 1

MichaelSp opened this issue 6 years ago · 10 comments

MichaelSp commented 6 years ago

The following code:

hash['val'] += 1

Should unparse to
hash['val'] += 1
or
hash.[]('val', hash.[]('val') + 1)

instead of
hash.[]("val") += 1

which is SyntaxError: unexpected token tOP_ASGN

Answer 1 · 2018-12-30T23:41:40.000Z

I'm suprised rubyspec does not have such a case.

BTW:

hash.[]('val', hash.[]('val') + 1)

Is not exactly the same semantics, as:

A: Parsing it does not result in the same AST
B: It causes the engine to re-evaluate the key twice, which is not the case for an op-assign.

So

hash['val'] += 1

Is the only correct form.

Answer 2 · 2018-12-30T23:47:11.000Z

@MichaelSp I cannot reproduce this on master / latest release:

$ bundle exec unparser --verbose --evaluate 'hash['val'] += 1'
Original-Source:
hash[val] += 1
          Original-AST:
s(:op_asgn,
  s(:indexasgn,
    s(:send, nil, :hash),
    s(:send, nil, :val)), :+,
  s(:int, 1))
          Generated-Source:
hash[val] += 1
          Generated-AST:
s(:op_asgn,
  s(:indexasgn,
    s(:send, nil, :hash),
    s(:send, nil, :val)), :+,
  s(:int, 1))
Success: (string)

So please update your unparser version.

Answer 3 · 2018-12-30T23:49:00.000Z

@MichaelSp It might be you can provide a more complex input that triggers the wrong generation, I'm happy to re-open this issue in that case.

Answer 4 · 2018-12-30T23:55:26.000Z

Thanks for your insanely fast response.
I came across this while working on a PR hyperstack-org/hyperstack#95

But maybe I got this wrong..
Here is the failing build https://travis-ci.org/hyperstack-org/hyperstack/jobs/473685344
It supposed to use 0.4.2 https://github.com/hyperstack-org/hyperstack/blob/78561680789e79a90dff86b02231cf7d6e71b338/ruby/hyper-operation/Gemfile.lock#L336
The relevant code: https://github.com/hyperstack-org/hyperstack/blob/78561680789e79a90dff86b02231cf7d6e71b338/ruby/hyper-operation/spec/hyper-operation/server_op_spec.rb#L290-L294

Am I missing something?

Answer 5 · 2018-12-30T23:59:58.000Z

Okay. Thanks for the details. So it looks that you are indeed using the latest release.

I need to know the source around the source you gave me. Its unlikely but possible that this may trigger the different form. I doubt it.

Its more likely that the AST being fed into unparser is NOT the modern AST format (see readme). Can you print the AST before unparsing? Unparser makes not attempts to validate the AST, if you fed it the old format it may very well produce undesired results.

I've limited resources, I can only support the AST format I use by myself.

Answer 6 · 2018-12-31T00:07:50.000Z

Here is the complete AST: https://gist.github.com/MichaelSp/5bc013b1d008e11e81c65f2ee83ab78b
No idea which version that is...

By the way: its generated from this line: https://github.com/hyperstack-org/hyperstack/blob/78561680789e79a90dff86b02231cf7d6e71b338/ruby/hyper-spec/lib/hyper-spec/component_test_helpers.rb#L160

Answer 7 · 2018-12-31T00:09:29.000Z

Old format: https://gist.github.com/MichaelSp/5bc013b1d008e11e81c65f2ee83ab78b#file-ast-rb-L33-L39

See on how these nodes are not indexasgn but regular sends.

Solution: Either change the parser to opt into modern format, or downgrade unparser.

Answer 8 · 2018-12-31T00:10:39.000Z

BTW: It could also be that your use of unparser gets fed an manually constructed AST. I know nothing about your use case and cannot spend time to learn it. In that case you need to construct modern AST ;)

Answer 9 · 2018-12-31T00:12:38.000Z

AST is generated using https://github.com/whitequark/parser version 2.3.3.1
just saw that they have a newer version... gonna try that

Answer 10 · 2018-12-31T00:13:30.000Z

@MichaelSp mhh, TBH you missed my hints that you manually have to change the parser API usage use to opt into the modern AST format.

Please read the readme as I instructed you to do.