8000 gh-130907: Treat all module-level annotations as conditional by JelleZijlstra · Pull Request #131550 · python/cpython · GitHub
[go: up one dir, main page]

Skip to content

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

Merged
merged 13 commits into from
Apr 28, 2025
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions Include/internal/pycore_compile.h
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,8 @@ int _PyCompile_EnterScope(struct _PyCompiler *c, identifier name, int scope_type
void _PyCompile_ExitScope(struct _PyCompiler *c);
Py_ssize_t _PyCompile_AddConst(struct _PyCompiler *c, PyObject *o);
_PyInstructionSequence *_PyCompile_InstrSequence(struct _PyCompiler *c);
void _PyCompile_SetInstrSequence(struct _PyCompiler *c,
_PyInstructionSequence *instr_sequence);
int _PyCompile_FutureFeatures(struct _PyCompiler *c);
void _PyCompile_DeferredAnnotations(
struct _PyCompiler *c, PyObject **deferred_annotations,
Expand Down
2 changes: 2 additions & 0 deletions Include/internal/pycore_instruction_sequence.h
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,8 @@ _PyJumpTargetLabel _PyInstructionSequence_NewLabel(_PyInstructionSequence *seq);
int _PyInstructionSequence_ApplyLabelMap(_PyInstructionSequence *seq);
int _PyInstructionSequence_InsertInstruction(_PyInstructionSequence *seq, int pos,
int opcode, int oparg, _Py_SourceLocation loc);
int _PyInstructionSequence_PrependSequence(_PyInstructionSequence *seq, int pos,
_PyInstructionSequence *nested);
Copy link
Member

Choose a reason for hiding this comment

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

Re naming:

  • if you give a pos then it's not prepend but something like inject.
  • we already use "nested sequence" for something else in this space.

Copy link
Member Author

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!

int _PyInstructionSequence_AddNested(_PyInstructionSequence *seq, _PyInstructionSequence *nested);
void PyInstructionSequence_Fini(_PyInstructionSequence *seq);

Expand Down
42 changes: 27 additions & 15 deletions Lib/test/test_dis.py
Original file line number Diff line number Diff line change
Exp 8000 and Up @@ -382,24 +382,36 @@ def wrap_func_w_kwargs():
# leading newline is for a reason (tests lineno)

dis_annot_stmt_str = """\
0 RESUME 0
-- MAKE_CELL 0 (__conditional_annotations__)

2 LOAD_SMALL_INT 1
STORE_NAME 0 (x)
0 RESUME 0

4 LOAD_SMALL_INT 1
LOAD_NAME 1 (lst)
LOAD_NAME 2 (fun)
PUSH_NULL
LOAD_SMALL_INT 0
CALL 1
STORE_SUBSCR
2 LOAD_CONST 1 (<code object __annotate__ at 0x..., file "<dis>", line 2>)
MAKE_FUNCTION
STORE_NAME 4 (__annotate__)
BUILD_SET 0
STORE_NAME 0 (__conditional_annotations__)
LOAD_SMALL_INT 1
STORE_NAME 1 (x)
LOAD_NAME 0 (__conditional_annotations__)
LOAD_SMALL_INT 0
SET_ADD 1
POP_TOP

2 LOAD_CONST 1 (<code object __annotate__ at 0x..., file "<dis>", line 2>)
MAKE_FUNCTION
STORE_NAME 3 (__annotate__)
LOAD_CONST 2 (None)
RETURN_VALUE
3 LOAD_NAME 0 (__conditional_annotations__)
LOAD_SMALL_INT 1
SET_ADD 1
POP_TOP

4 LOAD_SMALL_INT 1
LOAD_NAME 2 (lst)
LOAD_NAME 3 (fun)
PUSH_NULL
LOAD_SMALL_INT 0
CALL 1
STORE_SUBSCR
LOAD_CONST 2 (None)
RETURN_VALUE
"""

fn_with_annotate_str = """
Expand Down
5 changes: 3 additions & 2 deletions Lib/test/test_grammar.py
Original file line number Diff line number Diff line change
Expand Up @@ -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})
Copy link
Member Author

Choose a reason for hiding this comment

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

This test started failing because __annotate__ now always accesses __conditional_annotations__ from the globals, and the separate globals and locals namespaces break that. This would have already failed if the annotations referred to any name defined in the module (e.g. type x = int; y: x), so I don't think that's a problem.

with self.assertRaises(KeyError):
gns['__annotate__']

def test_var_annot_rhs(self):
ns = {}
Expand Down
10 changes: 9 additions & 1 deletion Lib/test/test_type_annotations.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
import textwrap
import types
import unittest
from test.support import run_code, check_syntax_error, cpython_only
from test.support import run_code, check_syntax_error, import_helper, cpython_only


class TypeAnnotationTests(unittest.TestCase):
Expand Down Expand Up @@ -109,6 +109,14 @@ class D(metaclass=C):
del D.__annotations__
self.assertEqual(D.__annotations__, {})

def test_partially_executed_module(self):
partialexe = import_helper.import_fresh_module("test.typinganndata.partialexecution")
self.assertEqual(
partialexe.a.__annotations__,
{"v1": int, "v2": int},
)
self.assertEqual(partialexe.b.annos, {"v1": int})

@cpython_only
def test_no_cell(self):
# gh-130924: Test that uses of annotations in local scopes do not
Expand Down
1 change: 1 addition & 0 deletions Lib/test/typinganndata/partialexecution/__init__.py
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
from . import a
5 changes: 5 additions & 0 deletions Lib/test/typinganndata/partialexecution/a.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
v1: int

from . import b

v2: int
3 changes: 3 additions & 0 deletions Lib/test/typinganndata/partialexecution/b.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
from . import a

annos = a.__annotations__
1 change: 1 addition & 0 deletions Makefile.pre.in
Original file line number Diff line number Diff line change
Expand Up @@ -2666,6 +2666,7 @@ TESTSUBDIRS= idlelib/idle_test \
test/translationdata/getopt \
test/translationdata/optparse \
test/typinganndata \
test/typinganndata/partialexecution \
test/wheeldata \
test/xmltestdata \
test/xmltestdata/c14n-20 \
Expand Down
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.
22 changes: 21 additions & 1 deletion Objects/moduleobject.c
Original file line number Diff line number Diff line change
Expand Up @@ -1246,6 +1246,25 @@ module_get_annotations(PyObject *self, void *Py_UNUSED(ignored))

PyObject *annotations;
if (PyDict_GetItemRef(dict, &_Py_ID(__annotations__), &annotations) == 0) {
PyObject *spec;
if (PyDict_GetItemRef(m->md_dict, &_Py_ID(__spec__), &spec) < 0) {
Py_DECREF(dict);
return NULL;
}
bool is_initializing = false;
if (spec != NULL) {
int rc = _PyModuleSpec_IsInitializing(spec);
if (rc < 0) {
Py_DECREF(spec);
Py_DECREF(dict);
return NULL;
}
Py_DECREF(spec);
if (rc) {
is_initializing = true;
}
}

PyObject *annotate;
int annotate_result = PyDict_GetItemRef(dict, &_Py_ID(__annotate__), &annotate);
if (annotate_result < 0) {
Expand Down Expand Up @@ -1273,7 +1292,8 @@ module_get_annotations(PyObject *self, void *Py_UNUSED(ignored))
annotations = PyDict_New();
}
Py_XDECREF(annotate);
if (annotations) {
// Do not cache annotations if the module is still initializing
if (annotations && !is_initializing) {
int result = PyDict_SetItem(
dict, &_Py_ID(__annotations__), annotations);
if (result) {
Expand Down
23 changes: 22 additions & 1 deletion Python/codegen.c
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Copy link
Member

Choose a reason for hiding this comment

The 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)?

Copy link
Member

Choose a reason for hiding this comment

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

Ah I see, it's stuff from outside this function.

Copy link
Member Author

Choose a reason for hiding this comment

The 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.

Copy link
Member

Choose a reason for hiding this comment

The 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 _PyCfg_OptimizedCfgToInstructionSequence.

Copy link
Member

Choose a reason for hiding this comment

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

Or rather, _PyCfg_FromInstructionSequence.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done! Wasn't too bad after all.

Copy link
Member

Choose a reason for hiding this comment

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

Cool. So now we shouldn't need _PyCompile_SetInstrSequence and nested_instr_seq, right?

Copy link
Member Author

Choose a reason for hiding this comment

The 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 _PyCompile_EnterScope, but we don't want to enter a new scope here; this code is in the parent scope.

Copy link
Member

Choose a reason for hiding this comment

The 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 _PyCompile_SetInstrSequence, add functions to "begin annotations block" and "end annotations block" and let the compiler manage its data structures by itself.

Copy link
Member Author

Choose a reason for hiding this comment

The 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
Expand All @@ -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;
Expand All @@ -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;
Expand Down
8 changes: 7 additions & 1 deletion Python/compile.c
Original file line number Diff line number Diff line change
Expand Up @@ -1154,7 +1154,7 @@ _PyCompile_AddDeferredAnnotation(compiler *c, stmt_ty s,
}
Py_DECREF(ptr);
PyObject *index;
if (c->u->u_in_conditional_block) {
if (c->u->u_scope_type == COMPILE_SCOPE_MODULE || c->u->u_in_conditional_block) {
index = PyLong_FromLong(c->u->u_next_conditional_annotation_index);
if (index == NULL) {
return ERROR;
Expand Down Expand Up @@ -1231,6 +1231,12 @@ _PyCompile_InstrSequence(compiler *c)
return c->u->u_instr_sequence;
}

void
_PyCompile_SetInstrSequence(compiler *c, instr_sequence *seq)
{
c->u->u_instr_sequence = seq;
}

int
_PyCompile_FutureFeatures(compiler *c)
{
Expand Down
39 changes: 35 additions & 4 deletions Python/instruction_sequence.c
Original file line number Diff line number Diff line change
Expand Up @@ -34,24 +34,29 @@ typedef _Py_SourceLocation location;
}

static int
instr_sequence_next_inst(instr_sequence *seq) {
instr_sequence_grow(instr_sequence *seq, int size) {
assert(seq->s_instrs != NULL || seq->s_used == 0);


_Py_c_array_t array = {
.array = (void*)seq->s_instrs,
.allocated_entries = seq->s_allocated,
.item_size = sizeof(instruction),
.initial_num_entries = INITIAL_INSTR_SEQUENCE_SIZE,
};

RETURN_IF_ERROR(_Py_CArray_EnsureCapacity(&array, seq->s_used + 1));
RETURN_IF_ERROR(_Py_CArray_EnsureCapacity(&array, seq->s_used + size));
seq->s_instrs = array.array;
seq->s_allocated = array.allocated_entries;

assert(seq->s_allocated >= 0);
assert(seq->s_used < seq->s_allocated);
return seq->s_used++;
seq->s_used += size;
return seq->s_used - 1;
}

static int
instr_sequence_next_inst(instr_sequence *seq) {
return instr_sequence_grow(seq, 1);
}

_PyJumpTargetLabel
Expand Down Expand Up @@ -154,6 +159,32 @@ _PyInstructionSequence_InsertInstruction(instr_sequence *seq, int pos,
return SUCCESS;
}

int
_PyInstructionSequence_PrependSequence(instr_sequence *seq, int pos,
instr_sequence *nested)
{
assert(pos >= 0 && pos <= seq->s_used);
// Merging labelmaps is not supported
assert(nested->s_labelmap_size == 0 && nested->s_nested == NULL);
if (nested->s_used == 0) {
return SUCCESS;
}

int last_idx = instr_sequence_grow(seq, nested->s_used);

RETURN_IF_ERROR(last_idx);
for (int i = last_idx - nested->s_used; i >= pos; i--) {
seq->s_instrs[i + nested->s_used] = seq->s_instrs[i];
}
for (int i=0; i < nested->s_used; i++) {
seq->s_instrs[i + pos] = nested->s_instrs[i];
}
for(int lbl=0; lbl < seq->s_labelmap_size; lbl++) {
seq->s_labelmap[lbl] += nested->s_used;
}
return SUCCESS;
}

int
_PyInstructionSequence_AddNested(instr_sequence *seq, instr_sequence *nested)
{
Expand Down
6 changes: 4 additions & 2 deletions Python/symtable.c
Original file line number Diff line number Diff line change
Expand Up @@ -2749,8 +2749,10 @@ symtable_visit_annotation(struct symtable *st, expr_ty annotation, void *key)
// Annotations in local scopes are not executed and should not affect the symtable
bool is_unevaluated = st->st_cur->ste_type == FunctionBlock;

if ((st->st_cur->ste_type == ClassBlock || st->st_cur->ste_type == ModuleBlock)
&& st->st_cur->ste_in_conditional_block
// Module-level annotations are always considered conditional because the module
// may be partially executed.
if ((((st->st_cur->ste_type == ClassBlock && st->st_cur->ste_in_conditional_block)
|| st->st_cur->ste_type == ModuleBlock))
&& !st->st_cur->ste_has_conditional_annotations)
{
st->st_cur->ste_has_conditional_annotations = 1;
Expand Down
Loading
0