Unparser adds parentheses to a + b + c

[mvidner]

Thank you for Unparser!

I've found that a + b + c unparses to (a + b) + c, which I think is a bug. Here is an addition to the spec to demonstrate it: https://github.com/mvidner/unparser/commit/6f256ac89e83899a773cc44ea008d56129a1ce51

[mbj]

@mvidner Its not wrong. Its just not beautiful from your POV ;)

bundle exec ruby-parse -e 'a + b + c'
(send
  (send
    (send nil :a) :+
    (send nil :b)) :+
  (send nil :c))

As you see, a + b gets evaluated before (a + b) + c. Unparsers unparses this into (a + b) + c perfectly valid and semantically equivalent ruby code.

Unparser produces equivalent source code. Not identical. Its a tool that should produce ruby code with the SAME semantics than the AST being fed into it, for tooling. Ruby is a freaking complex language to parse and to generate. I need to to some trade offs to keep the amount of complexity managable.

When you can make a patch that makes unparser avoid unneded parens for this case and that passes all of its specs, including round tripping of rubyspec I'd consider to pull it.

I'll close this issue as I do not see any kind of defect nor a feature I wanna build. Feel free to discuss here anyway.

[mvidner]

Thanks for a quick reply! I agree with your points. Yes, it is not as beautiful as I would like. I'll see whether I can fix it myself.

[mbj]

@mvidner What is your use case?

[7even]

I believe this is a place to look at.

[mbj]

@7even Remove it and see tons of edge cases fail.

[mvidner]

@mbj The use case is Zombie Killer.

Basically we have tons of Ruby code which is ugly, because it was translated from a legacy language, striving for bug-compatibility. I am trying to analyze the code for situations where it is safe to replace the ugly variants with nice ones.

Here is an example of what the tool can do now. Apart from a problem with making lines too long, you can see that we have to deal with long addition chains and it would be nice to get rid of the parens.

[7even]

@mbj I didn't mean it should be removed, but if the solution to the problem exists, it should be somewhere around terminating the first operand in my opinion. Though it seems like there's no way to determine if it needs the parentheses without looking to the parent node.

[mbj]

@7even Exactly. And that problem goes really deep. I mentioned in a pairing that its easy to rewrite the AST when preprocessing to include artificial nodes that can be emitted in a specific style. Nodes could be marked as "sanitized" already. For example we could include an artificial "terminated" node type that can wrap binary operators we consider "safe" to emit unparenthised for a certain use case.

But I for myself do not have a use case for this, and I have a strict policy only to create features for my OSSed libraries when:

• Some of my other OSS activities require it.
• Someone pays me to do it and pays for maintenance.

When a patch for a feature makes it into PR I'll probably accept it in case it does not create a maintenance burden.

Unparser was written to support mutant, and as much as I love to see other people using it for other proposes, I cannot make everyone happy on my time. This is simply not possible.

Else OSS can easily get a rabbit hole consuming my life and burn me out.

[mbj]

@mvidner Thats actually a cool use case. I think you maybe should accept the way mutant outputs it code currently as an intermediary step? As long as it is an improvement I'd do any improvement that is available via a low hanging fruit. Unparser currently is very stable and I think you might be better of accepting some extra parenthesis now and do a 2nd pass later.