Fix instance __dict__ lookup with custom metaclasses - #11582
Conversation
vetrovk
changed the title
|
@microsoft-github-policy-service agree |
|
🔒 Automated review in progress — @rchiodo is auto-reviewing this PR. |
| // Skip the "type" class as an optimization because it is known to not | ||
| // define any instance variables, and it's by far the most common metaclass. | ||
| if (metaclass && isClass(metaclass) && !ClassType.isBuiltIn(metaclass, 'type')) { | ||
| if (isInstantiableClass(classType) && metaclass && isClass(metaclass) && !ClassType.isBuiltIn(metaclass, 'type')) { |
There was a problem hiding this comment.
Warning · Non-blocking recommendation
📍 packages/pyright-internal/src/analyzer/typeUtils.ts:1774
The fix itself is root-cause correct, but the Skeptic and Architect both note it narrows a shared hot function with ~50 call sites, so it silently corrects every instance-through-metaclass lookup (e.g. c.meta_attr, enum c.__members__) — not just __dict__. This is desirable, but it can newly surface errors in previously-passing sample files. PR validation ran only typeEvaluator3.test; please run the broader analyzer suites (typeEvaluator1/2/3, checker, protocols, enums) before merge to confirm no caller depended on the old (incorrect) metaclass leakage. [unverified] that the other suites pass.
[verified]
|
|
||
|
|
||
| def custom_func(a: CustomClass): | ||
| reveal_type(a.__dict__, expected_text="dict[str, Any]") |
There was a problem hiding this comment.
Warning · Non-blocking recommendation
📍 packages/pyright-internal/src/tests/samples/classes3.py:54
The added tests pin only the __dict__ dunder (class → MappingProxyType, instance → dict), but the actual invariant the guard enforces is the general rule that instances must not resolve arbitrary metaclass instance attributes. As the Skeptic notes, if someone later loosens the guard, only __dict__ is protected while c.meta_attr leakage would silently return. Consider adding a metaclass with a non-dunder attribute (meta_attr: int) and asserting CustomClass.meta_attr resolves to int while instance a.meta_attr errors.
[verified]
rchiodo
left a comment
rchiodo
left a comment
There was a problem hiding this comment.
Approved via Review Center.
rchiodo
added
the
review-auto:approved
2 participants
Fixes #11548.
Instances of classes with custom metaclasses could incorrectly resolve
**dict**through the metaclass and inherittype.**dict**, producingMappingProxyType[str, Any]instead of the instance dictionary type.Restrict metaclass member lookup to instantiable class objects. This preserves
MappingProxyType[str, Any]forClass.**dict**while restoringdict[str, Any]for instances.Tests cover both class-level and instance-level
**dict**access.Validation:
npx jest typeEvaluator3.test --forceExit --runInBandnpm run build:cli:devgit diff --check