Extra Parenthesis in expressions

[aoeu256]

In[15]: astor.to_source(ast.parse('1+2')) Out[15]: '(1 + 2)'

[baallezx]

I ran in to this as well. You can disable it by commenting out the BinOp @enclose('()') declaration but then it can break the logic of some of the other statements.

[berkerpeksag]

Thanks for the feedback, @baallezx! Will try to fix this soon.

[aoeu256]

Hmm this would be easy to fix if there was a way to walk the AST, but as it yielded each node it annotated each node with a parent (and maybe a next, and previous) node reference. You'd check for Expr nodes that have nodes that are not Expr. You could also collect a dictionary of names that pointed to the last node that used that name.

[pmaupin]

You'd check for Expr nodes that have nodes that are not Expr.

I don't know that it's that easy to do it correctly. (It may not require all that much code at the end of the day, but it certainly requires some thought.) IfExpr nodes are not Expr. Tuples are not Expr, Compare are not Expr.

You could add a lot of code to remove redundant parentheses, and if not done carefully, there will be corner cases where it generates bad output.

The other problem with this is -- where do you stop?

You can remove the parentheses around "(a+b)" to get "a+b", which means that you have to write code that examines what a and b are.

Once you do that, of course, you can remove the parentheses around "((a*b)

• (c_d))" to get "a_b+c*d", and, of course, the parentheses around "((a+b)+c)" to get "a+b+c".

But the middle parentheses in "(a+(b+c))" are necessary and cannot be removed without altering the tree. That's OK, obviously you need to change the precedence level you trigger on depending on whether it is a left or right operand.

But what about when there are three operands?

The compiler will compile "1 if (2 if 3 else 4) else 5" but will throw a syntax error on "1 if 2 if 3 else 4 else 5", even though the former would appear to be the unambiguously correct way to parse the latter. Now that you know that, you'll make sure that you don't remove those parentheses, but where else does the compiler care about them that you won't anticipate?

The current codegen module doesn't know anything about precedence, and AFAIK generates correct, compilable code that round-trips into the same AST. You could give it some knowledge of precedence, but just remember that a little knowledge is a dangerous thing :-)

Best regards, Patrick Maupin

On Tue, Aug 26, 2014 at 9:53 AM, Alexis Rodriguez notifications@github.com wrote:

Hmm this would be easy to fix if there was a way to walk the AST, but as it yielded each node it annotated each node with a parent (and maybe a next, and previous) node reference. You'd check for Expr nodes that have nodes that are not Expr. You could also collect a dictionary of names that pointed to the last node that used that name.

— Reply to this email directly or view it on GitHub https://github.com/berkerpeksag/astor/issues/14#issuecomment-53433213.

[pmaupin]

@aoeu256

Please close if the results are acceptable.

Thanks, Pat

[leonardt]

I think the correct way to solve this issue is to implement precedence and associativity in the code generator using the proper rules for Python's syntax.

The ctree package does this for emitting c-code. Files of interest:

• Precedence/Associativity encoding: https://github.com/ucb-sejits/ctree/blob/master/ctree/precedence.py
• Computing whether parentheses are required when code generating a node: https://github.com/ucb-sejits/ctree/blob/master/ctree/c/codegen.py#L20-L37

[pmaupin]

Is there something wrong with the way the pretty printer does it now?

[leonardt]

@pmaupin Nope, just commenting w.r.t.

The current codegen module doesn't know anything about precedence, and AFAIK generates correct, compilable code that round-trips into the same AST. You could give it some knowledge of precedence, but just remember that a little knowledge is a dangerous thing :-)

I agree with you, just thought I'd provide a working example of how to extend the codegen to do it correctly.

[pmaupin]

But that comment was about a suggestion to do this in an (imo) half-assed fashion.

Later, I eventually bit the bullet and did it in what IMO was a correct fashion. So if we need a separate example of how to do it correctly, I need to understand what about the current implementation isn't actually correct.

Thanks, Pat

[leonardt]

I see, I was going off the context of purely this thread so I wasn't aware of that work. I would suggest tabling this discussion until it becomes an actual issue (i.e. a bug is found in your implementation). I was just walking through issues open w.r.t. the 0.6 release as I'd like to make the release happen (I am working on some projects that depend on this release)

[pmaupin]

Unfortunately, the original submitter never responded to "Please close if the results are acceptable." so the issue's still just hanging out there. My fix will be two years old in a month, with no complaints since then, so it might be best to close it so it doesn't attract further well-meaning attention based on out-of-date results. I'll leave that decision up to @berkerpeksag

[berkerpeksag]

Yeah, let's close this issue :)