impl invert boolean #2060
impl invert boolean #2060
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request implements an "Invert boolean" refactoring feature that flips a local variable's stored boolean value and updates all usages across the file. The refactor adds/removes not operators at load sites and inverts boolean literals in assignments, with conservative guards against multi-target assignments and deletions.
Changes:
- Implemented core invert boolean refactoring logic with support for boolean literals, unary not expressions, and complex expressions
- Added LSP integration to expose the refactoring as a
REFACTOR_REWRITEcode action - Added basic test coverage for the new refactoring feature
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| pyrefly/lib/state/lsp/quick_fixes/invert_boolean.rs | New module implementing the invert boolean refactoring logic including AST traversal, edit collection, and safety checks |
| pyrefly/lib/state/lsp/quick_fixes/mod.rs | Registers the new invert_boolean module |
| pyrefly/lib/state/lsp.rs | Adds public API method for accessing invert boolean code actions |
| pyrefly/lib/lsp/non_wasm/server.rs | Integrates invert boolean into LSP code action handler and registers REFACTOR_REWRITE capability |
| pyrefly/lib/test/lsp/code_actions.rs | Adds test helper functions and three test cases for invert boolean functionality |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| range: unary.range(), | ||
| replacement: name.id.to_string(), | ||
| }); | ||
| return; |
There was a problem hiding this comment.
The early return on line 265 prevents the visitor from walking nested expressions when a "not variable" pattern is found. This means that if the variable appears within nested expressions under a unary not operation, those occurrences won't be processed. The walk_expr call should be executed before the return statement, or the return should be removed to allow proper tree traversal.
| #[test] | ||
| fn invert_boolean_basic_refactor() { | ||
| let code = r#" | ||
| def foo(): | ||
| abc = True | ||
| return abc | ||
| "#; | ||
| let selection = find_nth_range(code, "abc", 2); | ||
| let updated = | ||
| apply_first_invert_boolean_action(code, selection).expect("expected invert-boolean action"); | ||
| let expected = r#" | ||
| def foo(): | ||
| abc = False | ||
| return (not abc) | ||
| "#; | ||
| assert_eq!(expected.trim(), updated.trim()); | ||
| } | ||
|
|
||
| #[test] | ||
| fn invert_boolean_removes_not() { | ||
| let code = r#" | ||
| def foo(): | ||
| abc = False | ||
| if not abc: | ||
| return 1 | ||
| return 0 | ||
| "#; | ||
| let selection = find_nth_range(code, "abc", 2); | ||
| let updated = | ||
| apply_first_invert_boolean_action(code, selection).expect("expected invert-boolean action"); | ||
| let expected = r#" | ||
| def foo(): | ||
| abc = True | ||
| if abc: | ||
| return 1 | ||
| return 0 | ||
| "#; | ||
| assert_eq!(expected.trim(), updated.trim()); | ||
| } | ||
|
|
||
| #[test] | ||
| fn invert_boolean_rejects_multi_target_assignment() { | ||
| let code = r#" | ||
| def foo(): | ||
| abc = other = True | ||
| return abc | ||
| "#; | ||
| let selection = find_nth_range(code, "abc", 2); | ||
| assert_no_invert_boolean_action(code, selection); | ||
| } |
There was a problem hiding this comment.
Consider adding test coverage for additional scenarios: 1) annotated assignments (e.g., abc: bool = True), 2) variables that are deleted (the code rejects these but there's no test), 3) multiple assignments to the same variable, 4) nested expressions containing the target variable (e.g., return not (abc and other)), and 5) assignments with complex expressions like abc = not other_var. These tests would help ensure the implementation handles all code paths correctly.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Co-authored-by: Copilot <[email protected]> . fmt
asukaminato0721
force-pushed
the
364-4
branch
from
6c52d92 to
cb56106
Compare
This comment has been minimized.
This comment has been minimized.
|
According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅ |
2 participants
Summary
Fixes part of #364
Implemented an “Invert boolean” refactor code action that flips a local variable’s stored boolean value and updates all load sites (adds/removes not) across the file, with conservative guards against multi-target assignments and deletes. Wired it into the LSP code action pipeline as a refactor.rewrite.
Test Plan
added focused LSP code action tests.