Skip to content

Commit 82f3b22

Browse files
self-sasimeta-codesync[bot]
authored andcommitted
fix: avoid adding parameters for variables defined in the selection during helper extraction (#2373)
Summary: This PR fixes an issue where the “Extract into helper" functionality incorrectly added variables as parameters to the extracted function when those variables were actually defined inside the selection. Specifically, variables on the left-hand side of augmented assignments (e.g., `total` in `total += price`) were unconditionally added to the extracted method's parameter list, even if the variable was initialised earlier in the same selection (e.g., `total = 0`). The fix simply checks `store_refs` for a prior assignment of the variable within the selection before adding it as a parameter. Fixes #2335 Pull Request resolved: #2373 Test Plan: 1. Added a regression test to cover the changed behaviour. 2. Manual testing: 2.1. Locally built pyrefly to `/some-dir/pyrefly` 2.2. In editor settings, add the following setting: `”pyrefly.lspPath”: ”/some-dir/pyrefly”` 2.3. Manually test the “Extract into helper” functionality to verify fix. See below for reference. https://github.com/user-attachments/assets/58b18f2a-262b-468b-8f28-1a6e7ee35ebe Reviewed By: grievejia Differential Revision: D93156987 Pulled By: kinto0 fbshipit-source-id: b027e9a978a99585349efca736993268eecd8184
1 parent 84719ad commit 82f3b22

File tree

2 files changed

+66
-2
lines changed

2 files changed

+66
-2
lines changed

pyrefly/lib/state/lsp/quick_fixes/extract_function.rs

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -72,8 +72,13 @@ pub(crate) fn extract_function_code_actions(
7272
continue;
7373
}
7474
if ident.synthetic_load {
75-
seen_params.insert(ident.name.clone());
76-
params.push(ident.name.clone());
75+
let defined_earlier_in_selection = store_refs
76+
.iter()
77+
.any(|store| store.name == ident.name && store.position < ident.position);
78+
if !defined_earlier_in_selection {
79+
seen_params.insert(ident.name.clone());
80+
params.push(ident.name.clone());
81+
}
7782
continue;
7883
}
7984
let defs = transaction.find_definition(handle, ident.position, FindPreference::default());

pyrefly/lib/test/lsp/code_actions.rs

Lines changed: 59 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1486,6 +1486,65 @@ class Outer:
14861486
assert_eq!(expected.trim(), updated.trim());
14871487
}
14881488

1489+
#[test]
1490+
fn extract_function_excludes_vars_defined_in_selection() {
1491+
// variables defined within the selection should not become parameters, even
1492+
// when they are later used in augmented assignments (e.g., total += price).
1493+
let code = r#"
1494+
def calculate_total_price(prices: list[int]) -> float:
1495+
# EXTRACT-START
1496+
total = 0
1497+
for price in prices:
1498+
total += price
1499+
with_tax = total * 1.085
1500+
# EXTRACT-END
1501+
return with_tax
1502+
"#;
1503+
let updated = apply_first_extract_action(code).expect("expected extract refactor action");
1504+
let expected = r#"
1505+
def extracted_function(prices):
1506+
total = 0
1507+
for price in prices:
1508+
total += price
1509+
with_tax = total * 1.085
1510+
return with_tax
1511+
1512+
def calculate_total_price(prices: list[int]) -> float:
1513+
# EXTRACT-START
1514+
with_tax = extracted_function(prices)
1515+
# EXTRACT-END
1516+
return with_tax
1517+
"#;
1518+
assert_eq!(expected.trim(), updated.trim());
1519+
}
1520+
1521+
#[test]
1522+
fn extract_function_includes_var_from_augmented_assign_without_prior_def() {
1523+
// When the selection contains only an augmented assignment (e.g., x += 1)
1524+
// without a prior definition of that variable, the variable must still be
1525+
// added as a parameter to the extracted function.
1526+
let code = r#"
1527+
def update(x: int) -> int:
1528+
# EXTRACT-START
1529+
x += 1
1530+
# EXTRACT-END
1531+
return x
1532+
"#;
1533+
let updated = apply_first_extract_action(code).expect("expected extract refactor action");
1534+
let expected = r#"
1535+
def extracted_function(x):
1536+
x += 1
1537+
return x
1538+
1539+
def update(x: int) -> int:
1540+
# EXTRACT-START
1541+
x = extracted_function(x)
1542+
# EXTRACT-END
1543+
return x
1544+
"#;
1545+
assert_eq!(expected.trim(), updated.trim());
1546+
}
1547+
14891548
#[test]
14901549
fn extract_variable_basic_refactor() {
14911550
let code = r#"

0 commit comments

Comments
 (0)