GeezerVison

🤖 New build scheduled with the buildbot fleet by @ericsnowcurrently for commit 4acf0254591d1e6bcc526cb7b111faaeaf0dd1a0 🤖

If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again.

🤖 New build scheduled with the buildbot fleet by @ericsnowcurrently for commit 1d2053878ed6a02ebc5ad727a095255bad29b679 🤖

If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again.

FYI, the "test-with-buildbots" run shows failures on two of the Windows 10 buildbots. (The refleaks buildbot issue is unrelated.) The failures are consistent in test_clinic, test_peg_generator, and test_tools, and are all founded in _PyEval_MakeFrameVector():

• (test.test_peg_generator) UnboundLocalError: cannot access local variable 'method' where it is not associated with a value
• (test_tools) NameError: cannot access free variable 'var_name' where it is not associated with a value in enclosing scope
• (test_clinic) NameError: cannot access free variable 'kwargs' where it is not associated with a value in enclosing scope

These definitely seem related to this PR so I'm going to investigate. (I'll debug the issue as soon as I have VS and the cpython repo set up on my Windows laptop.)

So far I haven't been able to reproduce the failures showing up on those two buildbots. I was able to build on Windows and run the tests but got no failures.

So far I haven't been able to reproduce the failures showing up on those two buildbots. I was able to build on Windows and run the tests but got no failures.

Same here.

Hmm, I'm pretty sure the problem is that I forgot to bump the .pyc magic number, though I would have expected more failures. Perhaps buildbots clear all the cached .pyc files except in the Tools directory?

Yep, it looks like on Windows we only delete .pyc files under Lib/ (before running the tests). See PCbuild/rt.bat.

Perhaps we should not delete any pic files, just to expose this kind of bug? At least in some test runs? Else this might have gone through...

Yeah, I think you're right.

🤖 New build scheduled with the buildbot fleet by @ericsnowcurrently for commit 92f62e7b85088b6250adbaf70a0e3e56e4c1f0c3 🤖

If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again.

Those two buildbots are happy with this PR now. 😄

I've just had a play with this:

>>> def test(arg=0, escapes_arg=1):
...     local = 2
...     escapes = 3
...     another_local = 4
...     def inner():
...         return escapes, escapes_arg
... 
>>> dis.dis(test)

              0 MAKE_CELL                5 (escapes)
              2 MAKE_CELL                1 (escapes_arg)

  2           4 LOAD_CONST               1 (2)
              6 STORE_FAST               2 (local)

  3           8 LOAD_CONST               2 (3)
             10 STORE_DEREF              5 (escapes)

  4          12 LOAD_CONST               3 (4)
             14 STORE_FAST               3 (another_local)

  5          16 LOAD_CLOSURE             ...

It looks like variables are still being shuffled around. escapes has index 5.
Why not leave the locals in their "natural" order (apart from free variables)?

I would expect this:

              0 MAKE_CELL                1 (escapes_arg)
              3 MAKE_CELL                3 (escapes)

  2           4 LOAD_CONST               1 (2)
              6 STORE_FAST               2 (local)

  3           8 LOAD_CONST               2 (3)
             10 STORE_DEREF              3 (escapes)

  4          12 LOAD_CONST               3 (4)
             14 STORE_FAST               4 (another_local)

  5          16 LOAD_CLOSURE             ...

What is the point of _varname_from_oparg?
It seems synonymous with co_varnames:

>>> for i, name in enumerate(test.__code__.co_varnames):
...      assert name == test.__code__._varname_from_oparg(i)

What would be more useful is a method to get the kind of the variable.

It looks like variables are still being shuffled around.

Yeah, I noticed that too. Currently in compile.c (in insert_prefix_instructions()) we iterate over the compiler's mapping of cellvar names (mapped to indices). So that's the order we're getting. Putting them in index order would take an extra O(n) step during compilation.

What is the point of _varname_from_oparg?

I was exposing _co_localsplusnames before since that is what I needed for dis. I changed it to that function when you brought up not exposing the implementation details.

It seems synonymous with co_varnames:

Mostly, I don't care about the specific name. I was trying to avoid something like localsplusname_from_oparg since that wasn't much better than _co_localsplusnames. We can call it whatever we want though. 😄

What would be more useful is a method to get the kind of the variable.

That's what @gvanrossum was talking about yesterday. We could add _co_varkind_from_oparg() or combine the two into _co_varinfo_from_oparg() (which would return a 2-tuple of name/kind). We could also just expose the (private) attributes and get rid of the abstraction. It doesn't really matter to me.

FYI, neither of those two issues relates to this PR. 😄 They were introduced in my previous PRs.

I’m waiting for,this to land. Mark, are there any specific things you need to see changed?

I’m waiting for,this to land. Mark, are there any specific things you need to see changed?

Yes, we shouldn't be shuffling around the variables. The order of the variables in co_varnames and in the locals (excluding the free variables) should be the same.

In 3.10 all cells came after all other locals. Inefficient, but predictable.
in 3.11 cells and all other locals should be in the "natural" order (the same order as co_varnames) which is both efficient and predictable. We might need the order to be predictable when specializing calls.

Currently they are in no clear order, which is likely to be error prone.

It also suggests to me that something is wrong in the compiler; co_varnames and the fastlocals should have a common source and be in the same order.

Actually, it looks like cell variables only appear in co_varnames in 3.10 if the cell is an argument, and this PR doesn't change that.
So, the co_varnames and fast locals are consistent. I guess it doesn't matter too much where the cell variables occur in the list,
although I can't help thinking it would be simpler not to re-order them.

co_varnames should include all names, but I guess that could be done in another PR.

We want to treat all cells the same, regardless of whether they are a parameter or not.
So, the following should be true:
co_.co_nlocals == len(co.co_varnames)
(set(co.co_cellvars) & set(co.co_varnames)) == set(co.co_cellvars))
The name of the fastlocal variable at index i should be co.co_varnames[i]

@markshannon, I see what you mean now. I'm not opposed to putting cells in their "natural" position. I looked at doing that when I first started working on all this. However, at the time I didn't see any easy way to do so. It would require changes in the compiler and symtable code that I wasn't sure were worth making at the time. I do agree that we can look at doing so later.

FYI, I plan on merging this first thing in the morning (my morning anyway).

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

(I should just add that I had always assumed that a "regular" variable could never be a cell. But the existence of the __closure__ attribute on function objects defies that. This makes everything more complex in this PR.)

🤖 New build scheduled with the buildbot fleet by @ericsnowcurrently for commit bdb12a7 🤖

If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again.