-
Notifications
You must be signed in to change notification settings - Fork 19
jit(wasm): drop the wasm32 arm of the self-recursive root-bridge inline #1106
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 all commits
a62f004
95ec7f2
203cd0c
87c0439
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 |
|---|---|---|
| @@ -1,11 +1,14 @@ | ||
| bridges_compiled=26 | ||
| bridges_compiled=27 | ||
| descr_set_absent=0 | ||
| descr_set_ambiguous=0 | ||
| descr_set_stale_absent=0 | ||
| fbw_blackhole_adopted_multi_frame=0 | ||
| fbw_blackhole_adopted_single_frame=0 | ||
| fbw_rolled_back_with_effects=0 | ||
| fbw_store_journal_rollback_failed=0 | ||
| field_pos_attached_misplaced=0 | ||
| field_pos_spec_misplaced=0 | ||
| guard_failures=5165 | ||
| guard_failures=5150 | ||
| internal_compile_panics=0 | ||
| loops_aborted=2 | ||
| loops_aborted=1 | ||
| loops_compiled=3 |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,14 @@ | ||
| bridges_compiled=6 | ||
| descr_set_absent=0 | ||
| descr_set_ambiguous=0 | ||
| descr_set_stale_absent=0 | ||
| fbw_blackhole_adopted_multi_frame=0 | ||
| fbw_blackhole_adopted_single_frame=0 | ||
| fbw_rolled_back_with_effects=0 | ||
| fbw_store_journal_rollback_failed=0 | ||
| field_pos_attached_misplaced=0 | ||
| field_pos_spec_misplaced=0 | ||
| guard_failures=806 | ||
| internal_compile_panics=0 | ||
| loops_aborted=0 | ||
| loops_compiled=2 |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,14 @@ | ||
| bridges_compiled=6 | ||
| descr_set_absent=0 | ||
| descr_set_ambiguous=0 | ||
| descr_set_stale_absent=0 | ||
| fbw_blackhole_adopted_multi_frame=0 | ||
| fbw_blackhole_adopted_single_frame=0 | ||
| fbw_rolled_back_with_effects=0 | ||
| fbw_store_journal_rollback_failed=0 | ||
| field_pos_attached_misplaced=0 | ||
| field_pos_spec_misplaced=0 | ||
| guard_failures=806 | ||
| internal_compile_panics=0 | ||
| loops_aborted=0 | ||
| loops_compiled=2 |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,38 @@ | ||
| # pyre-check: max-pypy-ratio=20 | ||
| # Coverage for the self-recursive root-bridge inline when the recursion is | ||
| # non-tail and carries a Ref local. | ||
| # | ||
| # `walk` is self-recursive, takes exact-integer arguments and holds a | ||
| # `BinaryOp` residual, so a guard-failure bridge that reaches its CALL takes the | ||
| # root-bridge admission (`bridge_rec_root_selfrec`, inline_call.rs). | ||
| # `bridge_recursion_overflow` already covers that admission, but only in its | ||
| # easiest form: tail recursion whose live set is two machine integers. Two | ||
| # ingredients of the "a Ref reached an int operation" failure it is meant to | ||
| # guard against were therefore unexercised. | ||
| # | ||
| # `acc * 2` crosses the machine-int boundary partway down the recursion, so the | ||
| # accumulator promotes to a long — a Ref — at a level that moves with the | ||
| # caller's seed, and the overflow guard fires inside the recursive frame. `tag` | ||
| # keeps a second Ref live across the recursive CALL beside it, and the non-tail | ||
| # `inner + len(tag)` leaves a paused caller chain, so the guard's resume stream | ||
| # is multi-frame and mixes Ref with Int rather than being one frame of | ||
| # integers. | ||
| # | ||
| # Byte-parity against CPython/PyPy is the gate: Python integers are unbounded, | ||
| # so the promotion must not be observable in the result. | ||
| _TAGS = ("a", "bb", "ccc", "dddd") | ||
|
|
||
|
|
||
| def walk(n, acc): | ||
| if n == 0: | ||
| return acc | ||
| tag = _TAGS[n & 3] | ||
| nxt = acc * 2 + len(tag) | ||
| inner = walk(n - 1, nxt) | ||
| return inner + len(tag) | ||
|
|
||
|
|
||
| out = [] | ||
| for i in range(50000): | ||
| out.append(walk(24, (1 << 50) + i) % 1000000007) | ||
| print(out[0], out[-1], sum(out) % 1000000007) | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,14 @@ | ||
| bridges_compiled=6 | ||
| descr_set_absent=0 | ||
| descr_set_ambiguous=0 | ||
| descr_set_stale_absent=0 | ||
| fbw_blackhole_adopted_multi_frame=0 | ||
| fbw_blackhole_adopted_single_frame=0 | ||
| fbw_rolled_back_with_effects=0 | ||
| fbw_store_journal_rollback_failed=0 | ||
| field_pos_attached_misplaced=0 | ||
| field_pos_spec_misplaced=0 | ||
| guard_failures=806 | ||
| internal_compile_panics=0 | ||
| loops_aborted=0 | ||
| loops_compiled=2 |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,9 +1,14 @@ | ||
| bridges_compiled=1 | ||
| bridges_compiled=3 | ||
| descr_set_absent=0 | ||
| descr_set_ambiguous=0 | ||
| descr_set_stale_absent=0 | ||
| fbw_blackhole_adopted_multi_frame=0 | ||
| fbw_blackhole_adopted_single_frame=0 | ||
| fbw_rolled_back_with_effects=0 | ||
| guard_failures=404 | ||
| fbw_store_journal_rollback_failed=0 | ||
| field_pos_attached_misplaced=0 | ||
| field_pos_spec_misplaced=0 | ||
| guard_failures=601 | ||
| internal_compile_panics=0 | ||
| loops_aborted=1 | ||
| loops_compiled=2 |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,38 @@ | ||
| # The operand stack an aborted inline sub-walk hands back to the interpreter, | ||
| # for a call made inside a FOR_ITER body. | ||
| # | ||
| # When the walker gives up on inlining a callee it flushes the caller's frame | ||
| # at the CALL it entered under and lets the interpreter re-execute the whole | ||
| # call. That flush rebuilds the operand stack from two sources: the enclosing | ||
| # FOR_ITER's iterator (and anything else below the call) from the vstack | ||
| # mirror, and the call's own operands from the encoded residual op. The | ||
| # operands do not sit at a fixed byte offset in that op — the method-form CALL | ||
| # helpers lower through a shape whose leading Int list is variable-width, so a | ||
| # reader that assumes the plain shape's offset picks up the Int list's register | ||
| # indices and resolves them in the Ref bank. The result is a stack of the | ||
| # right HEIGHT (so the flush's depth check passes) holding the wrong objects: | ||
| # here the enclosing loop's iterator arrived as the subscript index, and | ||
| # `SubPattern.__getitem__` raised `TypeError: list indices must be integers or | ||
| # slices, not list_iterator`. | ||
| # | ||
| # `re.compile` of a large flat alternation is the reproducer: `_compile_info` | ||
| # calls `_get_charset_prefix`, whose BRANCH arm loops `for p in av[1]` and | ||
| # subscripts `p[0]` inside the body. The alternation has to be big enough for | ||
| # that loop to go hot — it is clean below about a thousand branches — and no | ||
| # hand-written class with the same shape has been made to reach the abort leg, | ||
| # so the real module drives it. | ||
| # | ||
| # The compile is the assertion: any wrong operand raises out of `re`. | ||
|
|
||
| import re | ||
|
|
||
| BRANCHES = 2000 | ||
|
|
||
| pattern = "|".join("%d" % x for x in range(BRANCHES)) | ||
| compiled = re.compile(pattern) | ||
|
|
||
| # Alternation is first-match, so "1999" is matched by the earlier "1" branch. | ||
| assert compiled.match("1999").group(0) == "1" | ||
| assert compiled.match("nope") is None | ||
|
|
||
| print("OK") |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -70,9 +70,14 @@ def raises(call, exc): | |
| os.close(fd) | ||
|
|
||
| # `times` is the one argument here that may be spelled either way — it sits | ||
| # before the keyword-only marker. | ||
| os.utime(p, times=(-5.0, -6.0)) | ||
| check(os.stat(p).st_mtime_ns == -6_000_000_000, "utime(times=...) by keyword") | ||
| # before the keyword-only marker. The pair is one the platform holds: Windows | ||
| # refuses times before the epoch, for the reason given above. | ||
| atime, mtime = (5.0, 6.0) if sys.platform == "win32" else (-5.0, -6.0) | ||
|
Comment on lines
+73
to
+75
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.
On Windows, AGENTS.md reference: AGENTS.md:L252-L254 Useful? React with 👍 / 👎. |
||
| os.utime(p, times=(atime, mtime)) | ||
| check( | ||
| os.stat(p).st_mtime_ns == int(mtime) * 1_000_000_000, | ||
| f"utime(times=...) by keyword -> {os.stat(p).st_mtime_ns}", | ||
| ) | ||
| raises(lambda: os.utime(p, (1, 2), times=(3, 4)), TypeError) | ||
|
|
||
| # Back to a time the rest of the file can be reasoned about. | ||
|
|
@@ -82,6 +87,10 @@ def raises(call, exc): | |
| # Every name the table carries either answers with a number or refuses the | ||
| # question — the terminal-only limits are not ones a regular file has. What no | ||
| # answer may be is None: a host with no determinate value says so with -1. | ||
| # | ||
| # `pathconf` and the `pathconf_names` table it resolves through are a POSIX | ||
| # surface; neither runtime carries them on Windows, so there is nothing to | ||
| # compare there. | ||
| def limits(target): | ||
| for name in sorted(os.pathconf_names): | ||
| try: | ||
|
|
@@ -93,22 +102,23 @@ def limits(target): | |
| yield name, limit | ||
|
|
||
|
|
||
| answered = dict(limits(p)) | ||
| check(answered, "pathconf answered no name at all") | ||
| check("PC_NAME_MAX" in answered, "pathconf refused PC_NAME_MAX on a regular file") | ||
| if sys.platform != "win32": | ||
| answered = dict(limits(p)) | ||
| check(answered, "pathconf answered no name at all") | ||
| check("PC_NAME_MAX" in answered, "pathconf refused PC_NAME_MAX on a regular file") | ||
|
|
||
| if os.pathconf in os.supports_fd: | ||
| fd = os.open(p, os.O_RDONLY) | ||
| try: | ||
| by_fd = dict(limits(fd)) | ||
| check( | ||
| by_fd.get("PC_NAME_MAX") == answered["PC_NAME_MAX"], | ||
| "the descriptor and the name disagree about PC_NAME_MAX", | ||
| ) | ||
| finally: | ||
| os.close(fd) | ||
|
|
||
| raises(lambda: os.pathconf(p, "PC_NOT_A_REAL_NAME"), ValueError) | ||
| if os.pathconf in os.supports_fd: | ||
| fd = os.open(p, os.O_RDONLY) | ||
| try: | ||
| by_fd = dict(limits(fd)) | ||
| check( | ||
| by_fd.get("PC_NAME_MAX") == answered["PC_NAME_MAX"], | ||
| "the descriptor and the name disagree about PC_NAME_MAX", | ||
| ) | ||
| finally: | ||
| os.close(fd) | ||
|
|
||
| raises(lambda: os.pathconf(p, "PC_NOT_A_REAL_NAME"), ValueError) | ||
|
|
||
| # ── truncate's length ───────────────────────────────────────────────────── | ||
| # A length wider than off_t is not a size the file can be given; the cast that | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1300,7 +1300,14 @@ pub(crate) fn reconstructed_all_ref_call_stack<Sym: WalkSym>( | |
| op: &DecodedOp, | ||
| ctx: &WalkContext<'_, '_, Sym>, | ||
| ) -> Option<Vec<pyre_object::PyObjectRef>> { | ||
| let fresh = read_ref_var_list_concrete(code, op, 1, ctx); | ||
| // The Ref list is NOT at a fixed offset: the method-form `CALL` helpers | ||
| // this leg latches for lower through the mixed `iIRd>r` shape, whose | ||
| // leading Int list shifts it (`dispatch_residual_call_iIRd_kind` reads it | ||
| // at `1 + i_width`). Reading offset 1 there takes the Int list's register | ||
| // indices into the Ref bank — refs unrelated to the call, of a length that | ||
| // still passes the flush's depth check. | ||
| let ref_operand_offset = ref_var_list_operand_offset(code, op)?; | ||
| let fresh = read_ref_var_list_concrete(code, op, ref_operand_offset, ctx); | ||
| if fresh.is_empty() { | ||
| return None; | ||
| } | ||
|
|
@@ -2865,13 +2872,10 @@ pub(crate) fn try_walker_inline_resolved_user_call<Sym: WalkSym>( | |
| // body sub-walk reaches its own recursive CALL as a nested residual, which | ||
| // `fbw_abort_nested_unjournaled_residual` declines on the self-recursive | ||
| // hazard arm — an abort storm that folds the whole guard bridge back to | ||
| // residual. The native `CALL_ASSEMBLER` self-recursion fold already exempts | ||
| // that decline via `SELFREC_CA_FOLD_ACTIVE`; the same exemption applies to | ||
| // this admitted inline, whose recursive residual runs concretely at the | ||
| // pre-execute site (executed, so no replay double-apply). Native only: the | ||
| // wasm always-portal path type-confuses the self-recursive inline | ||
| // (`setintbound: got Ref`), so it keeps the correct residual-fallback | ||
| // decline. | ||
| // residual. The `CALL_ASSEMBLER` self-recursion fold already exempts that | ||
| // decline via `SELFREC_CA_FOLD_ACTIVE`; the same exemption applies to this | ||
| // admitted inline, whose recursive residual runs concretely at the | ||
| // pre-execute site (executed, so no replay double-apply). | ||
| let mut bridge_rec_root_selfrec = false; | ||
| if ctx.trace_ctx.is_bridge_trace | ||
| && args_all_builtin_integer | ||
|
|
@@ -2911,12 +2915,11 @@ pub(crate) fn try_walker_inline_resolved_user_call<Sym: WalkSym>( | |
| if !safe_root_bridge { | ||
| return Ok(None); | ||
| } | ||
| bridge_rec_root_selfrec = cfg!(not(target_arch = "wasm32")) | ||
| && unsafe { | ||
| let raw = pyre_interpreter::w_code_get_ptr(w_code as pyre_object::PyObjectRef) | ||
| as *const pyre_interpreter::CodeObject; | ||
| !raw.is_null() && pyre_interpreter::code_is_self_recursive(&*raw) | ||
| }; | ||
| bridge_rec_root_selfrec = unsafe { | ||
| let raw = pyre_interpreter::w_code_get_ptr(w_code as pyre_object::PyObjectRef) | ||
| as *const pyre_interpreter::CodeObject; | ||
| !raw.is_null() && pyre_interpreter::code_is_self_recursive(&*raw) | ||
| }; | ||
|
Comment on lines
+2918
to
+2922
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 removal newly enables the exemption on wasm whenever AGENTS.md reference: AGENTS.md:L14-L19 Useful? React with 👍 / 👎. |
||
| } | ||
| // A callee `fbw_abort_nested_unjournaled_residual` already named on its | ||
| // hazard arm residualizes from here on. The hazard is a static property of | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -3947,6 +3947,44 @@ fn concrete_from_recorded_opref<Sym: WalkSym>( | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// The `operand_offset` of this op's Ref var-list (`R`), for a reader that | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// holds only the decoded op and must find that list itself. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// A dispatcher that decoded the whole op passes the offset it already | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// computed; anything reached later — an abort leg, a specializer entered | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// after resolution — has no such value and must not guess one. `R` sits at | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// offset 1 for the Ref-only residual shape (`iRd>r`) but NOT for the mixed | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// one (`iIRd>r`, `riIRd>r`, `iiIRd>r`), whose leading Int list is itself | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// variable-width: reading offset 1 there lands on the Int list's length byte | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// and takes its register indices into the Ref bank, yielding unrelated | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// objects with a plausible length. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// Walks the argcode widths of `blackhole.py:112-157`, the same walk | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// [`decode_op_at`] performs. `None` — an op declaring no Ref list, or one | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// whose earlier operands cannot be width-counted — leaves the caller to | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// decline. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// [`decode_op_at`]: crate::jitcode_runtime::decode_op_at | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| fn ref_var_list_operand_offset(code: &[u8], op: &DecodedOp) -> Option<usize> { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| let first_operand_pc = op.pc + 1; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| let mut cursor = first_operand_pc; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| let mut chars = op.argcodes.chars(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| while let Some(c) = chars.next() { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| match c { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| 'R' => return Some(cursor - first_operand_pc), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| 'i' | 'c' | 'r' | 'f' => cursor += 1, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| 'L' | 'd' | 'j' => cursor += 2, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| 'I' | 'F' => cursor += 1 + *code.get(cursor)? as usize, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| '>' => { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| chars.next()?; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| cursor += 1; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+3974
to
+3980
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. 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win Reject incomplete operands before returning the offset.
For Validate every skipped operand and the complete Proposed fix- 'R' => return Some(cursor - first_operand_pc),
- 'i' | 'c' | 'r' | 'f' => cursor += 1,
- 'L' | 'd' | 'j' => cursor += 2,
- 'I' | 'F' => cursor += 1 + *code.get(cursor)? as usize,
+ 'R' => {
+ let len = *code.get(cursor)? as usize;
+ let end = cursor.checked_add(1 + len)?;
+ code.get(end.checked_sub(1)?)?;
+ return Some(cursor - first_operand_pc);
+ }
+ 'i' | 'c' | 'r' | 'f' => {
+ let end = cursor.checked_add(1)?;
+ code.get(end.checked_sub(1)?)?;
+ cursor = end;
+ }
+ 'L' | 'd' | 'j' => {
+ let end = cursor.checked_add(2)?;
+ code.get(end.checked_sub(1)?)?;
+ cursor = end;
+ }
+ 'I' | 'F' => {
+ let len = *code.get(cursor)? as usize;
+ let end = cursor.checked_add(1 + len)?;
+ code.get(end.checked_sub(1)?)?;
+ cursor = end;
+ }📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| _ => return None, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| None | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// Read concrete shadow values for a Ref-bank variadic operand list. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// Parallels [`read_ref_var_list`] — reads the | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// same byte indices but resolves through `ctx.concrete_registers_r`. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
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.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
Repository: youknowone/pyre
Length of output: 21136
🌐 Web query:
pyproject.toml ruff PERF401 preferred method build list comprehension💡 Result:
The Ruff rule PERF401 (manual-list-comprehension) identifies for-loops that can be replaced with a list comprehension or a list extend operation to improve readability and performance [1][2]. The preferred method depends on the context of the code: 1. When initializing a new list: Ruff suggests replacing the loop with a list comprehension [1][3]. Example: # Original filtered = [] for i in original: if i % 2: filtered.append(i) # Preferred (List Comprehension) filtered = [i for i in original if i % 2] 2. When appending to an existing, already populated list: Ruff suggests using the list.extend method with a generator expression [1][4]. Example: # Original existing_list = [1][5] for i in original: if i % 2: existing_list.append(i) # Preferred (list.extend) existing_list.extend(i for i in original if i % 2) Key Considerations: - Purpose: The rule is a micro-optimization [1][2]. While list comprehensions are generally faster (typically 10-25% depending on the Python version), the performance impact is negligible in many real-world scenarios [1][2]. - Limitations: The automatic fix is not always applicable. Ruff applies this rule primarily to simple for-loops (e.g., those with a single non-if statement or a single if statement containing one nested operation) [6][4]. If a loop contains complex logic or multiple operations, it may not trigger the rule [6]. - False Positives: If the logic requires flattening (which.extend does) versus adding elements, or if the loop body is more complex than the rule's simple heuristic can analyze, you should manually evaluate if the suggested transformation maintains correct behavior [4].
Citations:
perflint] Allow list function calls to be replaced with a comprehension (PERF401) astral-sh/ruff#17519perflint] AddPERF401andPERF402rules astral-sh/ruff#5298Resolve Ruff
PERF401without changing the benchmark shape.This initialization loop triggers
PERF401, but converting it to alist(...)construction may change the JIT trace and invalidate the.jitstatsbaselines. Add a local suppression if the explicit loop is intentional, or regenerate all threeselfrec_bridge_nontail_promoteJIT-stat files if the comprehension is kept.Proposed localized suppression
📝 Committable suggestion
🧰 Tools
🪛 Ruff (0.16.1)
[warning] 37-37: Use a list comprehension to create a transformed list
(PERF401)
🤖 Prompt for AI Agents
Source: Linters/SAST tools