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 intofacebook:mainfrom
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 |
63b902b to
015278d
Compare
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.
|
I wasn't able to figure out a balanced solution here that didn't regress mypy primer |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
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]
cloud-init (https://github.com/canonical/cloud-init)
+ ERROR cloudinit/sources/DataSourceMAAS.py:148:9-26: Class member `DataSourceMAAS.check_instance_id` overrides parent class `DataSource` in an inconsistent manner [bad-override]
aiohttp (https://github.com/aio-libs/aiohttp)
+ ERROR aiohttp/connector.py:343:43-65: `StackSummary | Unknown` is not assignable to TypedDict key `source_traceback` with type `list[str] | str | Self@BaseConnector` [bad-typed-dict-key]
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:251:21-253:67: `NotifyFormat | PersistentStoreMode` is not assignable to attribute `__storage_mode` with type `PersistentStoreMode` [bad-assignment]
+ ERROR apprise/asset.py:499:16-35: Returned type `NotifyFormat | PersistentStoreMode` is not assignable to declared return type `PersistentStoreMode` [bad-return]
- ERROR apprise/asset.py:515:16-28: Returned type `Unknown | None` is not assignable to declared return type `tzinfo` [bad-return]
+ ERROR apprise/asset.py:515:16-28: Returned type `tzinfo | Unknown | None` is not assignable to declared return type `tzinfo` [bad-return]
+ ERROR apprise/cli.py:1177:65-78: No matching overload found for function `str.join` called with arguments: (set[object] | set[Unknown]) [no-matching-overload]
- ERROR apprise/config/base.py:146: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:375:33-60: `Unknown | None` is not assignable to attribute `interpret_emojis` with type `bool` [bad-assignment]
- ERROR apprise/plugins/kodi.py:321:17-59: `<=` is not supported between `tuple[Literal['kodi'], Literal['xbmc']]` and `int` [unsupported-operation]
+ ERROR apprise/plugins/kodi.py:321:17-59: `<=` is not supported between `tuple[str, str]` and `int` [unsupported-operation]
+ ERROR apprise/plugins/kodi.py:321:17-59: `<=` is not supported between `None` and `int` [unsupported-operation]
- ERROR apprise/plugins/kodi.py:369:17-59: `<=` is not supported between `tuple[Literal['kodi'], Literal['xbmc']]` and `int` [unsupported-operation]
+ ERROR apprise/plugins/kodi.py:369:17-59: `<=` is not supported between `tuple[str, str]` and `int` [unsupported-operation]
+ ERROR apprise/plugins/kodi.py:369:17-59: `<=` is not supported between `None` and `int` [unsupported-operation]
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]
core (https://github.com/home-assistant/core)
+ ERROR homeassistant/components/script/__init__.py:560:5-9: Class member `ScriptEntity.icon` overrides parent class `BaseScriptEntity` in an inconsistent manner [bad-override]
+ ERROR homeassistant/components/script/__init__.py:560:5-9: Class member `ScriptEntity.icon` overrides parent class `RestoreEntity` in an inconsistent manner [bad-override]
|
Primer Diff Classification❌ 2 regression(s) | ✅ 5 improvement(s) | 7 project(s) total | +18, -7 errors 2 regression(s) across mkdocs, aiohttp. error kinds:
Detailed analysis❌ Regression (2)mkdocs (+5)
aiohttp (+1)
✅ Improvement (5)cloud-init (+1)
Looking at the code: The bad-override error is likely due to a signature or return type mismatch between The fact that this is newly detected by pyrefly may be related to the PR's changes in how class members and method signatures are resolved, but the core issue is a method override that doesn't match the parent class's signature - not specifically about the type of
bokeh (-1)
apprise (+6, -6)
Per-category reasoning:
strawberry (+4)
core (+1)
Suggested fixesSummary: The new attribute type widening logic (unioning class-body definitions with init assignments) fails to account for descriptor types like cached_property, causing 5 false positives in mkdocs and 1 in aiohttp. 1. In the new init_assignments widening block in class_field.rs (around line 1859-1921), the code checks
2. The aiohttp regression is an indirect consequence of the widening logic changing
Was this helpful? React with 👍 or 👎 Classification by primer-classifier (7 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