fix use all usages within class (or within __init__) to infer type, not just initial assignment #1159#2356
fix use all usages within class (or within __init__) to infer type, not just initial assignment #1159#2356asukaminato0721 wants to merge 3 commits into
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Pull request overview
This PR updates Pyrefly’s class attribute inference so that assignments to self.<attr> inside class methods can contribute to the inferred attribute type, avoiding type errors when a class-body attribute is later assigned a different type in a method (Issue #1159).
Changes:
- Collect and retain all method self-assignments per attribute during class-scope finalization.
- Plumb collected method assignments through
BindingClassFieldinto the solver and widen inferred attribute types by unioning class-body type with method-assigned types (when safe). - Add regression tests validating class-body + method assignment widening to a union type.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
pyrefly/lib/test/attributes.rs |
Adds tests for widening class-body attributes based on method assignments. |
pyrefly/lib/binding/scope.rs |
Collects method-defined attribute assignments and associates them with class fields. |
pyrefly/lib/binding/class.rs |
Stores per-field method assignment metadata into BindingClassField. |
pyrefly/lib/binding/binding.rs |
Extends BindingClassField to include method assignments; adds MethodDefinedAttribute type; updates size asserts. |
pyrefly/lib/alt/solve.rs |
Passes method assignment metadata into class-field solving. |
pyrefly/lib/alt/class/class_field.rs |
Unions class-body inferred types with method-assignment inferred types under certain conditions. |
Comments suppressed due to low confidence (1)
pyrefly/lib/binding/scope.rs:2136
class_body.kindis moved when matchingif let ScopeKind::Class(class_scope) = class_body.kind, butclass_bodyis then used again (e.g.,class_body.stat/class_body.flow). This will not compile due to a partial move. Restructure this to avoid moving a single field out ofclass_body(e.g., destructureclass_bodyintostat/flow/kindfirst, or match on a reference tokind).
let class_body = self.pop();
let class_scope = {
if let ScopeKind::Class(class_scope) = class_body.kind {
class_scope
} else {
unreachable!("Expected class body scope, got {:?}", class_body.kind)
}
};
self.pop(); // Also pop the annotation scope that wrapped the class body.
class_body.stat.0.iter_hashed().for_each(
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| testcase!( | ||
| test_infers_attribute_union_from_class_and_method_assignments, | ||
| r#" | ||
| from typing import assert_type | ||
|
|
||
| class A: | ||
| value = 1 | ||
|
|
||
| def promote(self, flag: bool) -> None: | ||
| if flag: | ||
| self.value = "a" | ||
|
|
||
| def takes(a: A) -> None: | ||
| assert_type(a.value, int | str) | ||
| "#, | ||
| ); | ||
|
|
There was a problem hiding this comment.
The PR adds coverage for class-body attribute + method reassignment, but there’s still no test exercising the “within init” case (attribute first set in __init__, then assigned a different type in another method) to ensure the new method_assignments plumbing actually widens the inferred attribute type as intended.
| /// The resulting map of field definitions: | ||
| /// - Includes both fields defined in the class body and implicit definitions | ||
| /// coming from self-assignment in methods. If both occur, only the class body | ||
| /// definition is tracked. | ||
| /// - Panics if the current scope is not a class body. |
There was a problem hiding this comment.
The doc comment says that when both a class-body definition and method self-assignments exist, “only the class body definition is tracked”, but the implementation now also tracks method_assignments and merges them into the returned map. Update the comment to reflect the new behavior to avoid misleading future changes.
| class, | ||
| name, | ||
| direct_annotation.as_ref(), | ||
| true, |
There was a problem hiding this comment.
When analyzing each method_assignments value, this passes inferred_from_method = true into analyze_class_field_value. If the attribute exists on an ancestor, analyze_class_field_value will return the inherited type and skip analyzing the assigned value, which can prevent the union from widening to include the method-assigned type. For widening, the method assignment’s actual value type should be analyzed (i.e., avoid the inherited-type short-circuit).
| true, | |
| false, |
|
interesting... it does end up adding more errors than it removes so the argument of "widen type instead of erroring" might not hold. let's see what other people think |
|
Hmm. I'm not convinced this is a good idea. Looking at the proposed new behavior: this strikes me as potentially surprising behavior (if |
rchen152
left a comment
There was a problem hiding this comment.
(Leaving a comment to clear this out from my review queue.)
|
we should experiment with only allowing this in |
6e25e96 to
cde1201
Compare
This comment has been minimized.
This comment has been minimized.
cde1201 to
519c85e
Compare
This comment has been minimized.
This comment has been minimized.
|
@yangdanny97 done |
|
thanks @asukaminato0721 odd, the mypy primer delta is more mixed than i would expect @migeed-z can you run the classifier on this one? |
This comment has been minimized.
This comment has been minimized.
|
wow that's a long comment lol, i'll read tomorrow and see if it's legit |
|
I think the regressions are mostly caused by something being initialized both on the class & in the constructor. I wonder if we should only take the union of assignments in The downside is that it wouldn't be able to handle things like this. We currently have a special-case for |
|
@yangdanny97 has imported this pull request. If you are a Meta employee, you can view this in D94676249. |
|
ok, i tried a few tweaks on top of this, but the delta still does not look super good. the LLM classifier output is somewhat nonsensical, which confused me for a bit. I'm going to attempt a different approach that synthesizes fake variables for each attribute & infers the type based on their flow info at exit points to |
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.
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.
Primer Diff Classification❌ 1 regression(s) | ✅ 5 improvement(s) | ➖ 1 neutral | 7 project(s) total | +17, -8 errors 1 regression(s) across mkdocs. error kinds:
Detailed analysis❌ Regression (1)mkdocs (+5)
All 5 errors are cascade false positives from this single
Neither mypy nor pyright reports any of these errors, confirming they are false positives caused by pyrefly's incorrect handling of
✅ Improvement (5)bokeh (-1)
apprise (+5, -6)
aiohttp (+1)
strawberry (+4)
core (+1)
➖ Neutral (1)dd-trace-py (+1, -1)
Suggested fixesSummary: The PR's new logic for unioning class body field types with init assignments fails to resolve 1. In the new init_assignments processing block in
Was this helpful? React with 👍 or 👎 Classification by primer-classifier (1 heuristic, 6 LLM) |
|
Diff from mypy_primer, showing the effect of this PR on open source code: mkdocs (https://github.com/mkdocs/mkdocs)
+ ERROR mkdocs/structure/files.py:261:32-47: No matching overload found for function `posixpath.normpath` called with arguments: (((self: Self@File, use_directory_urls: bool | None = None) -> str) | str) [no-matching-overload]
+ ERROR mkdocs/structure/files.py:395:44-49: No matching overload found for function `posixpath.split` called with arguments: (((self: Self@File, use_directory_urls: bool | None = None) -> str) | str) [no-matching-overload]
+ ERROR mkdocs/structure/files.py:400:24-29: No matching overload found for function `urllib.parse.quote` called with arguments: (((self: Self@File, use_directory_urls: bool | None = None) -> str) | str | Unknown) [no-matching-overload]
+ ERROR mkdocs/structure/files.py:419:45-75: No matching overload found for function `posixpath.join` called with arguments: (str, ((self: Self@File, use_directory_urls: bool | None = None) -> str) | str) [no-matching-overload]
+ ERROR mkdocs/structure/files.py:564:49-70: No matching overload found for function `typing.MutableMapping.setdefault` called with arguments: (((self: File, use_directory_urls: bool | None = None) -> str) | str, File) [no-matching-overload]
bokeh (https://github.com/bokeh/bokeh)
- ERROR src/bokeh/models/annotations/legends.py:319:26-43: `Value[NullStringSpec & str]` is not assignable to attribute `label` with type `NullStringSpec` [bad-assignment]
apprise (https://github.com/caronc/apprise)
- ERROR apprise/asset.py:275:21-277:67: `NotifyFormat | PersistentStoreMode` is not assignable to attribute `__storage_mode` with type `PersistentStoreMode` [bad-assignment]
+ ERROR apprise/asset.py:523:16-35: Returned type `NotifyFormat | PersistentStoreMode` is not assignable to declared return type `PersistentStoreMode` [bad-return]
- ERROR apprise/asset.py:539:16-28: Returned type `Unknown | None` is not assignable to declared return type `tzinfo` [bad-return]
+ ERROR apprise/asset.py:539:16-28: Returned type `tzinfo | Unknown | None` is not assignable to declared return type `tzinfo` [bad-return]
- ERROR apprise/config/base.py:166:29-51: `object` is not assignable to attribute `encoding` with type `str` [bad-assignment]
+ ERROR apprise/config/file.py:121:22-57: No matching overload found for function `open` called with arguments: (Unknown, encoding=object | str) [no-matching-overload]
- ERROR apprise/plugins/base.py:566:33-60: `Unknown | None` is not assignable to attribute `interpret_emojis` with type `bool` [bad-assignment]
- ERROR apprise/plugins/kodi.py:322:17-59: `<=` is not supported between `tuple[Literal['kodi'], Literal['xbmc']]` and `int` [unsupported-operation]
+ ERROR apprise/plugins/kodi.py:322:17-59: `<=` is not supported between `tuple[str, str]` and `int` [unsupported-operation]
+ ERROR apprise/plugins/kodi.py:322:17-59: `<=` is not supported between `None` and `int` [unsupported-operation]
- ERROR apprise/plugins/kodi.py:370:17-59: `<=` is not supported between `tuple[Literal['kodi'], Literal['xbmc']]` and `int` [unsupported-operation]
+ ERROR apprise/plugins/kodi.py:370:17-59: `<=` is not supported between `tuple[str, str]` and `int` [unsupported-operation]
+ ERROR apprise/plugins/kodi.py:370:17-59: `<=` is not supported between `None` and `int` [unsupported-operation]
aiohttp (https://github.com/aio-libs/aiohttp)
+ ERROR aiohttp/connector.py:339:43-65: `StackSummary | Unknown` is not assignable to dict key `source_traceback` with type `list[str] | str | Self@BaseConnector` [bad-assignment]
strawberry (https://github.com/strawberry-graphql/strawberry)
+ ERROR strawberry/aiohttp/views.py:88:5-26: Class member `GraphQLView.allow_queries_via_get` overrides parent class `AsyncBaseHTTPView` in an inconsistent manner [bad-override]
+ ERROR strawberry/asgi/__init__.py:100:5-26: Class member `GraphQL.allow_queries_via_get` overrides parent class `AsyncBaseHTTPView` in an inconsistent manner [bad-override]
+ ERROR strawberry/fastapi/router.py:60:5-26: Class member `GraphQLRouter.allow_queries_via_get` overrides parent class `AsyncBaseHTTPView` in an inconsistent manner [bad-override]
+ ERROR strawberry/sanic/views.py:57:5-26: Class member `GraphQLView.allow_queries_via_get` overrides parent class `AsyncBaseHTTPView` in an inconsistent manner [bad-override]
dd-trace-py (https://github.com/DataDog/dd-trace-py)
- ERROR ddtrace/appsec/_iast/_patch_modules.py:99:58-67: Argument `str` is not assignable to parameter `wrapper` with type `(...) -> Unknown` in function `IASTFunction.force_wrapper` [bad-argument-type]
+ ERROR ddtrace/appsec/_iast/_patch_modules.py:99:58-67: Argument `str | Unknown` is not assignable to parameter `wrapper` with type `(...) -> Unknown` in function `IASTFunction.force_wrapper` [bad-argument-type]
core (https://github.com/home-assistant/core)
+ ERROR homeassistant/components/script/__init__.py:556:5-9: Class member `ScriptEntity.icon` overrides parent class `BaseScriptEntity` in an inconsistent manner [bad-override]
+ ERROR homeassistant/components/script/__init__.py:556:5-9: Class member `ScriptEntity.icon` overrides parent class `RestoreEntity` in an inconsistent manner [bad-override]
|
Primer Diff Classification❌ 1 regression(s) | ✅ 5 improvement(s) | ➖ 1 neutral | 7 project(s) total | +17, -8 errors 1 regression(s) across mkdocs. error kinds:
Detailed analysis❌ Regression (1)mkdocs (+5)
✅ Improvement (5)bokeh (-1)
apprise (+5, -6)
aiohttp (+1)
strawberry (+4)
core (+1)
➖ Neutral (1)dd-trace-py (+1, -1)
Suggested fixesSummary: The new attribute type widening logic in class_field.rs fails to handle cached_property descriptors, causing 5 false positive errors in mkdocs where the raw function type leaks into the union instead of the descriptor's resolved return type. 1. In the new init_assignments block in
Was this helpful? React with 👍 or 👎 Classification by primer-classifier (1 heuristic, 6 LLM) |
Summary
Fixes #1159
Implemented the issue 1159 request to use assignments within the class (e.g., a = 1 then self.a = "a") when inferring attribute types, so the method assignment widens the inferred type instead of erroring.
Test Plan
add tests