impl implement codeaction/resolve #2540#2542
impl implement codeaction/resolve #2540#2542asukaminato0721 wants to merge 1 commit intofacebook:mainfrom
Conversation
|
According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅ |
There was a problem hiding this comment.
Pull request overview
Implements codeAction/resolve support in the non-wasm LSP server to make expensive refactors (notably introduce_parameter) lazy/resolved-on-demand, and reduces automatic code-action request cost by tightening refactor computation based on context.only and triggerKind.
Changes:
- Advertise
codeActionProvider.resolveProviderand handlecodeAction/resolveto resolveintroduce_parameteredits on demand. - Adjust code action kind filtering to properly respect hierarchical kinds in
context.only, and skip refactors for unfiltered automatic/unspecified-trigger requests. - Update LSP interaction tests to expect the new capability and set
triggerKindwhere refactors are required.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
pyrefly/lib/lsp/non_wasm/server.rs |
Adds resolve capability + request handling, introduces resolve data payload, refines kind filtering and automatic-request refactor skipping, and defers introduce_parameter edits to resolve. |
pyrefly/lib/state/lsp/quick_fixes/introduce_parameter.rs |
Adds a lightweight titles-only path for introduce_parameter and adjusts occurrence handling to avoid consuming ranges. |
pyrefly/lib/state/lsp.rs |
Exposes Transaction::introduce_parameter_action_titles for server use. |
pyrefly/lib/test/lsp/lsp_interaction/basic.rs |
Updates initialize-capabilities snapshot to include resolveProvider under codeActionProvider. |
pyrefly/lib/test/lsp/lsp_interaction/convert_module_package.rs |
Updates code action request contexts to include triggerKind: 1 so refactor actions are computed in tests. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } else if let Some(params) = as_request::<CodeActionResolveRequest>(&x) { | ||
| if let Some(params) = self | ||
| .extract_request_params_or_send_err_response::<CodeActionResolveRequest>( | ||
| params, &x.id, | ||
| ) | ||
| { | ||
| if let Some(CodeActionResolveData::IntroduceParameter { uri, .. }) = | ||
| code_action_resolve_data(¶ms) | ||
| { | ||
| self.set_file_stats(uri.clone(), telemetry_event); | ||
| } | ||
| let activity_key = telemetry_event.activity_key.as_ref(); |
There was a problem hiding this comment.
codeAction/resolve requests are currently subject to the generic “cancel on subsequent mutation” behavior because CodeActionResolveRequest::METHOD is not included in the ONLY_ONCE allowlist. Since resolve requests commonly refer to a code action computed for an earlier document version, canceling them on subsequent edits can cause the client to receive RequestCanceled and never get a resolved edit. Consider adding CodeActionResolveRequest::METHOD to ONLY_ONCE (similar to ResolveCompletionItem::METHOD).
| pub(crate) fn introduce_parameter_action_titles( | ||
| transaction: &Transaction<'_>, | ||
| handle: &Handle, | ||
| selection: TextRange, | ||
| ) -> Option<Vec<String>> { | ||
| let module_info = transaction.get_module_info(handle)?; | ||
| let ast = transaction.get_ast(handle)?; | ||
| let selection_text = validate_non_empty_selection(selection, module_info.code_at(selection))?; | ||
| let (_, expression_text, _, expression_range) = split_selection(selection_text, selection)?; | ||
| if !is_exact_expression(ast.as_ref(), expression_range) { | ||
| return None; | ||
| } | ||
| let function_ctx = find_function_context(ast.as_ref(), expression_range)?; |
There was a problem hiding this comment.
introduce_parameter_action_titles largely duplicates the validation / analysis logic from introduce_parameter_code_actions. This duplication makes it easy for the title computation and resolve computation to drift over time (e.g., producing titles that no longer match resolvable actions). Consider factoring out a shared helper that returns the computed context (function ctx, template, param_name, occurrence count, etc.) used by both paths.
| let template = | ||
| ExpressionTemplate::new(expression_text, expression_range, &name_refs, ¶m_names); | ||
| let base_name = suggest_parameter_name(ast.as_ref(), expression_range, ¶m_names); | ||
| let param_name = unique_name(&base_name, |name| param_names.contains(name)); | ||
| let occurrence_ranges = collect_matching_expression_ranges( | ||
| module_info.contents(), | ||
| &function_ctx.function_def.body, | ||
| &template.text, | ||
| ); | ||
| let mut titles = vec![format!("Introduce parameter `{param_name}`")]; | ||
| if occurrence_ranges.len() > 1 { | ||
| titles.push(format!( | ||
| "Introduce parameter `{param_name}` (replace all occurrences)" | ||
| )); | ||
| } |
There was a problem hiding this comment.
In introduce_parameter_action_titles, collect_matching_expression_ranges computes and allocates the full list of occurrence ranges, but the titles path only needs to know whether there is more than one occurrence. This can still be expensive for large function bodies / many matches. Consider adding a cheap “count up to 2” helper (or early-exit traversal) so listing only determines 0/1/many without materializing every range.
| if allows_kind(&CodeActionKind::REFACTOR_EXTRACT) { | ||
| let start = Instant::now(); | ||
| if let Some(titles) = transaction.introduce_parameter_action_titles(&handle, range) { | ||
| for title in titles { | ||
| let data = CodeActionResolveData::IntroduceParameter { | ||
| uri: uri.clone(), | ||
| range: lsp_range.clone(), | ||
| title: title.clone(), | ||
| }; | ||
| actions.push(CodeActionOrCommand::CodeAction(CodeAction { | ||
| title, | ||
| kind: Some(CodeActionKind::REFACTOR_EXTRACT), | ||
| data: Some( | ||
| serde_json::to_value(data) | ||
| .expect("introduce_parameter code action data is serializable"), | ||
| ), | ||
| ..Default::default() | ||
| })); |
There was a problem hiding this comment.
New behavior returns unresolved introduce_parameter code actions with data and relies on codeAction/resolve to populate edit. There doesn’t appear to be an LSP interaction test that exercises the full round-trip (list action -> resolve -> verify edit applied). Adding an integration test here would guard against regressions in serialization, request routing, and title matching.
Summary
Fixes #2540
Added codeAction/resolve support and advertised resolveProvider; introduce-parameter actions now return unresolved code actions with data and resolve to edits on demand.
Made refactor computation respect context.only and skip refactors for unfiltered requests without a triggerKind, reducing automatic-request cost.
Added introduce_parameter_action_titles to avoid expensive callsite edits during listing
Test Plan
updated LSP tests to expect resolveProvider and set triggerKind for refactor requests.