[asukaminato0721]

Summary

part of #2207

adds Remove unused variable x for simple unused local assignments

Test Plan

add test

[Copilot]

Pull request overview

Adds an LSP local quick-fix for unused local variables in Pyrefly, enabling editors to offer an automated “Remove unused variable” action alongside existing quick-fixes.

Changes:

• Introduces a remove_unused_variable quick-fix that removes the corresponding assignment statement and inserts pass when needed.
• Wires the new quick-fix into Transaction::local_quickfix_code_actions_sorted based on bindings.unused_variables().
• Adds LSP code-action tests covering basic removal, pass insertion, and skipping tuple-unpacking assignments.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

File	Description
pyrefly/lib/test/lsp/code_actions.rs	Adds helper + tests validating the new “Remove unused variable” code action behavior.
pyrefly/lib/state/lsp/quick_fixes/remove_unused_variable.rs	Implements the new quick-fix edit generation (statement removal + optional pass).
pyrefly/lib/state/lsp/quick_fixes/mod.rs	Exposes the new quick-fix module.
pyrefly/lib/state/lsp.rs	Adds the new quick-fix to the local quick-fix code action list using unused-variable binding info.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[meta-codesync]

This pull request has been imported. If you are a Meta employee, you can view this in D113979059. (Because this pull request was imported automatically, there will not be any future comments.)

[codspeed]

Merging this PR will not alter performance

✅ 35 untouched benchmarks
⏩ 9 skipped benchmarks¹

---

_{Comparing asukaminato0721:2207-unused-variable (8d72928) with main (1af3067)}

Footnotes

1. 9 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩

[stroxler]

Thanks! Based on AI review, I think this has a number of flaws we need to patch before we can land

Required RHS evaluation is deleted

[remove_unused_variable.rs:28–35](

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

Lines 28 to 35 in df22441

	let context = find_unused_assignment_context(&ast.body, unused_range)?;
	let removal_range = statement_removal_range(module_info.contents(), context.stmt)?;
	let replacement = if needs_pass_after_removal(context.parent_body, context.stmt.range()) {
	let (indent, _) =
	line_indent_and_start(module_info.contents(), context.stmt.range().start())?;
	format!("{indent}pass\n")
	} else {
	String::new()

)

The fix removes the entire assignment without checking its initializer. unused = print("important") loses the print; unused = yield 1 can stop the function being a generator. More directly:

def f() -> int:
    unused = (used := 1)
    return used

The action deletes the definition of used while retaining its use.

Fix: preserve evaluation as an expression statement—with necessary parentheses—or decline the action when evaluation cannot safely be discarded.

Whole-line deletion removes unrelated code

[remove_unused_variable.rs:29–33](

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

Lines 29 to 33 in df22441

	let removal_range = statement_removal_range(module_info.contents(), context.stmt)?;
	let replacement = if needs_pass_after_removal(context.parent_body, context.stmt.range()) {
	let (indent, _) =
	line_indent_and_start(module_info.contents(), context.stmt.range().start())?;
	format!("{indent}pass\n")

)

For unused = 1; return 2, the edit removes both statements. The placeholder check sees two AST statements, so it inserts no pass, leaving an invalid empty function.

For def f() -> None: unused = 1, the edit replaces the entire function with module-level pass.

Fix: decline assignments that do not occupy standalone lines, or use separator- and suite-aware deletion.

A used closure variable can be deleted

[lsp.rs:3786–3801](

pyrefly/pyrefly/lib/state/lsp.rs

Lines 3786 to 3801 in df22441

	if let Some(answers) = self.get_answers(handle) {
	let bindings = answers.bindings();
	for unused in bindings.unused_variables() {
	if unused.range.contains_range(range)
	&& let Some(action) =
	quick_fixes::remove_unused_variable::remove_unused_variable_code_action(
	&module_info,
	&ast,
	unused.name.as_str(),
	unused.range,
	)
	{
	other_actions.push(action);
	}
	}
	}

)

def f():
    def g(): return x
    x = 1
    return g

The existing detector misses the read before registration, and this PR offers deletion of x = 1. Calling the returned closure then raises NameError. A loop reading a variable before its textual assignment also reaches the action.

Fix: correct the detector or conservatively validate actual references before offering destructive edits. The underlying diagnostic weakness predates the PR; turning it into deletion is new.

Intentionally unused names receive the action

The same integration block omits the diagnostics’ Ast::is_intentionally_unused filter. _ = compute() therefore receives “Remove unused variable _” despite deliberately using the discard convention.

Fix: share the eligibility filter with unused-variable diagnostics.

[github-actions]

According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅

[stroxler]

This is another one I should have closed, it's too old and is linked to a stale diff :/

Let's close this and open a fresh PR for easier review turnaround. While we're at it, a fresh round of review caught a one more edge case worth addressing:

deletion can invalidate a nonlocal declaration

The reference guard at [remove_unused_variable.rs:38–47](https://github.com/meta/pyrefly/blob/8d7292890c3901b4caa31a726abeda4a2344df52/pyrefly/lib/state/lsp/quick_fixes/remove_unused_variable.rs#L38-L47) only checks Expr::Name. A nonlocal declaration stores its name as an Identifier, so this receives the removal action:

def f():
    x = 1
    def g():
        nonlocal x
    return g

Removing x = 1 makes the entire module fail to compile: SyntaxError: no binding for nonlocal 'x' found. Source tracing confirms the detector does not mark this declaration as a use; I verified the before/after compilation failure.

Fix: conservatively reject matching Stmt::Nonlocal declarations and add this regression. This is small enough for a maintainer fix-forward rather than another broad author revision.

[stroxler]

Again - let's close this, open a fresh PR, and address the one remaining edge case. I think it should be good to merge then. You can assign the fresh PR to me