Skip to content

gh-135700: Fix instructions in __annotate__ have incorrect code positions #135814

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.

Sign up for GitHub

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

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
+46 −7
Open

gh-135700: Fix instructions in __annotate__ have incorrect code positions #135814

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
983ae79
fix instructions in __annotate__ have incorrect code positions
AndPuQing Jun 22, 2025
dd3bea2
📜🤖 Added by blurb_it.
blurb-it[bot] Jun 22, 2025
c2f0a4d
Update Misc/NEWS.d/next/Core_and_Builtins/2025-06-22-10-47-27.gh-issu…
AndPuQing Jun 22, 2025
b79060e
add unit tests
AndPuQing Jun 22, 2025
2201864
fix test
AndPuQing Jun 23, 2025
aed06b7
fix test
AndPuQing Jun 23, 2025
d85247e
Update Lib/test/test_dis.py
AndPuQing Jun 24, 2025
eca9248
Apply suggestions from code review
AndPuQing Jun 24, 2025
e14cb29
fix test
AndPuQing Jun 24, 2025
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
37 changes: 37 additions & 0 deletions Lib/test/test_dis.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1124,6 +1124,43 @@ def test_bug_46724(self):
# Test that negative operargs are handled properly
self.do_disassembly_test(bug46724, dis_bug46724)

def test_annotate_source_locations(self):
# See gh-135700
issue_135700 = "1\nx: int"
issue_135700_class = "class A:\n 1\n x: int"

test_cases = [
("module", compile(issue_135700, "<string>", "exec").co_consts[1]),
(
"class",
compile(ast.parse(issue_135700_class), "?", "exec")
.co_consts[0]
.co_consts[1],
),
]

for case_name, annotate_code in test_cases:
Copy link
Member

Choose a reason for hiding this comment

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

assert here that annotate_code is a code object and that annotate_code.co_name is '__annotate__'.

with self.subTest(case=case_name):
line_starts_iterator = dis.findlinestarts(annotate_code)
valid_line_starts = [
item[0]
for item in line_starts_iterator
if item[1] is not None
] # The first item is not RESUME in class case
setup_scope_begin = valid_line_starts[0]
setup_scope_end = valid_line_starts[1]
setup_annotations_scope_positions = {
instr.positions
for instr in dis.get_instructions(annotate_code)
if setup_scope_begin <= instr.offset < setup_scope_end
and instr.positions
}
Copy link
Member

Choose a reason for hiding this comment

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

Why does this need to be so complicated? Don't we just need to check that all the instructions in this code object have the same line number (excluding Nones)?

Copy link
Contributor Author

@AndPuQing AndPuQing Jun 24, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Because the important thing is to make sure the instructions in setup_annotations_scope are consistent with the end_col_offset of RESUME, not just the lineno

no fix:

Positions(lineno=1, end_lineno=1, col_offset=0, end_col_offset=0) RESUME       
Positions(lineno=1, end_lineno=1, col_offset=0, end_col_offset=2) LOAD_FAST_BORROW         # incorrect
Positions(lineno=1, end_lineno=1, col_offset=0, end_col_offset=2) LOAD_SMALL_INT           # incorrect
Positions(lineno=1, end_lineno=1, col_offset=0, end_col_offset=2) COMPARE_OP               # incorrect
Positions(lineno=1, end_lineno=1, col_offset=0, end_col_offset=2) POP_JUMP_IF_FALSE        # incorrect
Positions(lineno=1, end_lineno=1, col_offset=0, end_col_offset=2) NOT_TAKEN                # incorrect
Positions(lineno=1, end_lineno=1, col_offset=0, end_col_offset=2) LOAD_COMMON_CONSTANT     # incorrect
Positions(lineno=1, end_lineno=1, col_offset=0, end_col_offset=2) RAISE_VARARGS            # incorrect
Positions(lineno=1, end_lineno=1, col_offset=0, end_col_offset=2) BUILD_MAP                # incorrect

fix:

Positions(lineno=1, end_lineno=1, col_offset=0, end_col_offset=0) RESUME
Positions(lineno=1, end_lineno=1, col_offset=0, end_col_offset=0) LOAD_FAST_BORROW
Positions(lineno=1, end_lineno=1, col_offset=0, end_col_offset=0) LOAD_SMALL_INT
Positions(lineno=1, end_lineno=1, col_offset=0, end_col_offset=0) COMPARE_OP
Positions(lineno=1, end_lineno=1, col_offset=0, end_col_offset=0) POP_JUMP_IF_FALSE
Positions(lineno=1, end_lineno=1, col_offset=0, end_col_offset=0) NOT_TAKEN
Positions(lineno=1, end_lineno=1, col_offset=0, end_col_offset=0) LOAD_COMMON_CONSTANT
Positions(lineno=1, end_lineno=1, col_offset=0, end_col_offset=0) RAISE_VARARGS
Positions(lineno=1, end_lineno=1, col_offset=0, end_col_offset=0) BUILD_MAP

And not all annotate lines are the same, because some opcodes need to indicate the "code" where the annotation occurs.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, so we need to check they all have the same end_col_offset as well.

Copy link
Member

Choose a reason for hiding this comment

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

And not all annotate lines are the same, because some opcodes need to indicate the "code" where the annotation occurs.

The test doesn't need to work for all inputs. Only the two inputs it runs on.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And not all annotate lines are the same, because some opcodes need to indicate the "code" where the annotation occurs.

The test doesn't need to work for all inputs. Only the two inputs it runs on.

Just to be clear, instructions within code object can correspond to different line numbers. So I made an offset restriction here, limiting the check to between RESUME and BUILD_MAP (setup_annotations_scope)

Copy link
Member

Choose a reason for hiding this comment

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

Another option is to leave dis and the bytecode alone, and write a test that asserts that something which is user-visible is fixed. How did you find this problem? A traceback pointed to the wrong place?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Another option is to leave dis and the bytecode alone, and write a test that asserts that something which is user-visible is fixed. How did you find this problem? A traceback pointed to the wrong place?

This issue appears to stem from an implementation choice in a downstream repository. For full context, you can refer to the issue #135700 linked in the PR.

My understanding is that the downstream project expects the position information for bytecode within setup_annotations_scope to have semantics similar to a RESUME instruction. Their reasoning seems to be that this bytecode is synthetic—it's generated by the compiler and isn't tied to any specific line of user source code, so there's no obvious "correct" location for it to point to.

Ultimately, this doesn't seem to be a factual bug, but rather a disagreement or ambiguity in the semantic interpretation of this position information.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe @JelleZijlstra has an idea how to write a meaningful test for this.

Copy link
Member

Choose a reason for hiding this comment

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

I don't have much of an intuition on how line numbers should work for synthetic code.

Copy link
Member

Choose a reason for hiding this comment

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

@15r10nk are you able to suggest a meaningful unit test for this? Alternatively, can you confirm that the PR resolved the issue?

self.assertEqual(
len(setup_annotations_scope_positions),
1,
f"{case_name}: Expected uniform positions, found {len(setup_annotations_scope_positions)}: {setup_annotations_scope_positions}",
)

def test_kw_names(self):
# Test that value is displayed for keyword argument names:
self.do_disassembly_test(wrap_func_w_kwargs, dis_kw_names)
Expand Down
1 change: 1 addition & 0 deletions Misc/NEWS.d/next/Core_and_Builtins/2025-06-22-10-47-27.gh-issue-135700.0qdtCl.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Fix instructions positions in :attr:`~object.__annotate__`.
15 changes: 8 additions & 7 deletions Python/codegen.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -685,13 +685,14 @@ codegen_setup_annotations_scope(compiler *c, location loc,
PyObject *value_with_fake_globals = PyLong_FromLong(_Py_ANNOTATE_FORMAT_VALUE_WITH_FAKE_GLOBALS);
assert(!SYMTABLE_ENTRY(c)->ste_has_docstring);
_Py_DECLARE_STR(format, ".format");
ADDOP_I(c, loc, LOAD_FAST, 0);
ADDOP_LOAD_CONST(c, loc, value_with_fake_globals);
ADDOP_I(c, loc, COMPARE_OP, (Py_GT << 5) | compare_masks[Py_GT]);

ADDOP_I(c, NO_LOCATION, LOAD_FAST, 0);
ADDOP_LOAD_CONST(c, NO_LOCATION, value_with_fake_globals);
ADDOP_I(c, NO_LOCATION, COMPARE_OP, (Py_GT << 5) | compare_masks[Py_GT]);
NEW_JUMP_TARGET_LABEL(c, body);
ADDOP_JUMP(c, loc, POP_JUMP_IF_FALSE, body);
ADDOP_I(c, loc, LOAD_COMMON_CONSTANT, CONSTANT_NOTIMPLEMENTEDERROR);
ADDOP_I(c, loc, RAISE_VARARGS, 1);
ADDOP_JUMP(c, NO_LOCATION, POP_JUMP_IF_FALSE, body);
ADDOP_I(c, NO_LOCATION, LOAD_COMMON_CONSTANT, CONSTANT_NOTIMPLEMENTEDERROR);
ADDOP_I(c, NO_LOCATION, RAISE_VARARGS, 1);
USE_LABEL(c, body);
return SUCCESS;
}
Expand Down Expand Up @@ -750,7 +751,7 @@ codegen_deferred_annotations_body(compiler *c, location loc,
assert(PyList_CheckExact(conditional_annotation_indices));
assert(annotations_len == PyList_Size(conditional_annotation_indices));

ADDOP_I(c, loc, BUILD_MAP, 0); // stack now contains <annos>
ADDOP_I(c, NO_LOCATION, BUILD_MAP, 0); // stack now contains <annos>

for (Py_ssize_t i = 0; i < annotations_len; i++) {
PyObject *ptr = PyList_GET_ITEM(deferred_anno, i);
Expand Down
Loading