Skip to content

Remove unused rust impl for formatting dis output #7660

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

Merged
youknowone merged 28 commits into from
May 5, 2026
Merged

Remove unused rust impl for formatting dis output #7660

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
2cbbaf1
Remove unused rust impl for formatting dis output
ShaharNaveh Apr 23, 2026
774bdcd
remove `examples/dis.rs`
ShaharNaveh Apr 23, 2026
ad9feb0
Added tests
ShaharNaveh Apr 23, 2026
256252c
Update lock
ShaharNaveh Apr 28, 2026
7e8938d
Try to set snapshot dir
ShaharNaveh Apr 28, 2026
693ab8c
Remove verbose flag
ShaharNaveh Apr 28, 2026
ce8b0c8
Regenerate snapshots after #7711
ShaharNaveh Apr 28, 2026
d9751c9
Revert "Bump insta from 1.46.3 to 1.47.2 (#7706)"
ShaharNaveh Apr 28, 2026
b0ec4a0
Debug info
ShaharNaveh Apr 29, 2026
dc4c857
Show diff as well
ShaharNaveh Apr 29, 2026
bae530c
Show debug faster
ShaharNaveh Apr 29, 2026
7a77e65
CI: true env
ShaharNaveh Apr 29, 2026
58c8d53
Recert CI
ShaharNaveh Apr 29, 2026
a3f690e
Add `CI: true` in ci emv
ShaharNaveh Apr 29, 2026
182b4f5
Reapply "Bump insta from 1.46.3 to 1.47.2 (#7706)"
ShaharNaveh Apr 29, 2026
c235e6c
simplify macro
ShaharNaveh Apr 30, 2026
42353fa
trim on function side
ShaharNaveh Apr 30, 2026
2450d69
Force insta workspace root
ShaharNaveh May 1, 2026
b5326b3
Merge remote-tracking branch 'upstream/main' into remove-fmt-dis
ShaharNaveh May 1, 2026
e4f7ea0
Merge remote-tracking branch 'upstream/main' into remove-fmt-dis
ShaharNaveh May 2, 2026
89a4da6
Merge remote-tracking branch 'upstream/main' into remove-fmt-dis
ShaharNaveh May 2, 2026
8f3d793
Merge remote-tracking branch 'upstream/main' into remove-fmt-dis
ShaharNaveh May 4, 2026
1cc518f
Merge remote-tracking branch 'upstream/main' into remove-fmt-dis
ShaharNaveh May 4, 2026
ad2b24f
fix merge
ShaharNaveh May 4, 2026
fd5d487
Merge remote-tracking branch 'upstream/main' into remove-fmt-dis
ShaharNaveh May 5, 2026
5c3aa0d
fixing bad merge
ShaharNaveh May 5, 2026
15d212f
regen snapshot
ShaharNaveh May 5, 2026
5806655
Merge remote-tracking branch 'upstream/main' into remove-fmt-dis
ShaharNaveh May 5, 2026
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
5 changes: 4 additions & 1 deletion .github/workflows/ci.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ env:
CARGO_PROFILE_RELEASE_DEBUG: 0
CARGO_TERM_COLOR: always
FORCE_JAVASCRIPT_ACTIONS_TO_NODE24: true # TODO: Remove on 2026/06/02
CI: true

jobs:
determine_changes:
Expand Down Expand Up @@ -111,7 +112,9 @@ jobs:
uses: ./.github/actions/install-macos-deps

- name: run rust tests
run: cargo test --workspace ${{ env.WORKSPACE_EXCLUDES }} --verbose --features threading ${{ env.CARGO_ARGS }}
run: cargo test --workspace ${{ env.WORKSPACE_EXCLUDES }} --features threading ${{ env.CARGO_ARGS }}
Copy link
Copy Markdown
Contributor

@fanninpm fanninpm Apr 29, 2026

Choose a reason for hiding this comment

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

Suggested change
run: cargo test --workspace ${{ env.WORKSPACE_EXCLUDES }} --features threading ${{ env.CARGO_ARGS }}
run: cargo test --workspace ${{ env.WORKSPACE_EXCLUDES }} --features threading ${{ env.CARGO_ARGS }}
env:
CI: true

What about here?

Copy link
Copy Markdown
Contributor Author

@ShaharNaveh ShaharNaveh Apr 29, 2026

Choose a reason for hiding this comment

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

I still think there's a value to putting it at the top. other tools in the future (or even exiting ones) might look at this value, it because a standard from what I can tell. but it's not a hill I'm willing to die on

env:
INSTA_WORKSPACE_ROOT: ${{ github.workspace }}

- run: cargo doc --locked
if: runner.os == 'Linux'
Expand Down
3 changes: 2 additions & 1 deletion Cargo.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 0 additions & 1 deletion crates/codegen/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,6 @@ unicode_names2 = { workspace = true }

[dev-dependencies]
ruff_python_parser = { workspace = true }
insta = { workspace = true }

[lints]
workspace = true
100 changes: 0 additions & 100 deletions crates/codegen/src/compile.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12275,26 +12275,6 @@ def f(sys, os, file):
})
}

macro_rules! assert_dis_snapshot {
($value:expr) => {
insta::assert_snapshot!(
insta::internals::AutoName,
$value.display_expand_code_objects().to_string(),
stringify!($value)
)
};
}

#[test]
fn test_if_ors() {
assert_dis_snapshot!(compile_exec(
"\
if True or False or False:
pass
Comment on lines -12289 to -12293
Copy link
Copy Markdown
Member

@youknowone youknowone Apr 24, 2026

Choose a reason for hiding this comment

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

oh.. are these test going to be entirely deleted? This is not desired. Can we replace the snapshot test to other way not depending on internal dis?

Copy link
Copy Markdown
Contributor Author

@ShaharNaveh ShaharNaveh Apr 24, 2026

Choose a reason for hiding this comment

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

The only way I can think of is to move them under crates/vm or crates/stdlib, then parsing the source code into a PyCode, then run it like:

let module = vm.import("dis", 0)?;
let dis = module.get_attr("dis", vm)?;
let res = dis.call(my_pycode, vm)?;

// <convert res to String and pass to Insta>

Copy link
Copy Markdown
Contributor Author

@ShaharNaveh ShaharNaveh Apr 28, 2026

Choose a reason for hiding this comment

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

@youknowone lmk what you think

"
));
}

#[test]
fn test_trace_assert_true_try_pair() {
let trace = compile_exec_late_cfg_trace(
Expand Down Expand Up @@ -12529,44 +12509,6 @@ def f(self):
);
}

#[test]
fn test_if_ands() {
assert_dis_snapshot!(compile_exec(
"\
if True and False and False:
pass
"
));
}

#[test]
fn test_if_mixed() {
assert_dis_snapshot!(compile_exec(
"\
if (True and False) or (False and True):
pass
"
));
}

#[test]
fn test_nested_bool_op() {
assert_dis_snapshot!(compile_exec(
"\
x = Test() and False or False
"
));
}

#[test]
fn test_const_bool_not_op() {
assert_dis_snapshot!(compile_exec_optimized(
"\
x = not True
"
));
}

#[test]
fn test_plain_constant_bool_op_folds_to_selected_operand() {
let code = compile_exec(
Expand Down Expand Up @@ -12840,24 +12782,6 @@ def f(self, mod):
);
}

#[test]
fn test_nested_double_async_with() {
assert_dis_snapshot!(compile_exec(
"\
async def test():
for stop_exc in (StopIteration('spam'), StopAsyncIteration('ham')):
with self.subTest(type=type(stop_exc)):
try:
async with egg():
raise stop_exc
except Exception as ex:
self.assertIs(ex, stop_exc)
else:
self.fail(f'{stop_exc} was suppressed')
"
));
}

#[test]
fn test_scope_exit_instructions_keep_line_numbers() {
let code = compile_exec(
Expand Down Expand Up @@ -14006,20 +13930,6 @@ def f(expected_ns, namespace):
}
}

#[test]
fn test_bare_function_annotations_check_attribute_and_subscript_expressions() {
assert_dis_snapshot!(compile_exec(
"\
def f(one: int):
int.new_attr: int
[list][0].new_attr: [int, str]
my_lst = [1]
my_lst[one]: int
return my_lst
"
));
}

#[test]
fn test_non_simple_bare_name_annotation_does_not_create_local_binding() {
let code = compile_exec(
Expand Down Expand Up @@ -14052,16 +13962,6 @@ def f2bad():
);
}

#[test]
fn test_constant_true_if_pass_keeps_line_anchor_nop() {
assert_dis_snapshot!(compile_exec(
"\
if 1:
pass
"
));
}

#[test]
fn test_negative_constant_binop_folds_after_unary_folding() {
let code = compile_exec(
Expand Down
3 changes: 1 addition & 2 deletions crates/codegen/src/ir.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9378,8 +9378,7 @@ impl CodeInfo {
} else {
OpArg::new(ins.target.0)
};
let instr_display = instr.display(display_arg, self);
eprint!("{instr_display}: {depth} {effect:+} => ");
eprint!("{display_arg:?}: {depth} {effect:+} => ");
}
let new_depth = depth.checked_add_signed(effect).ok_or({
if effect < 0 {
Expand Down
67 changes: 0 additions & 67 deletions ..._compile__tests__bare_function_annotations_check_attribute_and_subscript_expressions.snap
View file
Open in desktop

This file was deleted.

9 changes: 0 additions & 9 deletions crates/codegen/src/snapshots/rustpython_codegen__compile__tests__const_bool_not_op.snap
View file
Open in desktop

This file was deleted.

10 changes: 0 additions & 10 deletions ...hots/rustpython_codegen__compile__tests__constant_true_if_pass_keeps_line_anchor_nop.snap
View file
Open in desktop

This file was deleted.

8 changes: 0 additions & 8 deletions crates/codegen/src/snapshots/rustpython_codegen__compile__tests__if_ands.snap
View file
Open in desktop

This file was deleted.

8 changes: 0 additions & 8 deletions crates/codegen/src/snapshots/rustpython_codegen__compile__tests__if_mixed.snap
View file
Open in desktop

This file was deleted.

10 changes: 0 additions & 10 deletions crates/codegen/src/snapshots/rustpython_codegen__compile__tests__if_ors.snap
View file
Open in desktop

This file was deleted.

35 changes: 0 additions & 35 deletions crates/codegen/src/snapshots/rustpython_codegen__compile__tests__nested_bool_op.snap
View file
Open in desktop

This file was deleted.

Loading
Loading