Skip to content

bpo-36220: STORE_GLOBAL: Support dict subclasses for globals. - #18033

Closed
pfalcon wants to merge 1 commit into
python:masterfrom
pfalcon:store_global-store_name-consistent
Closed

pfalcon wants to merge 1 commit into
python:masterfrom
pfalcon:store_global-store_name-consistent

Conversation

@pfalcon

@pfalcon pfalcon commented Jan 16, 2020 •

Copy link
Copy Markdown

If globals are not concrete dict type, use PyObject_SetItem() on it,
instead of PyDict_SetItem(). This is similar how STORE_NAME deals
with locals already. (And also similar to how some (but not all)
LOAD_* opcodes deal globals/locals),

Given that on the module level, locals and globals are the same,
this change fixed the situation when:

exec("foo = 1", my_globals)

and

exec("global foo; foo = 1", my_globals)

(where "my_globals" is a dict subclass, intended to trace namespace
operations)

lead to different behavior, where in first case, my_globals.setitem()
is called (because STORE_NAME opcode is generated), while in second case,
it's not called (because STORE_GLOBAL is generated).

Note that this change is not related in any way to security sandboxing
(in a sense that this change doesn't make any claims in regard to
improvements to such secuirty sandboxing matters). Instead, it's intended
to make the light-weight instrumentation of Python code for generic
purposes more consistent and predictable (e.g. to collect statistics,
profile code, etc).

https://bugs.python.org/issue36220

If globals are not concrete `dict` type, use PyObject_SetItem() on it,
instead of PyDict_SetItem(). This is similar how STORE_NAME deals
with locals already. (And also similar to how some (but not all)
LOAD_* opcodes deal globals/locals),

Given that on the module level, locals and globals are the same,
this change fixed the situation when:

exec("foo = 1", my_globals)

and

exec("global foo; foo = 1", my_globals)

(where "my_globals" is a dict subclass, intended to trace namespace
operations)

lead to different behavior, where in first case, my_globals.__setitem__()
is called (because STORE_NAME opcode is generated), while in second case,
it's not called (because STORE_GLOBAL is generated).

Note that this change is not related in any way to security sandboxing
(in a sense that this change doesn't make any claims in regard to
improvements to such secuirty sandboxing matters). Instead, it's intended
to make the light-weight instrumentation of Python code for generic
purposes more consistent and predictable (e.g. to collect statistics,
profile code, etc).

@pablogsal pablogsal left a comment •

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

There is no agreement in the issue about this, so I will mark it as DO-NOT-MERGE for now.

For reference, the discussion is happening at:

https://bugs.python.org/issue32615

@bedevere-bot

Copy link
Copy Markdown

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

@pfalcon

pfalcon commented Jan 16, 2020

Copy link
Copy Markdown
Author

Sure, it's just to show that the patch is trivial (and fully mirrors code in STORE_NAME), and to further the argument that it's to achieve consistency both with the behavior of STORE_NAME, and consistency of the behavior of 2 exec calls, where difference is only in "global" declaration of the var used in one exec().

@pablogsal

Copy link
Copy Markdown
Member

Closing as this was rejected in https://bugs.python.org/issue32615

@pablogsal pablogsal closed this Jan 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants