impl Implemented encapsulate_field #2207 by asukaminato0721 · Pull Request #2691 · facebook/pyrefly

asukaminato0721 · 2026-03-06T16:47:27Z

Summary

Fixes part of #2207

Implemented encapsulate_field as a new local rewrite refactor. It now offers a code action on attribute selections, inserts unique get_<field> / set_<field> methods, rewrites reads to getter calls, rewrites assignments and += to setter calls, and rejects unsupported tuple-assignment targets. It’s wired into the transaction and LSP code-action pipeline.

ref https://rope.readthedocs.io/en/latest/library.html#list-of-refactorings

Test Plan

Added focused tests for read/write rewriting, unique accessor naming, and tuple-target rejection.

Copilot

Pull request overview

Adds a new local “Encapsulate field” refactor to the Pyrefly LSP pipeline, generating getter/setter methods on a class field and rewriting local reads/writes to use those accessors.

Changes:

Implemented encapsulate_field local refactor (generate accessor methods + rewrite reads/writes and +=).
Wired the refactor into Transaction and the non-wasm LSP server code-action path.
Added focused LSP code-action tests for read/write rewriting, unique naming, and tuple-target rejection.

Reviewed changes

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

Show a summary per file

File	Description
`pyrefly/lib/state/lsp/quick_fixes/encapsulate_field.rs`	New refactor implementation (edits generation + method insertion).
`pyrefly/lib/state/lsp/quick_fixes/mod.rs`	Exposes the new quick-fix module.
`pyrefly/lib/state/lsp.rs`	Adds `Transaction::encapsulate_field_code_actions` entrypoint.
`pyrefly/lib/lsp/non_wasm/server.rs`	Adds the refactor to the server’s timed refactor action pipeline.
`pyrefly/lib/test/lsp/code_actions.rs`	Adds tests for the new refactor behavior.

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

You can also share your feedback on Copilot code review. Take the survey.

Copilot · 2026-03-06T17:50:18Z

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

+    for reference in references {
+        if handled_stmt_ranges
+            .iter()
+            .any(|stmt_range: &TextRange| stmt_range.contains(reference.start()))
+        {
+            continue;
+        }
+        let occurrence = classify_occurrence(ast.as_ref(), reference)?;
+        if reference == definition.definition_range && occurrence.is_definition_write() {
+            continue;
+        }


handled_stmt_ranges is checked before classifying the occurrence, so once a write in a statement is handled (usually the LHS, since references are sorted by start position), all later references in the same statement are skipped. This leaves un-rewritten reads in RHS expressions (e.g. self.value = self.value becomes self.set_value(self.value)), breaking encapsulation. Consider classifying first, then only dedup write occurrences per statement (or track handled write-target ranges instead of entire statement ranges).

Copilot · 2026-03-06T17:50:18Z

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

+    let definition = transaction
+        .find_definition(handle, position, FindPreference::default())
+        .into_iter()
+        .find(|definition| {
+            definition.module.path() == module_info.path()
+                && matches!(
+                    definition.metadata,
+                    DefinitionMetadata::Attribute | DefinitionMetadata::VariableOrAttribute(_)
+                )
+                && !matches!(
+                    definition.metadata.symbol_kind(),
+                    Some(
+                        SymbolKind::Module
+                            | SymbolKind::Function
+                            | SymbolKind::Method
+                            | SymbolKind::Class
+                    )
+                )
+        })?;
+    let ast = transaction.get_ast(handle)?;
+    let class_def = enclosing_class(ast.as_ref(), definition.definition_range)?;


The definition filter attempts to exclude methods/functions/classes via definition.metadata.symbol_kind(), but DefinitionMetadata::Attribute always reports SymbolKind::Attribute, even when the resolved attribute is actually a method/property. As a result, this refactor can be offered on obj.method and will rewrite obj.method() into something invalid like obj.get_method()(). Suggest explicitly rejecting attribute definitions whose definition_range is within a Stmt::FunctionDef/Stmt::ClassDef (via Ast::locate_node), or otherwise verifying the definition comes from an assignment/AnnAssign to a field rather than a callable/class member.

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

Copilot · 2026-03-06T17:50:19Z

pyrefly/lib/test/lsp/code_actions.rs

+        assert_eq!(expected.trim(), updated.trim());
+    }
+
+    #[test]


Current tests don’t cover the case where a write statement also contains reads of the same attribute (e.g. self.value = self.value or self.value += self.value). Given references are processed in source order, this scenario currently risks leaving RHS reads un-rewritten; adding a regression test here would prevent this from reappearing.

Suggested change

#[test]

#[test]

fn encapsulate_field_rewrites_rhs_reads_in_write() {

let code = r#"

class Counter:

def __init__(self, value):

self.value = value

def copy(self):

self.value = self.value

# ^

def double(self):

self.value += self.value

"#;

let updated =

apply_first_encapsulate_field_action(code).expect("expected encapsulate field action");

let expected = r#"

class Counter:

def __init__(self, value):

self.value = value

def copy(self):

self.set_value(self.get_value())

def double(self):

self.set_value(self.get_value() + self.get_value())

def get_value(self):

return self.value

def set_value(self, value):

self.value = value

# ^

"#;

assert_eq!(expected.trim(), updated.trim());

}

#[test]

Copilot · 2026-03-06T17:50:19Z

pyrefly/lib/test/lsp/code_actions.rs

+#            ^
+"#;
+        assert_no_encapsulate_field_action(code);
+    }


Consider adding a test ensuring encapsulate-field is not offered for method attributes (e.g. cursor on obj.method / obj.method()), since the refactor currently treats any resolved DefinitionMetadata::Attribute as a field and could generate invalid rewrites for callables/properties.

Suggested change

}

}

#[test]

fn encapsulate_field_not_offered_for_method_attribute() {

let code = r#"

class Counter:

def __init__(self, value):

self.value = value

def inc(self):

self.value += 1

def use(counter: Counter):

method_ref = counter.inc

# ^

"#;

assert_no_encapsulate_field_action(code);

}

#[test]

fn encapsulate_field_not_offered_for_method_call() {

let code = r#"

class Counter:

def __init__(self, value):

self.value = value

def inc(self):

self.value += 1

def use(counter: Counter):

counter.inc()

# ^

"#;

assert_no_encapsulate_field_action(code);

}

• Implemented encapsulate_field as a new local rewrite refactor in encapsulate_field.rs:42. It now offers a code action on attribute selections, inserts unique get_<field> / set_<field> methods, rewrites reads to getter calls, rewrites assignments and += to setter calls, and rejects unsupported tuple-assignment targets. It’s wired into the transaction and LSP code- action pipeline in lsp.rs:2352 and server.rs:4093. Added focused tests in code_actions.rs:3599 for read/write rewriting, unique accessor naming, and tuple-target rejection.

github-actions · 2026-03-06T20:43:33Z

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

meta-cla bot added the cla signed label Mar 6, 2026

This comment has been minimized.

Sign in to view

asukaminato0721 marked this pull request as ready for review March 6, 2026 17:44

Copilot AI review requested due to automatic review settings March 6, 2026 17:44

Copilot started reviewing on behalf of asukaminato0721 March 6, 2026 17:44 View session

Copilot AI reviewed Mar 6, 2026

View reviewed changes

asukaminato0721 marked this pull request as draft March 6, 2026 19:20

asukaminato0721 force-pushed the 2207-encapsulate_field branch from 4897879 to 1af67f5 Compare March 6, 2026 19:44

copilot

4aed48d

asukaminato0721 marked this pull request as ready for review March 6, 2026 19:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

impl Implemented encapsulate_field #2207#2691

impl Implemented encapsulate_field #2207#2691
asukaminato0721 wants to merge 2 commits intofacebook:mainfrom
asukaminato0721:2207-encapsulate_field

asukaminato0721 commented Mar 6, 2026 •

edited

Loading

Uh oh!

This comment has been minimized.

Copilot AI left a comment

Uh oh!

Copilot AI Mar 6, 2026

Uh oh!

Copilot AI Mar 6, 2026

Uh oh!

Uh oh!

Copilot AI Mar 6, 2026

Uh oh!

Copilot AI Mar 6, 2026

Uh oh!

github-actions bot commented Mar 6, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

-    #[test]
+    #[test]
+    fn encapsulate_field_rewrites_rhs_reads_in_write() {
+        let code = r#"
+class Counter:
+    def __init__(self, value):
+        self.value = value
+    def copy(self):
+        self.value = self.value
+#                    ^
+    def double(self):
+        self.value += self.value
+"#;
+        let updated =
+            apply_first_encapsulate_field_action(code).expect("expected encapsulate field action");
+        let expected = r#"
+class Counter:
+    def __init__(self, value):
+        self.value = value
+    def copy(self):
+        self.set_value(self.get_value())
+    def double(self):
+        self.set_value(self.get_value() + self.get_value())
+    def get_value(self):
+        return self.value
+    def set_value(self, value):
+        self.value = value
+#                   ^
+"#;
+        assert_eq!(expected.trim(), updated.trim());
+    }
+    #[test]

Conversation

asukaminato0721 commented Mar 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test Plan

Uh oh!

This comment has been minimized.

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Mar 6, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 6, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI Mar 6, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 6, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Mar 6, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

asukaminato0721 commented Mar 6, 2026 •

edited

Loading