[ty] Add support for dict literals and dict() calls as default values for parameters with TypedDict types#22161
Conversation
Typing conformance resultsNo changes detected ✅ |
|
|
The ecosystem results look great! I wouldn't worry about the new diagnostics on |
Hugo-Polloli
force-pushed
the
typed-dict-as-call-parameter
branch
from
7c98f57 to
59b330f
Compare
Hugo-Polloli
requested review from
AlexWaygood,
carljm,
dcreager and
sharkdp
as code owners
Thanks ! I checked too and from my understanding things are looking ok :) |
|
Thank you for working on this. Almost the entire team is out this week. It may take a few days before someone finds time to review your PR. Happy holidays. |
Hugo-Polloli
force-pushed
the
typed-dict-as-call-parameter
branch
from
59b330f to
b381672
Compare
|
Rebased on main as the branch was getting stale, but it looks like everything is still fine with no conflict/regression, so this PR is ready |
| self.index.try_expression(default_expr).unwrap_or_else(|| { | ||
| // Default expressions aren't registered as standalone expressions, so | ||
| // synthesize an `Expression` to allow type-context inference. | ||
| Expression::new( |
There was a problem hiding this comment.
Why do we need to perform standalone inference here? You can infer an expression with type context using self.infer_expression.
There was a problem hiding this comment.
I started with self.infer_expression, but parameter defaults aren't standalone expressions in the semantic index, so it panics with no entry found for key.
The workaround I found is to synthesize an Expression and run standalone inference in the default's enclosing scope.
Here's the test file I used:
class Foo(TypedDict):
x: int
def ok(default_x: Foo = {"x": 42}): ... # all good
outer_default_x = 42
def not_ok(default_x: Foo = {"x": outer_default_x}): ... # panicsI'm unsure that my fix is the correct one, I just know I could not get infer_expression to work in this case, though I agree it would be cleaner if we could make it work here, I'm open to suggestions/learning about things I missed!
Also in the meantime I pushed a diff that extract this custom handling to a file_expression_type_with_context function with comments explaining the reasoning if that can help
There was a problem hiding this comment.
parameter defaults aren't standalone expressions in the semantic index
It sounds like you tried infer_standalone_expression, I don't think infer_expression has that panic path?
There was a problem hiding this comment.
oof sorry, "standalone expression" was rly confusing wording on my part! 🫠 I didn't mean I used infer_standalone_expression, I tried infer_expression.
The issue is scope, infer_parameter_definition runs with the function body scope, but the parameter defaults are indexed in the enclosing scope. infer_expression resolves names using the current scope’s ast_ids map. So when defaults reference names from the outer scope (like outer_default_x in my above example), the lookup hits ast_ids.use_id(...) and this is where we panic with no entry found for key.
I hope it makes better sense now?
Hugo-Polloli
force-pushed
the
typed-dict-as-call-parameter
branch
from
b381672 to
6fe7de9
Compare
| .map(|default| self.file_expression_type(default)); | ||
| let default_expr = default.as_ref(); | ||
| if let Some(annotation) = parameter.annotation.as_ref() { | ||
| let declared_ty = self.file_expression_type(annotation); |
There was a problem hiding this comment.
We infer the parameter default as part of the outer scope here. Inferring them multiple times means that the original inferred type will still be displayed, e.g., by the IDE on hover. We should try to infer the value directly with type context in its scope.
I think what this requires is creating a deferred function scope if the function has default value expressions, even if it has type parameters, and then infer default values in infer_function_deferred with the parameter annotations. Note that if there are type parameters, the only thing that should be inferred in infer_function_deferred is the default values, and you should call infer_scope_types on the type-params scope to get the parameter annotation types to use as type context.
There was a problem hiding this comment.
Hi thank you so much, I completely missed that, I've started working on this and have set this PR as draft for now
There was a problem hiding this comment.
Okay, things are much better now
I hope what I did is what you had in mind! Defaults are now inferred once in the deferred pass with the parameter annotation as type context (for generic functions, we pull annotations from the type‑params scope).
An added benefit of this method is that we replace generic invalid-parameter-default errors by more explicit TypedDict specific errors, I updated the mdtest to reflect that :)
Hugo-Polloli
force-pushed
the
typed-dict-as-call-parameter
branch
from
6fe7de9 to
039b775
Compare
Hugo-Polloli
force-pushed
the
typed-dict-as-call-parameter
branch
from
039b775 to
48b2666
Compare
… for parameters with TypedDict types
Hugo-Polloli
force-pushed
the
typed-dict-as-call-parameter
branch
from
48b2666 to
63e2872
Compare
|
This looks great, thanks! |
* main: (62 commits) [`refurb`] Do not add `abc.ABC` if already present (`FURB180`) (#22234) [ty] Add a new `assert-type-unspellable-subtype` diagnostic (#22815) [ty] Avoid duplicate syntax errors for `await` outside functions (#22826) [ty] Fix unary operator false-positive for constrained TypeVars (#22783) [ty] Fix binary operator false-positive for constrained TypeVars (#22782) [ty] Fix false-positive `unsupported-operator` for "symmetric" TypeVars (#22756) [`pydocstyle`] Clarify which quote styles are allowed (`D300`) (#22825) [ty] Use distributed versions of AND and OR on constraint sets (#22614) [ty] Add support for dict literals and dict() calls as default values for parameters with TypedDict types (#22161) Document `-` stdin convention in CLI help text (#22817) [ty] Make `infer_subscript_expression_types` a method on `Type` (#22731) [ty] Simplify `OverloadLiteral::spans` and `OverloadLiteral::parameter_span` (#22823) [ty] Require both `*args` and `**kwargs` when calling a `ParamSpec` callable (#22820) [ty] Handle tagged errors in conformance (#22746) Add `--color` cli option to force colored output (#22806) Identify notebooks by LSP didOpen instead of `.ipynb` file extension (#22810) [ty] Fix docstring rendering for literal blocks after doctests (#22676) [ty] Update salsa to fix out-of-order query validation (#22498) [ty] Inline cycle initial and recovery functions (#22814) [ty] Pass the generic context through the decorator (#22544) ...
4 participants
Summary
Fixes astral-sh/ty#2161
invalid-parameter-defaultfalse positives forTypedDictdefaults (literals{...}and callsdict(...)).TypedDictdefaults plus a non-TypedDictassignability case.Test Plan
Assert no diagnostics for
Foodefaults via{"x": 42}anddict(x=42)Assert
invalid-parameter-defaultfor missing key, wrong value type, extra key inFoodefaults and non-assignable default (tuple[int] = ())