fix dict ** unpacking does not respect descriptor #2751#2760
fix dict ** unpacking does not respect descriptor #2751#2760asukaminato0721 wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Pull request overview
Fixes a false positive in **expr unpacking type checks by allowing mapping checks to respect descriptor __get__ results for attribute-access expressions, matching behavior of other type checkers and addressing issue #2751.
Changes:
- Add
infer_unpack_mapping_exprto infer mapping-appropriate types for**expr, with special handling for descriptor-typed annotation-only attributes. - Route dict-literal unpack (
{**x}) and call-site**kwargsunpacking through the new inference path. - Add regression tests for both dict unpacking and
**kwargsunpacking with descriptor-backed attributes.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| pyrefly/lib/test/simple.rs | Adds regression test for dict literal **model.data where data is a descriptor returning a dict. |
| pyrefly/lib/test/callable.rs | Adds regression test for f(**model.data) where data is a descriptor returning a dict. |
| pyrefly/lib/alt/expr.rs | Uses the new unpack-mapping inference for dict unpack items. |
| pyrefly/lib/alt/class/class_field.rs | Adds helper to resolve descriptor __get__ result starting from a raw field type. |
| pyrefly/lib/alt/callable.rs | Uses the new unpack-mapping inference for **kwargs splats when the value is an expression. |
| pyrefly/lib/alt/attr.rs | Implements unpack-specific descriptor-aware mapping inference logic. |
💡 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.
| let base_ty = self.expr_infer(&attr.value, errors); | ||
| let mut descriptor_tys = Vec::new(); | ||
| for (found, on) in self.lookup_attr(&base_ty, &attr.attr.id).found { | ||
| let Some(base) = Self::descriptor_base_for_unpacking(on) else { | ||
| continue; | ||
| }; | ||
| let raw_ty = match found { | ||
| Attribute::ClassAttribute(ClassAttribute::ReadWrite(raw_ty)) | ||
| | Attribute::ClassAttribute(ClassAttribute::ReadOnly(raw_ty, _)) => raw_ty, | ||
| _ => continue, | ||
| }; | ||
| if let Some(descriptor_ty) = self.resolve_descriptor_get_from_raw_type( | ||
| &attr.attr.id, | ||
| &raw_ty, | ||
| base, | ||
| x.range(), | ||
| errors, | ||
| ) && (matches!(descriptor_ty, Type::TypedDict(_)) | ||
| || self.unwrap_mapping(&descriptor_ty).is_some()) | ||
| { | ||
| descriptor_tys.push(descriptor_ty); | ||
| } | ||
| } | ||
| if descriptor_tys.is_empty() { | ||
| ty | ||
| } else { | ||
| self.unions(descriptor_tys) | ||
| } |
There was a problem hiding this comment.
infer_unpack_mapping_expr builds descriptor_tys by collecting only descriptor getter result types that look like mappings, and then returns unions(descriptor_tys). This drops any non-descriptor / non-mapping possibilities coming from attribute lookup (e.g. when the base expression is a union), which can incorrectly allow **expr unpacking even if some possible attribute types are not mappings. Consider constructing the result per-found attribute type (use the resolved descriptor-get type when available, otherwise keep the original raw attribute type) so that unpacking remains rejected unless all possibilities behave like mappings.
This comment has been minimized.
This comment has been minimized.
|
@grievejia has imported this pull request. If you are a Meta employee, you can view this in D95972229. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅ |
Summary
Fixes #2751
**exprmapping checks now look through a descriptor getter when expr is an attribute access whose raw field typedefines
__get__, but only for unpacking contexts.Test Plan
add test