bpo-37757: Disallow PEP 572 cases that expose implementation details #15131
Conversation
- comprehension iteration variables are explicitly local, but named expression targets in comprehensions are nonlocal or global. Raise TargetScopeError as specified in PEP 572 - named expression targets in the outermost iterable of a comprehension have an ambiguous target scope. Avoid resolving that question now by raising TargetScopeError. PEP 572 explicitly requires this for cases where the bound name conflicts with the iteration variable in the comprehension, but CPython can't easily restrict the exception to that case (as it doesn't know the target variable names when visiting the outermost iterator expression)
ncoghlan
changed the title
|
There's a test design issue that comes up with this PR: in CPython, all the different comprehension types share several common code paths, so these tests only test list comprehensions, and rely on the common code paths to extend that to the rules also being applied correctly for set, dict, and generator comprehensions. From a language testing perspective though, perhaps we should be testing all 4 cases, as other implementations may not have the same level of code sharing, and hence have bugs specific to one of the other variants that don't exist for list comprehensions? |
ncoghlan
changed the title
- use the public "assignment expression" name rather than the internal "named expression" one - mention the variable name when the conflict is related to the exact name rather than the code structure - don't pass the variable name when the exception message doesn't need it
|
@emilyemorehouse The PEP update to remove TargetScopeError and clarify the rationale for treating these cases as syntax errors has been accepted and merged, so this is ready for review now. Edit: I've updated the PR to add more test cases now, so the test suite is covering situations where we reference a global or nonlocal in a comprehension before we rebind it, like: I was right that the flags the PR is checking only get set for target names, not references, but it's good to have the extra test cases that ensure that remains true. I've also tweaked the tests for the scoping syntax error checks to make sure they trigger in all 3 scopes (module, function, class). We normally wouldn't bother to do that, but this is a case where we know the compiler contains scope dependent code paths, so it makes sense to explicitly cover them all. |
|
I don't believe edit notifications get sent via email, so adding an explicit note to say that I've added the extra test cases I mentioned in my last comment, and edited that comment accordingly. So unless I've missed something, I believe this should be good to go now. |
|
Note that I'm deliberately not bumping the bytecode magic number here - this means that you could technically compile affected code under 3.8b3 or earlier, and 3.8b4 and later will still run it. I think that's fine, and don't see a reason to compel a rebuild of all bytecode just to enforce new syntax errors. |
|
My current plan is to merge this on Sunday August 25th, to make the 3.8b4 cut-off. If folks have comments before then, I can take them into account for the beta, otherwise, I'm still interested in post-merge reviews, but I'll handle addressing them as follow-up changes before rc1. |
ncoghlan
deleted the
bpo-37757-targetscopeerror-for-iteration-variable-conflicts
branch
|
Thanks @ncoghlan for the PR 🌮🎉.. I'm working now to backport this PR to: 3.8. |
|
Sorry, @ncoghlan, I could not cleanly backport this to |
|
@emilyemorehouse Thanks for the review! |
…ythonGH-15131) - drop TargetScopeError in favour of raising SyntaxError directly as per the updated PEP 572 - comprehension iteration variables are explicitly local, but named expression targets in comprehensions are nonlocal or global. Raise SyntaxError as specified in PEP 572 - named expression targets in the outermost iterable of a comprehension have an ambiguous target scope. Avoid resolving that question now by raising SyntaxError. PEP 572 originally required this only for cases where the bound name conflicts with the iteration variable in the comprehension, but CPython can't easily restrict the exception to that case (as it doesn't know the target variable names when visiting the outermost iterator expression) (cherry picked from commit 5dbe0f5)
|
#15491 is the deconflicted backport - removing the Windows DLL export symbol for TargetScopeError failed in the implicit backport, as the two branches have different version numbers in the symbols. |
|
This commit introduced a reference leak that is being tracked in https://bugs.python.org/issue37954 |
…ythonGH-15131) - drop TargetScopeError in favour of raising SyntaxError directly as per the updated PEP 572 - comprehension iteration variables are explicitly local, but named expression targets in comprehensions are nonlocal or global. Raise SyntaxError as specified in PEP 572 - named expression targets in the outermost iterable of a comprehension have an ambiguous target scope. Avoid resolving that question now by raising SyntaxError. PEP 572 originally required this only for cases where the bound name conflicts with the iteration variable in the comprehension, but CPython can't easily restrict the exception to that case (as it doesn't know the target variable names when visiting the outermost iterator expression)
…ythonGH-15131) - drop TargetScopeError in favour of raising SyntaxError directly as per the updated PEP 572 - comprehension iteration variables are explicitly local, but named expression targets in comprehensions are nonlocal or global. Raise SyntaxError as specified in PEP 572 - named expression targets in the outermost iterable of a comprehension have an ambiguous target scope. Avoid resolving that question now by raising SyntaxError. PEP 572 originally required this only for cases where the bound name conflicts with the iteration variable in the comprehension, but CPython can't easily restrict the exception to that case (as it doesn't know the target variable names when visiting the outermost iterator expression)
…ythonGH-15131) - drop TargetScopeError in favour of raising SyntaxError directly as per the updated PEP 572 - comprehension iteration variables are explicitly local, but named expression targets in comprehensions are nonlocal or global. Raise SyntaxError as specified in PEP 572 - named expression targets in the outermost iterable of a comprehension have an ambiguous target scope. Avoid resolving that question now by raising SyntaxError. PEP 572 originally required this only for cases where the bound name conflicts with the iteration variable in the comprehension, but CPython can't easily restrict the exception to that case (as it doesn't know the target variable names when visiting the outermost iterator expression)
as per the updated PEP 572
named expression targets in comprehensions are nonlocal or
global. Raise SyntaxError as specified in PEP 572
comprehension have an ambiguous target scope. Avoid resolving
that question now by raising SyntaxError. PEP 572
originally required this only for cases where the bound name
conflicts with the iteration variable in the comprehension,
but CPython can't easily restrict the exception to that case
(as it doesn't know the target variable names when visiting
the outermost iterator expression)
https://bugs.python.org/issue37757