Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TargetScopeError not raised for comprehension scope conflict #81938

Closed
ncoghlan opened this issue Aug 5, 2019 · 16 comments
Closed

TargetScopeError not raised for comprehension scope conflict #81938

ncoghlan opened this issue Aug 5, 2019 · 16 comments
Assignees
Labels
3.8 only security fixes 3.9 only security fixes release-blocker type-bug An unexpected behavior, bug, or error

Comments

@ncoghlan
Copy link
Contributor

ncoghlan commented Aug 5, 2019

BPO 37757
Nosy @gvanrossum, @warsaw, @ncoghlan, @ambv, @serhiy-storchaka, @emilyemorehouse
PRs
  • bpo-37757: Disallow PEP 572 cases that expose implementation details #15131
  • [3.8] bpo-37757: Disallow PEP 572 cases that expose implementation details #15491
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = 'https://github.com/ncoghlan'
    closed_at = <Date 2019-08-25.14:43:28.483>
    created_at = <Date 2019-08-05.00:15:02.884>
    labels = ['type-bug', '3.8', '3.9', 'release-blocker']
    title = 'TargetScopeError not raised for comprehension scope conflict'
    updated_at = <Date 2019-08-25.14:43:28.483>
    user = 'https://github.com/ncoghlan'

    bugs.python.org fields:

    activity = <Date 2019-08-25.14:43:28.483>
    actor = 'ncoghlan'
    assignee = 'ncoghlan'
    closed = True
    closed_date = <Date 2019-08-25.14:43:28.483>
    closer = 'ncoghlan'
    components = []
    creation = <Date 2019-08-05.00:15:02.884>
    creator = 'ncoghlan'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 37757
    keywords = ['patch']
    message_count = 16.0
    messages = ['349012', '349022', '349033', '349057', '349084', '349092', '349094', '349100', '349139', '349141', '349632', '350292', '350310', '350455', '350460', '350461']
    nosy_count = 7.0
    nosy_names = ['gvanrossum', 'barry', 'ncoghlan', 'lukasz.langa', 'serhiy.storchaka', 'Damien George', 'emilyemorehouse']
    pr_nums = ['15131', '15491']
    priority = 'release blocker'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue37757'
    versions = ['Python 3.8', 'Python 3.9']

    @ncoghlan
    Copy link
    Contributor Author

    ncoghlan commented Aug 5, 2019

    While implementing PEP-572, Emily noted that the check for conflicts between assignment operators and comprehension iteration variables had not yet been implemented: https://bugs.python.org/issue35224#msg334331

    Damien George came across this discrepancy while implementing assignment expressions for MicroPython.

    The proposed discussion regarding whether or not the PEP should be changed didn't happen, and the PEP itself misses the genuinely confusing cases where even an assignment expression that *never executes* will still make the iteration variable leak:

    >>> [i for i in range(5)]
    [0, 1, 2, 3, 4]
    >>> [i := 10 for i in range(5)]
    [10, 10, 10, 10, 10]
    >>> i
    10
    >>> [False and (i := 10) for i in range(5)]
    [False, False, False, False, False]
    >>> i
    4

    And that side effect happens even if the assignment expression is nested further down in an inner loop:

    >>> [(i, j, k) for i in range(2) for j in range(2) for k in range(2)]
    [(0, 0, 0), (0, 0, 1), (0, 1, 0), (0, 1, 1), (1, 0, 0), (1, 0, 1), (1, 1, 0), (1, 1, 1)]
    >>> i
    Traceback (most recent call last):
      File "<stdin>", line 1, in <module>
    NameError: name 'i' is not defined
    >>> [(i, j, k) for i in range(2) for j in range(2) for k in range(2) if True or (i:=10)]
    [(0, 0, 0), (0, 0, 1), (0, 1, 0), (0, 1, 1), (1, 0, 0), (1, 0, 1), (1, 1, 0), (1, 1, 1)]
    >>> i
    1

    I'm at the PyCon AU sprints today, and will be working on a PR to make these cases raise TargetScopeError as specified in the PEP.

    @ncoghlan ncoghlan added deferred-blocker 3.8 only security fixes 3.9 only security fixes labels Aug 5, 2019
    @ncoghlan ncoghlan self-assigned this Aug 5, 2019
    @ncoghlan ncoghlan added the type-bug An unexpected behavior, bug, or error label Aug 5, 2019
    @gvanrossum
    Copy link
    Member

    Thanks for being part of the village raising this child!

    @ncoghlan
    Copy link
    Contributor Author

    ncoghlan commented Aug 5, 2019

    Added a PEP update as well: python/peps#1140

    @warsaw
    Copy link
    Member

    warsaw commented Aug 5, 2019

    I know the PEP defines TargetScopeError as a subclass of SyntaxError, but it doesn't really explain why a subclass is necessary. I think seeing "TargetScopeError" will be a head scratcher. Why not just SyntaxError without introducing a new exception?

    @gvanrossum
    Copy link
    Member

    [Barry]

    I know the PEP defines TargetScopeError as a subclass of SyntaxError, but it doesn't really explain why a subclass is necessary. I think seeing "TargetScopeError" will be a head scratcher. Why not just SyntaxError without introducing a new exception?

    Hm, that's not a bad point. We report all sorts of things found by the bytecode compiler as SyntaxError.

    OTOH This would require a PEP change, formal review, etc. (IMO much more so than adding the new edge case that Nick and Damien discovered.) Maybe the best way of doing this would be to implement TargetScopeError now, then start the debate about killing it, and try to get that in before beta4?

    @ncoghlan
    Copy link
    Contributor Author

    ncoghlan commented Aug 6, 2019

    I believe our main motivation for separating it out was the fact that even though TargetScopeError is a compile-time error, the affected code is syntactically fine - there are just issues with unambiguously inferring the intended read/write location for the affected target names. (Subclassing SyntaxError is then a pragmatic concession to the fact that "SyntaxError" also de facto means "CompilationError")

    Searching for "Python TargetScopeError" will also get folks to relevant information far more quickly than searching for "Python SyntaxError" will.

    Pre-seeding Stack Overflow with an answer to "What does TargetScopeError mean in Python?" would probably be a good idea though (similar to what I did for https://stackoverflow.com/questions/25445439/what-does-syntaxerror-missing-parentheses-in-call-to-print-mean-in-python )

    @gvanrossum
    Copy link
    Member

    But we don't do that with any of the other (many) errors detected by later passes of the compiler. Those report dozens of SyntaxErrors, with good descriptive messages. Users can search the web for those messages too.

    Also, I doubt that many people will ever get a TargetScopeError. The examples are all esoteric -- yes, the compiler needs to reject them, but no, they are not things one is likely to try intentionally. (The exception may be the forbidden form you're adding, [... for ... in (i := ...)].)

    @serhiy-storchaka
    Copy link
    Member

    We report all sorts of things found by the bytecode compiler as SyntaxError.

    Except of these which are reported with IndentationError or its subclass TabError.

    @warsaw
    Copy link
    Member

    warsaw commented Aug 6, 2019

    OTOH This would require a PEP change, formal review, etc.

    It would be trivial though. There are only two references to TargetScopeError in the PEP. One talks about adding the exception and the other just mentions it almost in passing as a subclass of SyntaxError.

    I think it would be better to just use SyntaxError.

    @gvanrossum
    Copy link
    Member

    If you and Nick both feel strongly about this please take it to python-dev,
    I'm sure we'll get closure quickly there.

    @ncoghlan
    Copy link
    Contributor Author

    The outcome of the python-dev discussion was that we agreed to switch to raising a plain SyntaxError, as that's what we do everywhere else that this kind of problem comes up (e.g. conflicts between global and nonlocal declarations, or between those and parameter declarations, as well as the various other cases of "that statement/expression is fine in isolation, but you can't use it *here*").

    Both PRs have been updated accordingly, and the PEP PR has been merged.

    @ambv
    Copy link
    Contributor

    ambv commented Aug 23, 2019

    This is marked as a release blocker. The last beta is scheduled for Monday. Please decide how to proceed ASAP.

    @gvanrossum
    Copy link
    Member

    The decision has been made to get rid of TargetScopeError and instead just use SyntaxError. PEP-572 was updated already. We're just waiting for someone (Serhiy?) to review Nick's patch, PR bpo-15131.

    @ncoghlan
    Copy link
    Contributor Author

    New changeset 5dbe0f5 by Nick Coghlan in branch 'master':
    bpo-37757: Disallow PEP-572 cases that expose implementation details (GH-15131)
    5dbe0f5

    @ncoghlan
    Copy link
    Contributor Author

    New changeset 6ca0307 by Nick Coghlan in branch '3.8':
    [3.8] bpo-37757: Disallow PEP-572 cases that expose implementation details (GH-15491)
    6ca0307

    @ncoghlan
    Copy link
    Contributor Author

    Merged for 3.8b4 after Emily's review.

    Thanks to all involved in the PEP update and change discussion!

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.8 only security fixes 3.9 only security fixes release-blocker type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    5 participants