PEP 709: one behavior change that was missed in the PEP

[carljm (Carl Meyer)]

@Jelle pointed out yesterday that the implementation of PEP 709 (inlined comprehensions) caused one additional behavior change that I failed to identify in the PEP text. Consider this code:

class C:
    incr = 1
    items = [x+incr for x in range(3)]

Before PEP 709, this code would fail with NameError, because the comprehension was compiled as a nested function, and functions cannot see variables defined in enclosing class scopes.

Since the merge of the PEP 709 implementation, this code succeeds and does what one might expect at first glance; the comprehension is no longer compiled as a nested function, so the incr variable is now accessible to it.

I think this is more intuitive and useful behavior, and generally consistent with the post-709 situation that list/dict/set comprehensions are no longer compiled as nested functions.

But if we want to make this change in behavior, we should consider it explicitly, and consider the backward-compatibility implications. It is possible for this to change the behavior of currently working code:

incr = 1
class C:
    incr = 2
    items = [x+incr for x in range(3)]

In Python 3.11, C.items will be [1, 2, 3] (because incr in the comprehension will bypass the class scope and refer to the global). In current main branch (with PEP 709), it will be [2, 3, 4] instead: the value from the class scope is used.

So there are two questions to consider here:

1. Do we eventually want the behavior where comprehensions in class scopes can access variables in that same class scope?

2. If so, do we need a deprecation path to get there (this seems to me fairly disruptive, and also challenging to implement)? Or can it just be noted prominently in the release notes? And do we want this in 3.12, along with the other changes from PEP 709, or separately in a future release?

Depending on the answers to those questions, there are a number of possible courses of action:

A) Document this behavior change in the release notes for 3.12.

B) Attempt to roll back just this behavior change (for now?), while leaving the rest of PEP 709 as is. (I think this is tricky but probably doable.)

C) Roll back this behavior change (for now?) by just not inlining comprehensions in class scopes at all. This means that other behaviors (tracebacks, what appears in locals()) will be inconsistent for comprehensions in class scopes vs other scopes.

D) Roll back all of PEP 709 for 3.12 and re-consider it with this behavior change for 3.13.

I expect to take this question to the SC and the 3.12 release manager, but it seems useful to gather opinions here first.

[jeanas (Jean Abou Samra)]

As a mere lurker watching this, let me assure you that I would easily have written such code believing it would access the incr variable and have been surprised by the NameError.

In short: the new behavior seems more useful. Whether it’s acceptable from the backwards compatibility POV, I cannot tell. I can certainly tell that if I reviewed code relying on the current behavior on a project that I maintain, I would ask for it to be refactored.

In fact, would it be possible to get the same behavior for genexprs?

[carljm (Carl Meyer)]

@jeanas

In fact, would it be possible to get the same behavior for genexprs?

Yes, the current approach for implementing PEP 695 (and PEP 649) would make it pretty easy to provide this behavior for genexprs as well. (Those PEPs both need to create functions that can access values in enclosing class scopes.)

[carljm (Carl Meyer)]

I think I’ll write a quick AST scanner to look for comprehensions that would be affected by this change and run it over some open source and internal codebases to see if it can turn anything up.

[gpshead (Gregory P. Smith)]

Thanks. If that scanner is meaningfully shareable I’d be happy to run it internally here as well.

My gut feeling is that while this is a change in behavior, the new behavior is more obviously what the reader of code would expect.

So while someone might have written code depending on the name lookup happening in a different namespace, that feels pretty esoteric to me vs design patterns I’m used to seeing. I don’t expect most people to even think of class scopes at all in their daily Python coding life.

If anything that means I lean towards “just prominently document this in What’s New in 3.12”.

[Dutcho]

Fully agree that the new behaviour is the more logical, and that one wouldn’t intentionally depend on the old behaviour.

However, devil’s advocate speaking, one might unintentionally.

Suppose, one (understandably) codes

class C:
    incr = 1
    items = [x+incr for x in range(3)]

and finds it fails with NameError.

Then they fix it by changing to

incr = 1

class C:
    incr = 1
    items = [x+incr for x in range(3)]

and are happy that it works.

Later (change of requirements or whatever), incr needs to become 2, and (by oversight) they only change at global level

incr = 2

class C:
    incr = 1
    items = [x+incr for x in range(3)]

and again are happy that the changed value is used.

Now (potentially much later) they upgrade to Python 3.12, don’t read the release notes (or even remember their code’s internals) and -much to their surprise- they flash back to the old version of their software.

Likely? No.
But then: is this sort of thing going to happen every time we use the Infinite Improbability drive?

[carljm (Carl Meyer)]

Yes, agreed that this is in the category of “unlikely to be intentional, but not impossible.” That’s why I want to collect some actual data on whether I can find occurrences of it in the wild.

[guido (Guido van Rossum)]

I am fine with the slight behavior change (since it is so obviously what the user intended) but I do want to stress (again) that we’re getting into gradually more murky waters where it comes to fine points of scopes and visibility. With PEP 709, we either have to state that this behavior is how all language implementations must work, or we have to introduce “undefined” (or maybe “unspecified”) behavior when it comes to scoping edge cases.

The difference between list/dict/set comprehensions (which are optimized by PEP 709) and generator expressions (which are not) also feels like a step backwards from the ideal situation (in my view) of treating all of them exactly the same as nested functions, for scoping purposes.

I am (-again wondering if we’re perhaps ready for some kind of long-term revision of how scopes actually work, especially class scopes (which seem to be the root cause of many scoping-related problems). Not that I have any bright ideas – we seem to have backed ourselves into a pretty tight corner here.

[brettcannon]

@guido

feels like a step backwards from the ideal situation (in my view) of treating all of them exactly the same as nested functions, for scoping purposes.

Specifically, it feels like going back to Python 2 scoping for list comprehensions.

[carljm (Carl Meyer)]

@brettcannon

Specifically, it feels like going back to Python 2 scoping for list comprehensions.

But with a pretty big difference: in Python 2, comprehension iteration variables stomped on outer-scope variables, and the value from the last comprehension iteration was visible after the comprehension. In PEP 709 they are still isolated.

[Jelle (Jelle Zijlstra)]

I also like the new behavior; I found out about it because I realized the new behavior from PEPs 695 and 709 would provide a way to make all comprehensions (including genexps) work more intuitively in classes. So I decided to try it out on main, and was surprised to find Carl already implemented it!

I feel uneasy though about making this change so late in the release cycle and with little time to think about it.

Here’s a somewhat realistic way this could break code:

class Cls:
    def set(self, key, value):
        ...

    x = [set(vals) for vals in [[1, 2], [3, 4]]]

Here currently set would refer to the builtin, but with the new semantics, it would refer to the method.

Also, this introduces a subtle behavior difference between comprehensions and genexps:

class Cls:
    def set(self, key, value):
        ...

    x = [set(vals) for vals in [[1, 2], [3, 4]]] # "set" is the method
    y = list(set(vals) for vals in [[1, 2], [3, 4]]) # "set" is the builtin

We can fix that by using PEP 695’s __classdict__ functionality in genexps, but that’s not something I’d be comfortable putting into 3.12.

[carljm (Carl Meyer)]

The tool is at GitHub - carljm/compfinder: Tool for finding class-scoped comprehensions whose name resolution might be impacted by PEP 709

All you need is finder.py; it’s a self-contained script with no external dependencies that should run on Python 3.10+ (though I’ve only run it on 3.11 myself.)

Run it like python3 finder.py /some/path /some/other/path/file.py. You can give it any number of paths, which can be either files or directories, and it will scan all of them. When given a directory, it will glob **/*.py inside that directory. If everything it finds is copacetic, it will issue no output. It’ll issue output if a) it can’t open a file with the default text encoding, b) it can’t parse a file to AST, or c) it finds a problematic comprehension (in this case you get line number and variable name.) It might also dump the occasional SyntaxWarning to stderr as it parses files.

(The tool considers a name problematic if it would resolve both pre- and post-709, but resolve to a binding from a different scope. If the name reference would be a NameError pre-709, the tool doesn’t consider making it work to be a problem.)

So far I’ve run it on the standard library, the Django repo, and the IG web server monolith repo, and it has found zero comprehensions whose name resolution would be impacted by this PEP 709 change.

I’m quite sure it doesn’t perfectly emulate every corner of Python scoping rules, but it’s pretty well unit tested and I’m confident it would catch the most likely cases of this, like the ones shown in this thread, as well as more complex nested cases.

Would be very happy to hear results from anyone who runs this on a codebase that you care about!

I’d also welcome code review on the tool itself, particularly if you can construct a case where the tool currently gives a false negative.

[Jelle (Jelle Zijlstra)]

Thanks! I ran it on all the Python code I had lying around (including the Quora codebase) and also found no hits.

[hugovk (Hugo van Kemenade)]

Some orgs

I ran on it on ~260 repos from GitHub orgs such as Jazzband, Pillow, PyPA, and Python, plus some more I have lying around, and found no hits (it’s not the complete set of repos from those orgs, but many).

Top 5k PyPI packages

I also ran it the top 5k PyPI packages (sdists downloaded 29th April), and after adding some more error handling, found a couple of hits:

/Users/hugo/top-pypi/output/aaa-out/seaborn-0.12.2/tests/test_regression.py:
    122 - rs

Which is (source):

    df["c"] = [rs.binomial(1, p_i) for p_i in p]

And:

/Users/hugo/top-pypi/output/aaa-out/datasets-2.12.0/tests/test_warnings.py:
    19 - MetricMock

Which is (source):

        _metrics = [MetricMock(metric_id) for metric_id in ["accuracy", "mse", "precision", "codeparrot/apps_metric"]]

1,000 forks

And also ran on the 129 source repos + 940 fork repos in my own profile, and found a single hit:

/Users/hugo/all-repos/hugovk/output/hugovk/datasets/datasets/americas_nli/americas_nli.py:
    94 - VERSION

Which is (source):

    BUILDER_CONFIGS = [
        AmericasNLIConfig(
            name=lang,
            language=lang,
            version=VERSION,
            description=f"Plain text import of AmericasNLI for the {lang} language",
        )
        for lang in _LANGUAGES
    ]

(Interesting this is also from Huggingface’s Datasets, albeit a year-old fork.)

[Jelle (Jelle Zijlstra)]

Thanks Hugo!

@hugovk

Which is (source):

    df["c"] = [rs.binomial(1, p_i) for p_i in p]

This file has an rs variable in global scope representing a RandomState, and another in the class scope initialized with a different parameter. Under the old semantics, this comprehension would use the global rs; now it uses the one in the class, which is probably what the author intended.

@hugovk

Which is (source):

        _metrics = [MetricMock(metric_id) for metric_id in ["accuracy", "mse", "precision", "codeparrot/apps_metric"]]

This looks like a false positive in the script; I think under either semantics MetricMock would refer to the same class.

@hugovk

Which is (source):

    BUILDER_CONFIGS = [
        AmericasNLIConfig(
            name=lang,
            language=lang,
            version=VERSION,
            description=f"Plain text import of AmericasNLI for the {lang} language",
        )
        for lang in _LANGUAGES
    ]

Here the new semantics mean that VERSION will be resolved from the class scope rather than the module scope, but it doesn’t matter: both refer to the same version.

[notatallshaw (Damian Shaw)]

@Jelle

This file has an rs variable in global scope representing a RandomState, and another in the class scope initialized with a different parameter. Under the old semantics, this comprehension would use the global rs; now it uses the one in the class, which is probably what the author intended.

I agree it’s probably actually the intended behavior but I think it’s worth bringing it to the attention of the project regardless and I have done so: Change in Behavior for Python 3.12 for TestRegressionPlotter · Issue #3365 · mwaskom/seaborn · GitHub

[carljm (Carl Meyer)]

Thanks @hugovk and @Jelle for running the script across much more code, and Jelle for the additional analysis of the results!

I’ve fixed the bug in the script that caused the one false positive from datasets. Besides that, it looks like we’ve so far found one case of actual behavior change, in which it appears likely the new behavior is actually what was intended by the author. Thanks @notatallshaw for notifying the authors about this case!

[gpshead (Gregory P. Smith)]

I ran your compfinder over a random 40% of the .py files in Google’s internal code base and only found a single hit - which upon manual inspection turned out to be a false positive. A lot of third_party open source PyPI or Github or otherwise code was included in that randomly scanned selection.

the file I see as a false positive was something similar to:

def create_class(xxx):
  func_a = xxx + '.value'
  
  class dynamic_class:
     CONSTISH_DICT = {
        'KEY': {
           i: (func_a + f'.{i}.blah') for i in range(16)
        }
     }

So from my perspective: no concerns.

[gpshead (Gregory P. Smith)]

actually looking my log closer, there are a ~3 other hits that are not false positives. But don’t look concerning:

Two of those are a VERSION like thing similar to what @hugovk found.

VERSION = '0.0.1'

class X:
  VERSION = some.module.Version(VERSION)
  CONFIGS = [
    some.module.Config(name=name, version=VERSION)
  ]

Looking at the some.module.Config API… it’s a dataclass with a version field type annotated as VersionOrStr so it’ll accept the Version instance or the original str.

So without digging further, chances are that code works both ways.

The last one is also trivially correct:

PACKAGES = ...

class Thing:
   PACKAGES = PACKAGES
   some_str = 'joiner'.join([x for x in PACKAGES if foo(x)])

So… I’m still not concerned.

[carljm (Carl Meyer)]

This false positive is the one I just pushed a fix for, so it should go away with the most recent version of the script. Not that you need to run it again and confirm that