Repository navigation
fix empty case and dict case of key autocompletion #1886 #2838 - #2843
asukaminato0721 wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
Improves LSP autocompletion for TypedDict construction in Python code, especially when parsing incomplete dict literals that Ruff represents as sets (e.g. {""}), and when constructing dicts via dict(...) kwargs.
Changes:
- Treat Ruff’s
{""}error-recovery shape (ExprSet) as a dict-key placeholder to enable key completion. - Recover contextual
TypedDicttype information from surrounding assignment/call sites when the literal itself doesn’t infer as aTypedDict. - Add
dict(...)-constructor kwargs completions for contextualTypedDictfields, plus tests covering these cases.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
pyrefly/lib/state/lsp/dict_completions.rs |
Extends dict-key completion to handle ExprSet placeholders and adds contextual type recovery + dict(...) kwargs completion. |
pyrefly/lib/lsp/wasm/completion.rs |
Wires in the new dict(...)-constructor kwargs completion and exposes expected_call_argument_type for reuse. |
pyrefly/lib/test/lsp/completion.rs |
Adds regression tests for incomplete literal {""}, call-argument {""}, and dict( kwargs completion. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // For key access we query the container expression; for literals we query the | ||
| // literal itself to pick up contextual TypedDict typing from assignments. | ||
| if let Some(base_type) = self.get_type_trace(handle, context.base_range()) | ||
| if let Some(base_type) = | ||
| self.contextual_typed_dict_type(handle, module, position, context.base_range()) | ||
| && let Some(typed_keys) = self.collect_typed_dict_keys(handle, base_type) |
There was a problem hiding this comment.
contextual_typed_dict_type is now used for both dict literals and key-access contexts. For key access (u[""] / d[""]), falling back to the enclosing call/assignment can produce incorrect TypedDict key suggestions even when the base expression is not a TypedDict (e.g. cfg: Config = data[""] could start suggesting Config fields for the key into data). Consider restricting the call/assignment fallback to the dict-literal construction path only (or gating it behind context.base_expr().is_none()), and keep key-access completion typed keys derived solely from the base expression’s own type trace.
| let Some(call) = | ||
| Ast::locate_node(module, position) | ||
| .into_iter() | ||
| .find_map(|node| match node { | ||
| AnyNodeRef::ExprCall(call) | ||
| if call.arguments.range.contains_inclusive(position) | ||
| && matches!( | ||
| call.func.as_ref(), | ||
| Expr::Name(name) if name.id.as_str() == "dict" | ||
| ) => | ||
| { |
There was a problem hiding this comment.
add_typed_dict_constructor_kwargs_completions triggers solely on Expr::Name("dict"). This will also fire if dict is shadowed (e.g., a local function/variable named dict), producing TypedDict-field completions in calls that aren’t the built-in dict constructor. Consider resolving the callee via bindings/exports (or otherwise confirming it refers to builtins.dict) before adding these completions.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
808875c to
89b03eb
Compare
This comment has been minimized.
This comment has been minimized.
3cbe995 to
23015d2
Compare
stroxler
left a comment
There was a problem hiding this comment.
This PR needs a rebase before it could be merged.
In addition, there are a number of correctness issues we need to fix:
-
Dictionary completion becomes quadratic — [lines 715–716](
).pyrefly/pyrefly/lib/state/lsp/dict_completions.rs
Lines 715 to 716 in 23015d2
Each improved candidate clones the entire dictionary AST. Completing the last key in a 1,000-entry ordinary dictionary took ~375 ms versus ~1.4 ms on the parent, in the same local development build. Retain references while selecting the winner and clone once afterward. -
Real sets lose DataFrame column completions — [lines 651–658](
).pyrefly/pyrefly/lib/state/lsp/dict_completions.rs
Lines 651 to 658 in 23015d2
df.select({"|"})and pandasdf.filter(items={"|"})previously offered column names; now they offer none. The set matcher takes precedence over the DataFrame matcher even without a TypedDict context. Preserve that fallback unless TypedDict recovery actually applies. -
Expected types are applied to the wrong expression — [lines 948–953](
).pyrefly/pyrefly/lib/state/lsp/dict_completions.rs
Lines 948 to 953 in 23015d2
Withlookup: dict[str, Config],take(lookup["|"])incorrectly offers Config’s fields as lookup keys. Nested literals also pick up outer TypedDict fields. Expected-type recovery needs to belong to the constructed expression, not merely the cursor’s enclosing call. -
Constructor kwargs target unrelated calls — [lines 1180–1186](
).pyrefly/pyrefly/lib/state/lsp/dict_completions.rs
Lines 1180 to 1186 in 23015d2
cfg: Config = dict(name=str(|))offersname=andage=insidestr(). A user-defined function nameddictalso receives those suggestions despite not accepting them. Require the innermost owning call and resolve its callee to builtindict. -
Invalid keyword names are emitted — [lines 1203–1210](
).pyrefly/pyrefly/lib/state/lsp/dict_completions.rs
Lines 1203 to 1210 in 23015d2
Functional TypedDict fields such as"class"and"two words"produceclass=andtwo words=, neither valid Python. Route these through the existingpush_kwarg_completionvalidation.
|
According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅ |
stroxler
left a comment
There was a problem hiding this comment.
Review automatically exported from Phabricator review in Meta.
|
This pull request has been merged in cf94755. |
Summary
Fixes #1886
fixes #2838
The dict-key matcher now treats Ruff’s error-recovery {""} shape as a placeholder key position, then recovers the contextual TypedDict from the enclosing assignment/call when the literal itself only infers as set[str].
also added a specialized dict(...) kwargs path so contextual TypedDict fields show up as name= / age= completions in the existing kwargs flow.
Test Plan
add test