Extra Parenthesis in expressions about astor HOT 12 CLOSED

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

from astor.

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.

from astor.

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

from astor.

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.

from astor.

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 [email protected]
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
#14 (comment).

from astor.

@aoeu256

Please close if the results are acceptable.

Thanks,
Pat

from astor.

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

from astor.

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

from astor.

@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.

from astor.

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

from astor.

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)

from astor.

Yeah, let's close this issue :)

from astor.