◐ Shell
clean mode source ↗

bpo-32012: Disallow trailing comma after genexpr without parenthesis. by serhiy-storchaka · Pull Request #4382 · python/cpython

ncoghlan

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test and compiler updates look good to me, just some minor grammar suggestions for the NEWS entry.

@@ -0,0 +1,4 @@
SyntaxError now is raised when a generator expression without parenthesis is

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor grammar fix: "now is" -> "is now"

@@ -0,0 +1,4 @@
SyntaxError now is raised when a generator expression without parenthesis is
passed as argument but followed by trailing comma. A generator expression

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"... as an argument, but followed by a trailing ..."

@serhiy-storchaka

ncoghlan

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this version goes too far - decorator factories that accept generator expressions don't break any syntactic design principles, and we don't check for any other "definitely not a valid base class" expressions in base class lists.

(The latter change might be an interesting one to make, but it should be a separate enhancement request that checks for all the constructs with a guaranteed non-class return type, and explicitly considers the question of metaclasses that might be doing something unusual with the base class list)


f(1 for x in [1],)

@deco(1 for x in [1])

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's wrong with this one? While we've restricted the form that decorator expressions take to being a name (dotted or otherwise) plus an optional function call, we've never restricted the arguments passed to that function call.

At the generator expression level, it also still meets the "must be surrounded by parentheses rule".

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The same as with

or

or

This looks like valid Python expression syntax, but the syntax of decorator expression is intentionally more limited.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This aspect of the change is more akin to prohibiting @deco(a+b*c) - once the top level expression is in the form of a function call, the fact that it's part of a decorator expression shouldn't be altering what's permitted in the argument list.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In any case these syntax changes should be discussed and officially approved. Could you please open a topic on Python-Dev about omitting parenthesis in a generator expression in these two cases? I have arguments for these changes and against them, but I think you will formulate them better. I'll add my comments.

Despite the fact that currently the CPython parser allows such syntax, it is illegal, and allowing it officially is a changing of Python language specification.

def f():
pass

class C(1 for x in [1]):

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one seems a bit odd to me as well, as we don't disallow other expressions in base class lists at compile time just because we know they can't produce a valid base class:

>>> class C([]): pass
... 
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: list() takes at most 1 argument (3 given)
>>> class C({}): pass
... 
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: dict expected at most 1 arguments, got 3

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You still can use

>>> class C((1 for x in [1])): pass
... 
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: cannot create 'generator' instances

Omitting parenthesis in a generator expression is a special case for improving readability when pass a sole generator expression argument. The syntax of class definition doesn't have such special case.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While I think there's merit to that argument, I don't think it belongs in the same PR (or issue) as the one that addresses the problem with mishandling trailing commas in function calls. Unlike the trailing comma case, there's no syntactic ambiguity being addressed.

@bedevere-bot

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

benjaminp

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to disallow a lot more than trailing commas in call genexps.


static expr_ty
ast_for_call(struct compiling *c, const node *n, expr_ty func)
ast_for_call(struct compiling *c, const node *n, expr_ty func, int allowgen)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

allowgen should be bool parameter and the callers should pass true or false.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Python/ast.c:614:72: error: unknown type name ‘bool’; did you mean ‘_Bool’?
Python/ast.c:1548:53: error: ‘false’ undeclared (first use in this function); did you mean ‘fabsl’?
Python/ast.c:2371:60: error: ‘true’ undeclared (first use in this function); did you mean ‘time’?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You need to #include <stdbool.h>

@serhiy-storchaka

This seems to disallow a lot more than trailing commas in call genexps.

Only makes the implementation more conforming to the specification.

@serhiy-storchaka

I have made the requested changes; please review again.

@bedevere-bot

Thanks for making the requested changes!

@ncoghlan: please review the changes made to this pull request.

ncoghlan

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! We can follow up on the others on the issue tracker, and potentially take them to python-dev if Guido thinks that's appropriate.

@serhiy-storchaka

@serhiy-storchaka

Created #4400 for fixing class definitions.

tacaswell added a commit to tacaswell/toolz that referenced this pull request

Nov 16, 2017

tacaswell added a commit to tacaswell/toolz that referenced this pull request

Nov 16, 2017