Skip to content

gh-109219: propagate free vars through type param scopes #109377

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

Merged
merged 4 commits into from
Sep 14, 2023

Conversation

carljm
Copy link
Member

@carljm carljm commented Sep 13, 2023

When an annotation scope is inside a class scope, unbound name references in the annotation scope that are bound in the enclosing class scope and would otherwise be free variables are overridden (in analyze_name) to use GLOBAL_EXPLICIT or GLOBAL_IMPLICIT scope instead, since they should behave like name references in the class scope, which see only the class-scope and globals, not other enclosing scopes. E.g. in this example, the reference to T in D's bases (which is in a nested annotation scope, since D is generic) should have GLOBAL_IMPLICIT scope, which results in it using LOAD_FROM_DICT_OR_GLOBALS (where the dict is the class namespace); just as if it were a reference directly in C's scope:

def f():
    T = str
    class C:
        T = int
        class D[U](T):
            pass

But in the case where the same name is also free in a child scope of the annotation scope (replace pass with T in the above example), it must be included in the free vars of the annotation scope, so that the child scope has access to the cell when its closure is constructed.

The symbol flag DEF_CLASS_FREE is intended for these class-scope cases, where a name must have a scope other than FREE in order for references to it to behave correctly, but must still be included in freevars because it is free in a child scope.

So we must add DEF_CLASS_FREE to such names not only in class scopes, but also in annotation scopes enclosed by class scopes.

Previously, we only added DEF_CLASS_FREE if the name was bound in the class scope; otherwise, it would be FREE anyway and DEF_CLASS_FREE would be redundant. To preserve this check in annotation scope cases, we'd have to check if the name is bound in the enclosing class scope, not the annotation scope. But this check is unnecessary, since a redundant DEF_CLASS_FREE on a FREE name is harmless; either way the name will be added to freevars, and this is the only impact of DEF_CLASS_FREE. So it's simpler to just remove the check.

Copy link
Member

@JelleZijlstra JelleZijlstra left a comment

Choose a reason for hiding this comment

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

Thanks! I'm happy the fix may be this simple.

I'll spend some more time later today coming up with more test cases.

Co-authored-by: Jelle Zijlstra <[email protected]>
@carljm carljm marked this pull request as ready for review September 14, 2023 14:08
@carljm carljm changed the title gh-109219: propagate used free vars through type param scopes gh-109219: propagate free vars through type param scopes Sep 14, 2023
@JelleZijlstra
Copy link
Member

The failing test is some concurrent.futures thing on FreeBSD, not relevant to this bug.

@carljm carljm added the needs backport to 3.12 only security fixes label Sep 14, 2023
@carljm carljm merged commit 909adb5 into python:main Sep 14, 2023
@miss-islington
Copy link
Contributor

Thanks @carljm for the PR 🌮🎉.. I'm working now to backport this PR to: 3.12.
🐍🍒⛏🤖 I'm not a witch! I'm not a witch!

@carljm carljm deleted the 695bug branch September 14, 2023 16:20
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Sep 14, 2023
…nGH-109377)

(cherry picked from commit 909adb5)

Co-authored-by: Carl Meyer <[email protected]>
Co-authored-by: Jelle Zijlstra <[email protected]>
@bedevere-app
Copy link

bedevere-app bot commented Sep 14, 2023

GH-109410 is a backport of this pull request to the 3.12 branch.

@bedevere-app bedevere-app bot removed the needs backport to 3.12 only security fixes label Sep 14, 2023
@bedevere-bot
Copy link

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

Hi! The buildbot s390x RHEL7 LTO 3.x has failed when building commit 909adb5.

What do you need to do:

  1. Don't panic.
  2. Check the buildbot page in the devguide if you don't know what the buildbots are or how they work.
  3. Go to the page of the buildbot that failed (https://buildbot.python.org/all/#builders/402/builds/5393) and take a look at the build logs.
  4. Check if the failure is related to this commit (909adb5) or if it is a false positive.
  5. If the failure is related to this commit, please, reflect that on the issue and make a new Pull Request with a fix.

You can take a look at the buildbot page here:

https://buildbot.python.org/all/#builders/402/builds/5393

Failed tests:

  • test.test_asyncio.test_subprocess
  • test_tools

Failed subtests:

  • test_freeze_simple_script - test.test_tools.test_freeze.TestFreeze.test_freeze_simple_script
  • test_subprocess_consistent_callbacks - test.test_asyncio.test_subprocess.SubprocessThreadedWatcherTests.test_subprocess_consistent_callbacks

Summary of the results of the build (if available):

==

Click to see traceback logs
Traceback (most recent call last):
  File "/home/dje/cpython-buildarea/3.x.edelsohn-rhel-z.lto/build/Lib/test/test_tools/test_freeze.py", line 28, in test_freeze_simple_script
    outdir, scriptfile, python = helper.prepare(script, outdir)
                                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/dje/cpython-buildarea/3.x.edelsohn-rhel-z.lto/build/Tools/freeze/test/freeze.py", line 146, in prepare
    copy_source_tree(srcdir, SRCDIR)
  File "/home/dje/cpython-buildarea/3.x.edelsohn-rhel-z.lto/build/Tools/freeze/test/freeze.py", line 95, in copy_source_tree
    shutil.copytree(oldroot, newroot, ignore=ignore_non_src)
  File "/home/dje/cpython-buildarea/3.x.edelsohn-rhel-z.lto/build/Lib/shutil.py", line 588, in copytree
    return _copytree(entries=entries, src=src, dst=dst, symlinks=symlinks,
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/dje/cpython-buildarea/3.x.edelsohn-rhel-z.lto/build/Lib/shutil.py", line 542, in _copytree
    raise Error(errors)
shutil.Error: [('/home/dje/cpython-buildarea/3.x.edelsohn-rhel-z.lto/build/build/test_python_65020æ/@test_65020_tmpæ-tardir/extractall_ctrl/ustar/fifotype', '/tmp/test_python_sn8obp9f/tmpj5vtdkcr/cpython/build/test_python_65020æ/@test_65020_tmpæ-tardir/extractall_ctrl/ustar/fifotype', '`/home/dje/cpython-buildarea/3.x.edelsohn-rhel-z.lto/build/build/test_python_65020æ/@test_65020_tmpæ-tardir/extractall_ctrl/ustar/fifotype` is a named pipe'), ('/home/dje/cpython-buildarea/3.x.edelsohn-rhel-z.lto/build/build/test_python_65020æ/@test_65020_tmpæ-tardir/extractall_none/ustar/fifotype', '/tmp/test_python_sn8obp9f/tmpj5vtdkcr/cpython/build/test_python_65020æ/@test_65020_tmpæ-tardir/extractall_none/ustar/fifotype', '`/home/dje/cpython-buildarea/3.x.edelsohn-rhel-z.lto/build/build/test_python_65020æ/@test_65020_tmpæ-tardir/extractall_none/ustar/fifotype` is a named pipe')]


Traceback (most recent call last):
  File "/home/dje/cpython-buildarea/3.x.edelsohn-rhel-z.lto/build/Lib/test/test_asyncio/test_subprocess.py", line 788, in test_subprocess_consistent_callbacks
    self.loop.run_until_complete(main())
  File "/home/dje/cpython-buildarea/3.x.edelsohn-rhel-z.lto/build/Lib/asyncio/base_events.py", line 664, in run_until_complete
    return future.result()
           ^^^^^^^^^^^^^^^
  File "/home/dje/cpython-buildarea/3.x.edelsohn-rhel-z.lto/build/Lib/test/test_asyncio/test_subprocess.py", line 780, in main
    self.assertEqual(events, [
AssertionError: Lists differ: [('pi[29 chars]t'), 'pipe_connection_lost', ('pipe_data_recei[57 chars]ted'] != [('pi[29 chars]t'), ('pipe_data_received', 2, b'stderr'), 'pi[57 chars]ted']

@bedevere-bot
Copy link

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

Hi! The buildbot AMD64 Debian root 3.x has failed when building commit 909adb5.

What do you need to do:

  1. Don't panic.
  2. Check the buildbot page in the devguide if you don't know what the buildbots are or how they work.
  3. Go to the page of the buildbot that failed (https://buildbot.python.org/all/#builders/345/builds/5838) and take a look at the build logs.
  4. Check if the failure is related to this commit (909adb5) or if it is a false positive.
  5. If the failure is related to this commit, please, reflect that on the issue and make a new Pull Request with a fix.

You can take a look at the buildbot page here:

https://buildbot.python.org/all/#builders/345/builds/5838

Failed tests:

  • test.test_multiprocessing_spawn.test_processes

Summary of the results of the build (if available):

==

Click to see traceback logs
remote: Enumerating objects: 22, done.        
remote: Counting objects:   4% (1/22)        
remote: Counting objects:   9% (2/22)        
remote: Counting objects:  13% (3/22)        
remote: Counting objects:  18% (4/22)        
remote: Counting objects:  22% (5/22)        
remote: Counting objects:  27% (6/22)        
remote: Counting objects:  31% (7/22)        
remote: Counting objects:  36% (8/22)        
remote: Counting objects:  40% (9/22)        
remote: Counting objects:  45% (10/22)        
remote: Counting objects:  50% (11/22)        
remote: Counting objects:  54% (12/22)        
remote: Counting objects:  59% (13/22)        
remote: Counting objects:  63% (14/22)        
remote: Counting objects:  68% (15/22)        
remote: Counting objects:  72% (16/22)        
remote: Counting objects:  77% (17/22)        
remote: Counting objects:  81% (18/22)        
remote: Counting objects:  86% (19/22)        
remote: Counting objects:  90% (20/22)        
remote: Counting objects:  95% (21/22)        
remote: Counting objects: 100% (22/22)        
remote: Counting objects: 100% (22/22), done.        
remote: Compressing objects:   8% (1/12)        
remote: Compressing objects:  16% (2/12)        
remote: Compressing objects:  25% (3/12)        
remote: Compressing objects:  33% (4/12)        
remote: Compressing objects:  41% (5/12)        
remote: Compressing objects:  50% (6/12)        
remote: Compressing objects:  58% (7/12)        
remote: Compressing objects:  66% (8/12)        
remote: Compressing objects:  75% (9/12)        
remote: Compressing objects:  83% (10/12)        
remote: Compressing objects:  91% (11/12)        
remote: Compressing objects: 100% (12/12)        
remote: Compressing objects: 100% (12/12), done.        
remote: Total 12 (delta 10), reused 1 (delta 0), pack-reused 0        
From https://github.com/python/cpython
 * branch                  main       -> FETCH_HEAD
Note: switching to '909adb5092c0ae9426814742d97932204b211cfb'.

You are in 'detached HEAD' state. You can look around, make experimental
changes and commit them, and you can discard any commits you make in this
state without impacting any branches by switching back to a branch.

If you want to create a new branch to retain commits you create, you may
do so (now or later) by using -c with the switch command. Example:

  git switch -c <new-branch-name>

Or undo this operation with:

  git switch -

Turn off this advice by setting config variable advice.detachedHead to false

HEAD is now at 909adb5092 gh-109219: propagate free vars through type param scopes (#109377)
Switched to and reset branch 'main'

configure: WARNING: pkg-config is missing. Some dependencies may not be detected correctly.

Kill <WorkerThread #1 running test=test_tools pid=1565972 time=3 min 34 sec> process group
Warning -- Failed to join <WorkerThread #1 running test=test_tools pid=1565972 time=4 min 4 sec> in 30.2 sec
make: *** [Makefile:2029: buildbottest] Error 5

@bedevere-bot
Copy link

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

Hi! The buildbot s390x RHEL8 LTO + PGO 3.x has failed when building commit 909adb5.

What do you need to do:

  1. Don't panic.
  2. Check the buildbot page in the devguide if you don't know what the buildbots are or how they work.
  3. Go to the page of the buildbot that failed (https://buildbot.python.org/all/#builders/442/builds/4975) and take a look at the build logs.
  4. Check if the failure is related to this commit (909adb5) or if it is a false positive.
  5. If the failure is related to this commit, please, reflect that on the issue and make a new Pull Request with a fix.

You can take a look at the buildbot page here:

https://buildbot.python.org/all/#builders/442/builds/4975

Failed tests:

  • test.test_asyncio.test_subprocess

Failed subtests:

  • test_subprocess_consistent_callbacks - test.test_asyncio.test_subprocess.SubprocessThreadedWatcherTests.test_subprocess_consistent_callbacks
  • test_subprocess_consistent_callbacks - test.test_asyncio.test_subprocess.SubprocessFastWatcherTests.test_subprocess_consistent_callbacks

Summary of the results of the build (if available):

==

Click to see traceback logs
Traceback (most recent call last):
  File "/home/dje/cpython-buildarea/3.x.edelsohn-rhel8-z.lto-pgo/build/Lib/test/test_asyncio/test_subprocess.py", line 788, in test_subprocess_consistent_callbacks
    self.loop.run_until_complete(main())
  File "/home/dje/cpython-buildarea/3.x.edelsohn-rhel8-z.lto-pgo/build/Lib/asyncio/base_events.py", line 664, in run_until_complete
    return future.result()
           ^^^^^^^^^^^^^^^
  File "/home/dje/cpython-buildarea/3.x.edelsohn-rhel8-z.lto-pgo/build/Lib/test/test_asyncio/test_subprocess.py", line 780, in main
    self.assertEqual(events, [
AssertionError: Lists differ: [('pi[29 chars]t'), 'process_exited'] != [('pi[29 chars]t'), ('pipe_data_received', 2, b'stderr'), 'pi[57 chars]ted']


Traceback (most recent call last):
  File "/home/dje/cpython-buildarea/3.x.edelsohn-rhel8-z.lto-pgo/build/Lib/test/test_asyncio/test_subprocess.py", line 788, in test_subprocess_consistent_callbacks
    self.loop.run_until_complete(main())
  File "/home/dje/cpython-buildarea/3.x.edelsohn-rhel8-z.lto-pgo/build/Lib/asyncio/base_events.py", line 664, in run_until_complete
    return future.result()
           ^^^^^^^^^^^^^^^
  File "/home/dje/cpython-buildarea/3.x.edelsohn-rhel8-z.lto-pgo/build/Lib/test/test_asyncio/test_subprocess.py", line 780, in main
    self.assertEqual(events, [
AssertionError: Lists differ: ['process_exited', ('pipe_data_received', 1, b'stdout')] != [('pipe_data_received', 1, b'stdout'), ('p[95 chars]ted']

@bedevere-bot
Copy link

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

Hi! The buildbot PPC64LE RHEL8 LTO 3.x has failed when building commit 909adb5.

What do you need to do:

  1. Don't panic.
  2. Check the buildbot page in the devguide if you don't know what the buildbots are or how they work.
  3. Go to the page of the buildbot that failed (https://buildbot.python.org/all/#builders/361/builds/4053) and take a look at the build logs.
  4. Check if the failure is related to this commit (909adb5) or if it is a false positive.
  5. If the failure is related to this commit, please, reflect that on the issue and make a new Pull Request with a fix.

You can take a look at the buildbot page here:

https://buildbot.python.org/all/#builders/361/builds/4053

Failed tests:

  • test.test_asyncio.test_unix_events

Failed subtests:

  • test_fork_signal_handling - test.test_asyncio.test_unix_events.TestFork.test_fork_signal_handling

Summary of the results of the build (if available):

==

Click to see traceback logs
Traceback (most recent call last):
  File "/home/buildbot/buildarea/3.x.cstratak-RHEL8-ppc64le.lto/build/Lib/unittest/async_case.py", line 90, in _callTestMethod
    if self._callMaybeAsync(method) is not None:
       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/buildbot/buildarea/3.x.cstratak-RHEL8-ppc64le.lto/build/Lib/unittest/async_case.py", line 117, in _callMaybeAsync
    return self._asyncioTestContext.run(func, *args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/buildbot/buildarea/3.x.cstratak-RHEL8-ppc64le.lto/build/Lib/test/support/hashlib_helper.py", line 49, in wrapper
    return func_or_class(*args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/buildbot/buildarea/3.x.cstratak-RHEL8-ppc64le.lto/build/Lib/test/test_asyncio/test_unix_events.py", line 1937, in test_fork_signal_handling
    self.assertTrue(child_handled.is_set())
AssertionError: False is not true

@JelleZijlstra
Copy link
Member

You're breaking a lot of buildbots today Carl :)

@carljm
Copy link
Member Author

carljm commented Sep 14, 2023

It's especially impressive how many I managed to break by landing a change in code under #if 0!

Yhg1s pushed a commit that referenced this pull request Sep 14, 2023
…09377) (#109410)

gh-109219: propagate free vars through type param scopes (GH-109377)
(cherry picked from commit 909adb5)

Co-authored-by: Carl Meyer <[email protected]>
Co-authored-by: Jelle Zijlstra <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants