String annotations [PEP 563] by gvanrossum · Pull Request #4390 · python/cpython (original) (raw)

ast_unparse.c is a close translation of the relevant parts of Tools/unparse.py. I didn't want to create an entire new thing from scratch as I'd miss edge cases that way for sure. Tools/unparse.py uses parens liberally as this is the simplest way to ensure the order of operations is preserved. To know if it's safe to omit a paren, a sub-expression would need to know where it's being emitted (e.g. the super-expression). That's way more complicated than what is already done. So, Tools/unparse.py makes no effort to omit parens when they aren't needed. This, as Guido points out, produces types that mypy is unable to parse (like "Dict[(str, int)]"). That won't fly for us so my C implementation allows for omitting parens under certain circumstances already (like the tuple index in the previous example). There are many tests around this. The goal was to not put any spurious parens in typical expressions used in typing. I didn't focus on minimizing parens in expressions which aren't valid types per PEP 484. Two enhancements that are definitely possible: - math operation ordering; and - omitting comma-catching parens if there is no comma in the inner expression (like in comprehensions, dict literals, etc.) I'll look into this next week. I am worried that such cleanup effort is likely to lead to bugs (expressions that end up semantically different from their original form). A spurious pair of parens is way less harmful than that. Speaking of harm, Serhiy, do you have a legit example where we can get to a stack overflow due to spurious parens? While this is theoretically possible, I think we'd have to maliciously create an expression pathological enough to trigger this condition. Any other parse errors or crashes that spurious parens induce? Serhiy, also, thank you for spending your time on reviewing this. I appreciate it. Your first round of review was already super helpful! Let me know if I can do anything to make review easier for you.

…

-- Best regards, Łukasz Langa

On Nov 22, 2017, at 11:37 AM, Guido van Rossum ***@***.***> wrote: Good observation. Also, mypy doesn't like redundant parentheses in type expressions. (Though it won't ever encounter these, since it parses the source, so maybe it doesn't matter.) On Wed, Nov 22, 2017 at 11:32 AM, Serhiy Storchaka ***@***.*** > wrote: > It will take a time for making a review of such large change. But one > comment I can say now. > > The unparser adds parenthesis for grouping subexpression. They are added > even if not strictly needed, e.g. in a + (b * (c ** d)). The problem is > not that redundant parenthesis makes an expression less readable. The > problem is that they increase the stack consumption when parse the > expression again. It is possible that the original expression can be > parsed, but parsing the unparsed expression will fail or even crash. > > I already encountered with similar problem when worked on the parser of > plural form expressions in gettext.py. A C-like syntax is parsed and > converted to Python syntax, and the result is evaluated. I minimized the > use of parenthesis. If the subexpression operator has higher priority than > the operator of the outer expression, parenthesis are not added. > > This is not a blocker, and we can solve this problem later, but you can > think about this while I'm making a review. > > — > You are receiving this because you authored the thread. > Reply to this email directly, view it on GitHub > <#4390 (comment)>, or mute > the thread > <https://github.com/notifications/unsubscribe-auth/ACwrMqGgQ0gZQAtXZIJpySJuO8Q7_UV0ks5s5HbagaJpZM4Qco6U> > . > -- --Guido van Rossum (python.org/~guido) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.