[asukaminato0721]

Summary

Fixes part of #2207

The feature adds a static factory such as Service.create(*args, **kwargs) and rewrites constructor calls from Service(...) to Service.create(...).

Matching now distinguishes actual constructors from calls such as Service.some_classmethod(...); unsupported forms such as Box[int]() suppress the action to prevent an incomplete refactor.

Reference collection now uses reverse-dependency-based global references for workspace files.

Only uncovered in-memory modules are checked individually because unsaved editor buffers may not appear in the filesystem dependency graph.

This avoids querying local references for every loaded module while preserving edits in open files.

Test Plan

Added coverage for same-file rewrites, cross-module rewrites, unique-name fallback, and unsupported-call rejection.

[Copilot]

Pull request overview

Adds a new LSP refactor code action to “introduce factory” for a selected Python class, inserting a @staticmethod factory method and rewriting constructor call sites across loaded modules.

Changes:

• Implement introduce_factory quick-fix that inserts a static factory method with a unique name and rewrites constructor calls.
• Wire the new refactor into the Transaction API and the non-wasm LSP server refactor pipeline.
• Add LSP code-action tests for same-file rewrites, cross-module rewrites, unique-name fallback, and unsupported-call rejection.

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/introduce_factory.rs	New refactor implementation: find selected class, insert factory, rewrite constructor call sites across modules.
pyrefly/lib/state/lsp/quick_fixes/mod.rs	Exposes the new introduce_factory quick-fix module.
pyrefly/lib/state/lsp.rs	Adds Transaction::introduce_factory_code_actions entry point.
pyrefly/lib/lsp/non_wasm/server.rs	Wires the refactor into the server’s refactor action pipeline/timing.
pyrefly/lib/test/lsp/code_actions.rs	Adds coverage for introduce-factory code actions and edit application.

---

💡 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.

[asukaminato0721]

@jvansch1 this can be reviewed

[meta-codesync]

@kinto0 has imported this pull request. If you are a Meta employee, you can view this in D113596086.

[kinto0]

i'd love some details on what this feature is in the summary + some kind of description as to why you chose to implement it this way

[kinto0]

local references on every module? do we need this?

[kinto0]

still unclear. is this a performance issue?

[stroxler]

I'm closing this PR: it is too old and has a stale diff attached, which makes review hard.

I'm not actually confident that it's worth reopening given that agents don't seem to use these kinds of refactors much; the priority is much lower than it was when we first made the IDE features issue.

That said, if you do want to reopen it here are some AI review suggestions - the current implementation is probably not mergable:

---

Correctness (must fix)

1. The language server can crash. Files that aren't open in the editor normally don't keep a parsed copy, so if one of them calls the constructor, the expect("modules containing references must have an AST") at introduce_factory.rs:180 panics. This happens while the editor is still building the code-action menu, before anyone picks this refactor. I reproduced it with one open main.py and one closed file that calls Service(). Use the existing get_ast_or_parse_module helper instead, and add a test where the calling file isn't open. Also keep each module's original Handle rather than rebuilding it with the selected file's SysInfo.

2. The generated code can be invalid. The four-space indent is hardcoded. Both of these reproduced:

   • A class indented with two spaces gets a factory indented with four, which is an IndentationError.
   • A one-line class (class Service: pass) gets an indented method added after it, which is also invalid.

   Use the indentation the class body already has, and either expand one-line class bodies or don't offer the action for them.

3. Nested classes get a factory that fails at runtime. For Outer.Inner, the generated method body is return Inner(...), which raises NameError (reproduced). Don't offer the action for nested classes for now, or emit a name that resolves.

4. An inherited method can be silently replaced. The name check only looks at the class's own body. If Base.create exists, introducing a factory on Service(Base) picks create and shadows it, so the existing Service.create() call changes behavior (reproduced). Check inherited members before choosing the name.

5. Subclasses get the wrong type back. A @staticmethod that returns Service(...) makes Sub.create() return a Service, not a Sub. Use @classmethod with return cls(*args, **kwargs).

6. Type checking is lost at every rewritten call. With (*args, **kwargs), a checked Service(1, 2) becomes an unchecked forward. That's a big cost for a type checker's own refactor. At least annotate the return type; better, copy __init__'s parameters.

Tests

• introduce_factory_basic_refactor is missing #[test], so the main test never runs (confirmed).
• Put the new tests in a separate passing commit before the implementation, then add cases for each issue above.

Simplicity and performance

• constructor_call_kind and build_factory_call_edit match the same patterns, which makes the "rewritable but no edit" failed path unreachable. Merge them into one function that returns Unrelated | Unsupported | Rewrite(range, text).
• contains_point duplicates TextRange::contains; use that instead.
• Every right-click on a class name searches the whole workspace for references, scans every loaded file, and walks each matching file's syntax tree, all before the menu appears. This only runs when the user opens the menu, not on every keystroke, but it's heavier than safe_delete. Either defer the call-site rewrites to code-action resolve, or add a limit and benchmark it. Also, the check of every open file against every reference is O(files × references); build a set of paths first.
• One unsupported call such as Box[int]() blocks the whole action, so it's never available on generic classes that are used with explicit type arguments. Either rewrite these calls (Box[int].create()) or say clearly that they're out of scope.

PR description

• The summary should explain what the feature does for users and why it's designed this way, which is what kinto0 asked for. In particular, say why open files that haven't been saved need their own search, or drop that search if it doesn't affect correctness.
• Make the action's title say how many call sites and files will change.

Lower confidence (not reproduced): an open file with unsaved changes might get the same edit twice, once through the disk copy and once through the editor copy. One test would settle it.

[stroxler]

Whoops. I think this auto-closed when I closed the IDE features issue (which I did mean to do - I don't want that issue spinning off new work anymore - but I'm willing to try to land work already done)

[github-actions]

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