gh-70764: inspect.getclosurevars now identifies global variables with LOAD_GLOBAL

[blhsing]

inspect.getclosurevars now identifies global variables with LOAD_GLOBAL

Currently inspect.getclosurevars identifies a global variable by testing if a name in co_names exists in the function's global namespace, but this approach can result in incorrectly classifying an attribute name as a global variable when the name exists both as an attriute and as a global variable.

This PR fixes the issue by using the LOAD_GLOBAL and LOAD_ATTR instructions instead to identify global names and unbound names, respectively.

• Issue: inspect.getclosurevars returns incorrect variable when using class member with the same name as other variable #70764

[carljm]

Thanks for the contribution!

This could make getclosurevars much slower if run on a very large function, because bytecode size can scale much faster than the set of names used.

In particular, if a large function has lots of references to builtins (even the same builtin used many times) we will repeatedly raise and catch an exception, possibly many times for the same builtin name. This repeated exception raising could at least be mitigated by keeping a set of seen global names, and not re-looking-up an already-seen name (or just avoiding re-looking-up names already present in builtin_vars).

That said, I'm not sure I see any other implementation strategy for fixing this function; we just don't preserve enough context from compilation on the code object, except in the bytecode itself.

Would like to see if any other reviewer has better ideas here, before merging this.

[blhsing]

This could make getclosurevars much slower if run on a very large function, because bytecode size can scale much faster than the set of names used.

In particular, if a large function has lots of references to builtins (even the same builtin used many times) we will repeatedly raise and catch an exception, possibly many times for the same builtin name. This repeated exception raising could at least be mitigated by keeping a set of seen global names, and not re-looking-up an already-seen name (or just avoiding re-looking-up names already present in builtin_vars).

That said, I'm not sure I see any other implementation strategy for fixing this function; we just don't preserve enough context from compilation on the code object, except in the bytecode itself.

Would like to see if any other reviewer has better ideas here, before merging this.

Thanks for the review! I totally agree with your assessments. I've updated the approach so that it collects all global names in a set first before looking them up in global and builtin namespaces. And yeah unfortunately I don't see a way not to scan through all the bytecodes if we are to aim for accuracy.

[carljm]

This will still be a slowdown, but not raising a lot of exceptions should help a lot, and inspect methods are often slow; correctness seems more important. And no better idea for how to make this method correct has surfaced; I don't think there is one. Going to go ahead and rebase this to make sure no other changes have impacted it in the last months, and then merge if signal looks good. Thanks again for the contribution!

[miss-islington-app]

Thanks @blhsing for the PR, and @carljm for merging it 🌮🎉.. I'm working now to backport this PR to: 3.13.
🐍🍒⛏🤖

[miss-islington-app]

Thanks @blhsing for the PR, and @carljm for merging it 🌮🎉.. I'm working now to backport this PR to: 3.12.
🐍🍒⛏🤖

[bedevere-app]

GH-126459 is a backport of this pull request to the 3.13 branch.

[bedevere-app]

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