-
-
Notifications
You must be signed in to change notification settings - Fork 32.4k
gh-130907: Treat all module-level annotations as conditional #131550
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
Changes from 7 commits
ace6bc5
60c633a
27004f8
ef4a375
d1e1761
f550d65
c2ecce5
137cea7
c76d0b5
ee8e857
719a2e5
aaa534a
862e5bf
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -383,9 +383,10 @@ def test_var_annot_simple_exec(self): | |
gns = {}; lns = {} | ||
exec("'docstring'\n" | ||
"x: int = 5\n", gns, lns) | ||
self.assertNotIn('__annotate__', gns) | ||
|
||
gns.update(lns) # __annotate__ looks at globals | ||
self.assertEqual(lns["__annotate__"](annotationlib.Format.VALUE), {'x': int}) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This test started failing because |
||
with self.assertRaises(KeyError): | ||
gns['__annotate__'] | ||
|
||
def test_var_annot_rhs(self): | ||
ns = {} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
from . import a |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
v1: int | ||
|
||
from . import b | ||
|
||
v2: int |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
from . import a | ||
|
||
annos = a.__annotations__ |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,2 @@ | ||
Raise an error if the ``__annotations__`` attribute is accessed on a module | ||
that has not finished executing. |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -792,6 +792,17 @@ codegen_process_deferred_annotations(compiler *c, location loc) | |
return SUCCESS; | ||
} | ||
|
||
instr_sequence *old_instr_seq = INSTR_SEQUENCE(c); | ||
instr_sequence *nested_instr_seq = NULL; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there a reason why we can't reorder the code in this function so that we first emit the instructions for annotations and then for the code (so we don't need to do this injection)? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah I see, it's stuff from outside this function. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, I considered an alternative where we generate the code first, then later edit the LOAD_CONST instruction to the correct code object we generate later, but that seemed like it'd be worse. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wonder if we could merge the annotation code into the main code in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Or rather, There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done! Wasn't too bad after all. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Cool. So now we shouldn't need There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We still need it right now because we need to make the extra instrseq that we can later stitch in the right place. The only other way to set the compiler's instrseq appears to be There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see. I think I would have moved this into the compiler. Instead of adding There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done |
||
int scope_type = SCOPE_TYPE(c); | ||
if (scope_type == COMPILE_SCOPE_MODULE) { | ||
nested_instr_seq = (instr_sequence *)_PyInstructionSequence_New(); | ||
if (nested_instr_seq == NULL) { | ||
goto error; | ||
} | ||
_PyCompile_SetInstrSequence(c, nested_instr_seq); | ||
} | ||
|
||
// It's possible that ste_annotations_block is set but | ||
// u_deferred_annotations is not, because the former is still | ||
// set if there are only non-simple annotations (i.e., annotations | ||
|
@@ -800,7 +811,6 @@ codegen_process_deferred_annotations(compiler *c, location loc) | |
PySTEntryObject *ste = SYMTABLE_ENTRY(c); | ||
assert(ste->ste_annotation_block != NULL); | ||
void *key = (void *)((uintptr_t)ste->ste_id + 1); | ||
int scope_type = SCOPE_TYPE(c); | ||
if (codegen_setup_annotations_scope(c, loc, key, | ||
ste->ste_annotation_block->ste_name) < 0) { | ||
goto error; | ||
|
@@ -817,8 +827,19 @@ codegen_process_deferred_annotations(compiler *c, location loc) | |
RETURN_IF_ERROR(codegen_leave_annotations_scope(c, loc)); | ||
RETURN_IF_ERROR(codegen_nameop(c, loc, &_Py_ID(__annotate__), Store)); | ||
|
||
if (nested_instr_seq != NULL) { | ||
RETURN_IF_ERROR( | ||
_PyInstructionSequence_PrependSequence(old_instr_seq, 1, nested_instr_seq)); | ||
_PyCompile_SetInstrSequence(c, old_instr_seq); | ||
PyInstructionSequence_Fini(nested_instr_seq); | ||
} | ||
|
||
return SUCCESS; | ||
error: | ||
if (nested_instr_seq != NULL) { | ||
PyInstructionSequence_Fini(nested_instr_seq); | ||
_PyCompile_SetInstrSequence(c, old_instr_seq); | ||
} | ||
Py_XDECREF(deferred_anno); | ||
Py_XDECREF(conditional_annotation_indices); | ||
return ERROR; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Re naming:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll rename the function to "InjectSequence" and the parameter to "injected". Thanks for the review!