bpo-32176: Set CO_NOFREE in the code object constructor#4675
Conversation
Previously, CO_NOFREE was set in the compiler, which meant it could end up being set incorrectly when code objects were created directly. Setting it in the constructor based on freevars and cellvars ensures it is always accurate, regardless of how the code object is defined.
|
|
||
| # Ensure the implicit super() call actually works | ||
| obj = List([1,2,3]) | ||
| self.assertEqual(obj[0], 1) |
There was a problem hiding this comment.
Does it print to stdout? stdout shouldn't be polluted by tests in non-verbose mode.
How do you know that the injected method actually works instead of the original list method? I suggest to transform the value in __getitem__. For example by calling str().
There was a problem hiding this comment.
Done - replaced the print() with the use of an f-string as the getitem result.
| self.assertFalse(function.__code__.co_flags & inspect.CO_NOFREE) | ||
|
|
||
| # Ensure the implicit super() call actually works | ||
| obj = List([1,2,3]) |
There was a problem hiding this comment.
Add spaces after commas.
| setattr(cls, name, FunctionType(code, globals(), name, defaults, closure)) | ||
|
|
||
| class List(list): | ||
| def append(self, elem): |
There was a problem hiding this comment.
Are these methods related to the test?
There was a problem hiding this comment.
Just copied from the original reproducer in the bug report - replaced with pass.
| def new_code(c_or_f): | ||
| '''A new code object with a __class__ cell added to freevars''' | ||
| c = c_or_f.__code__ if isinstance(c_or_f, FunctionType) else c_or_f | ||
| return CodeType( |
There was a problem hiding this comment.
Seems there are no direct tests for CodeType. I think we need to add them (in different issue).
There was a problem hiding this comment.
Yeah, that would probably be a good idea. I did a brief review for other unhandled edge cases in direct calls to the constructor and didn't see any, but I could easily have missed something.
|
|
||
| def new_code(c_or_f): | ||
| '''A new code object with a __class__ cell added to freevars''' | ||
| c = c_or_f.__code__ if isinstance(c_or_f, FunctionType) else c_or_f |
There was a problem hiding this comment.
In this test new_code() is called only with the code object.
There was a problem hiding this comment.
I refactored both this and add_foreign_method to only handle the cases needed for the test (the original more general versions came from the reported reproducer for the issue)
| self.assertIs(class_ref, List) | ||
|
|
||
| # Ensure the code correctly indicates it accesses a free variable | ||
| self.assertFalse(function.__code__.co_flags & inspect.CO_NOFREE) |
There was a problem hiding this comment.
Add the second argument hex(function.__code__.co_flags).
- use a non-dunder name for the injected function - remove some code branches not actually needed for the test - transform the value to ensure the injected method is running - don't print anything as a side effect of running the test - print the code flags as hex if the test assertion fails
ncoghlan
left a comment
There was a problem hiding this comment.
Thanks for the review! All suggestions accepted, and I spotted a few other opportunities for cleanups while I was editing.
|
|
||
| # Ensure the implicit super() call actually works | ||
| obj = List([1,2,3]) | ||
| self.assertEqual(obj[0], 1) |
There was a problem hiding this comment.
Done - replaced the print() with the use of an f-string as the getitem result.
| self.assertFalse(function.__code__.co_flags & inspect.CO_NOFREE) | ||
|
|
||
| # Ensure the implicit super() call actually works | ||
| obj = List([1,2,3]) |
| self.assertIs(class_ref, List) | ||
|
|
||
| # Ensure the code correctly indicates it accesses a free variable | ||
| self.assertFalse(function.__code__.co_flags & inspect.CO_NOFREE) |
| setattr(cls, name, FunctionType(code, globals(), name, defaults, closure)) | ||
|
|
||
| class List(list): | ||
| def append(self, elem): |
There was a problem hiding this comment.
Just copied from the original reproducer in the bug report - replaced with pass.
| def new_code(c_or_f): | ||
| '''A new code object with a __class__ cell added to freevars''' | ||
| c = c_or_f.__code__ if isinstance(c_or_f, FunctionType) else c_or_f | ||
| return CodeType( |
There was a problem hiding this comment.
Yeah, that would probably be a good idea. I did a brief review for other unhandled edge cases in direct calls to the constructor and didn't see any, but I could easily have missed something.
|
|
||
| def new_code(c_or_f): | ||
| '''A new code object with a __class__ cell added to freevars''' | ||
| c = c_or_f.__code__ if isinstance(c_or_f, FunctionType) else c_or_f |
There was a problem hiding this comment.
I refactored both this and add_foreign_method to only handle the cases needed for the test (the original more general versions came from the reported reproducer for the issue)
|
Thanks @ncoghlan for the PR 🌮🎉.. I'm working now to backport this PR to: 3.6. |
|
Sorry, @ncoghlan, I could not cleanly backport this to |
|
@serhiy-storchaka Thanks for the review! |
…GH-4675) Previously, CO_NOFREE was set in the compiler, which meant it could end up being set incorrectly when code objects were created directly. Setting it in the constructor based on freevars and cellvars ensures it is always accurate, regardless of how the code object is defined.. (cherry picked from commit 078f181)
|
GH-4684 is a backport of this pull request to the 3.6 branch. |
Previously, CO_NOFREE was set in the compiler, which meant
it could end up being set incorrectly when code objects
were created directly. Setting it in the constructor based
on freevars and cellvars ensures it is always accurate,
regardless of how the code object is defined.
https://bugs.python.org/issue32176