Skip to content

fix use all usages within class (or within __init__) to infer type, not just initial assignment #1159#2356

Open
asukaminato0721 wants to merge 3 commits into
facebook:mainfrom
asukaminato0721:1159
Open

fix use all usages within class (or within __init__) to infer type, not just initial assignment #1159#2356
asukaminato0721 wants to merge 3 commits into
facebook:mainfrom
asukaminato0721:1159

Conversation

@asukaminato0721

Copy link
Copy Markdown
Contributor

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

@meta-cla meta-cla Bot added the cla signed label Feb 7, 2026
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@asukaminato0721 asukaminato0721 marked this pull request as ready for review February 9, 2026 05:52
Copilot AI review requested due to automatic review settings February 9, 2026 05:52

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 BindingClassField into 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.kind is moved when matching if let ScopeKind::Class(class_scope) = class_body.kind, but class_body is 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 of class_body (e.g., destructure class_body into stat/flow/kind first, or match on a reference to kind).
        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.

Comment on lines +84 to +100
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)
"#,
);

Copilot AI Feb 9, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
Comment on lines 2118 to 2122
/// 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.

Copilot AI Feb 9, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
class,
name,
direct_annotation.as_ref(),
true,

Copilot AI Feb 9, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Suggested change
true,
false,
Copilot uses AI. Check for mistakes.
Comment thread pyrefly/lib/alt/class/class_field.rs Outdated
@kinto0

kinto0 commented Feb 13, 2026

Copy link
Copy Markdown
Contributor

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

@rchen152

Copy link
Copy Markdown
Contributor

Hmm. I'm not convinced this is a good idea. Looking at the proposed new behavior:

from typing import assert_type
class A:
    a = 1
    def set_a(self, value: str):
        self.a = value
def f(a: A):
    assert_type(a.a, int | str)

this strikes me as potentially surprising behavior (if set_a is buried in a long class definition, a user might be quite confused about where the str type is coming from) and a little out-of-step with the "infer on first use" rule that pyrefly uses elsewhere.

@rchen152 rchen152 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Leaving a comment to clear this out from my review queue.)

@yangdanny97

Copy link
Copy Markdown
Contributor

we should experiment with only allowing this in __init__

@asukaminato0721 asukaminato0721 force-pushed the 1159 branch 2 times, most recently from 6e25e96 to cde1201 Compare February 25, 2026 17:02
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@asukaminato0721

Copy link
Copy Markdown
Contributor Author
@yangdanny97

yangdanny97 commented Feb 25, 2026

Copy link
Copy Markdown
Contributor

thanks @asukaminato0721

odd, the mypy primer delta is more mixed than i would expect

@migeed-z can you run the classifier on this one?

@github-actions

This comment has been minimized.

@yangdanny97

Copy link
Copy Markdown
Contributor

wow that's a long comment lol, i'll read tomorrow and see if it's legit

@yangdanny97

Copy link
Copy Markdown
Contributor

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 __init__ if it's not initialized on the class, and take the class's initialization as the single source of truth if it's present.

The downside is that it wouldn't be able to handle things like this. We currently have a special-case for None on the class that makes it Any | None, but if we restrict this analysis we wouldn't be able to remove that hack.

class C:
  x = None

def __init__(self):
  if something:
    self.x = "foo"
@meta-codesync

meta-codesync Bot commented Feb 27, 2026

Copy link
Copy Markdown
Contributor

@yangdanny97 has imported this pull request. If you are a Meta employee, you can view this in D94676249.

@yangdanny97

Copy link
Copy Markdown
Contributor

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 __init__, similar to how we do return type inference.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

github-actions Bot commented Jun 6, 2026

Copy link
Copy Markdown

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: no-matching-overload. caused by finish_class_and_get_field_definitions(). 5 improvement(s) across bokeh, apprise, aiohttp, strawberry, core.

Project Verdict Changes Error Kinds Root Cause
mkdocs ❌ Regression +5 no-matching-overload finish_class_and_get_field_definitions()
bokeh ✅ Improvement -1 bad-assignment finish_class_and_get_field_definitions()
apprise ✅ Improvement +5, -6 bad-assignment, bad-return finish_class_and_get_field_definitions()
aiohttp ✅ Improvement +1 bad-assignment pyrefly/lib/alt/class/class_field.rs
strawberry ✅ Improvement +4 bad-override finish_class_and_get_field_definitions()
dd-trace-py ➖ Neutral +1, -1 bad-argument-type
core ✅ Improvement +1 bad-override finish_class_and_get_field_definitions()
Detailed analysis

❌ Regression (1)

mkdocs (+5)

The PR introduced a regression where pyrefly incorrectly infers the type of dest_uri as a union of the raw _get_dest_path function signature and str, instead of just str. The cached_property descriptor should resolve to its return type (str) when accessed on an instance. The root cause is that dest_uri = cached_property(_get_dest_path) uses cached_property as a class-level assignment rather than as a @cached_property decorator, and pyrefly fails to properly resolve the descriptor protocol for this usage pattern, instead producing a union of the underlying callable type and the return type.

All 5 errors are cascade false positives from this single cached_property inference bug:

  1. Line 261: self.dest_uri in dest_path property getter is inferred as the union type, causing os.path.normpath to reject it.
  2. Line 395: url = self.dest_uri in _get_url carries the wrong type, then posixpath.split(url) fails.
  3. Line 400: urlquote(url) fails because url may still carry the union type from the assignment at line 394.
  4. Line 419: self.dest_uri in abs_dest_path cached property is inferred as the union type, causing os.path.join to reject it.
  5. Line 564: file.dest_uri in get_files is inferred as the union type, causing dict.setdefault to reject it as a key for dict[str, File].

Neither mypy nor pyright reports any of these errors, confirming they are false positives caused by pyrefly's incorrect handling of cached_property used via class-level assignment.

Attribution: The change in pyrefly/lib/binding/scope.rs (finish_class_and_get_field_definitions()) now collects method-defined attributes and associates them with class body field definitions. When dest_uri = cached_property(_get_dest_path) is defined in the class body, and self.dest_uri = dest_uri appears in __init__ (line 345), the new code in pyrefly/lib/alt/class/class_field.rs unions the cached_property type with the __init__ assignment type. But it appears to be using the raw function type of _get_dest_path rather than the str return type that cached_property would produce, creating the incorrect union ((self, ...) -> str) | str.

✅ Improvement (5)

bokeh (-1)

This is an improvement. The PR fixes a false positive where pyrefly was too narrow in its attribute type inference, only considering class body assignments. Now it also considers init assignments, which correctly widens the attribute type and eliminates the spurious bad-assignment error on Bokeh's LegendItem.label.
Attribution: The changes to pyrefly/lib/binding/scope.rs in finish_class_and_get_field_definitions() now collect method-defined attributes (including init assignments) and associate them with class field definitions. The changes to pyrefly/lib/alt/class/class_field.rs then use these method_assignments to widen the inferred type by unioning the class body type with init assignment types. This directly caused the bad-assignment error to be removed because label's type is now widened to include the Value type from the init assignment.

apprise (+5, -6)

The PR correctly widens attribute types based on init assignments, which catches real bugs: (1) __storage_mode can be assigned a NotifyFormat value due to a likely typo (isinstance check against NotifyFormat instead of PersistentStoreMode), (2) _tzinfo can be None but the property claims non-optional return. The removed errors either moved to better locations or improved in precision. All 5 new errors are confirmed by pyright. Net change: 5 added, 6 removed = 1 fewer error overall, with better error quality.
Attribution: The changes in pyrefly/lib/binding/scope.rs (in finish_class_and_get_field_definitions()) collect method-defined attributes and associate them with class-level fields. The changes in pyrefly/lib/alt/class/class_field.rs (in the class field resolution logic) use init_assignments to widen the inferred type by unioning the class-body type with init assignment types. This causes __storage_mode to become NotifyFormat | PersistentStoreMode and _tzinfo to become tzinfo | None, surfacing real type mismatches at return sites.

aiohttp (+1)

The PR correctly widens _source_traceback to StackSummary | None based on the __init__ assignment. However, this triggers a downstream false positive: the context dict is inferred with a narrow value type from its literal, and assigning a StackSummary to it is flagged as incompatible. The code is correct — context is passed to call_exception_handler(dict[str, Any]) and can hold any value type. This is a regression because pyrefly is producing a new false positive that neither mypy nor pyright report.
Attribution: The change in pyrefly/lib/alt/class/class_field.rs and pyrefly/lib/binding/scope.rs now uses __init__ assignments to widen inferred attribute types. This widened _source_traceback from None to StackSummary | None. Combined with pyrefly's strict dict value type inference (locking the dict's value type to the types present in the literal), this produces a false positive bad-assignment error when assigning a StackSummary value to a dict key whose inferred value type is list[str] | str | Self@BaseConnector.

strawberry (+4)

The parent AsyncBaseHTTPView defines allow_queries_via_get as an @property @abc.abstractmethod (line 108-110 of async_base_view.py). The child classes override it with a plain class variable allow_queries_via_get = True. This is a real type-level inconsistency (property vs attribute), confirmed by pyright (4/4). While mypy allows this pattern and it works at runtime, it is a genuine override inconsistency. The PR's changes to type inference for class fields with __init__ assignments appear to have changed the code path enough to now trigger this check. These are legitimate findings, even if the pattern is common in practice.
Attribution: The changes in pyrefly/lib/binding/scope.rs (specifically finish_class_and_get_field_definitions()) and pyrefly/lib/alt/class/class_field.rs now collect __init__ assignments and union them with class body assignments. This changed the type inference path for allow_queries_via_get, which now triggers the bad-override check that wasn't triggered before. The new code in class_field.rs that processes init_assignments and calls self.unions(types) changes how the field type is computed, which apparently now causes the override checker to detect the property-vs-attribute mismatch.

core (+1)

This is a cascading effect of the PR's new type widening behavior. The icon = None class attribute in ScriptEntity now gets its type widened by incorporating self.icon = cfg.get(CONF_ICON) from __init__. This widened type then triggers a bad-override against the parent class's icon property. While pyright also flags this (suggesting it's a real type-level concern), this is an intentional and pervasive pattern in Home Assistant. The new error is a true positive at the type level — the child class does override a parent property with a plain attribute of a potentially different type. Since pyright agrees this is a real override inconsistency, this is an improvement in pyrefly's analysis.
Attribution: The change in pyrefly/lib/binding/scope.rs (finish_class_and_get_field_definitions()) now collects __init__ assignments and passes them to class_field.rs via method_assignments. This causes ScriptEntity.icon to have its type widened from just None (class body) to include the __init__ assignment cfg.get(CONF_ICON), which changes the inferred type and triggers the bad-override check against the parent's icon property.

➖ Neutral (1)

dd-trace-py (+1, -1)

Same errors at same locations with same error kinds — message wording changed, no behavioral impact.

Suggested fixes

Summary: The PR's new logic for unioning class body field types with init assignments fails to resolve cached_property descriptors, causing mkdocs regressions where dest_uri gets inferred as ((self, ...) -> str) | str instead of str.

1. In the new init_assignments processing block in class_field.rs (around line 1752-1810), after computing the unioned value_ty from class body and init assignments, the code skips descriptor resolution because the descriptor variable was computed only from the original class body value_ty. When dest_uri = cached_property(_get_dest_path) is defined in the class body and self.dest_uri = dest_uri appears in __init__, the class body type is cached_property[str] (a descriptor) while the init type resolves to str. The union becomes cached_property[str] | str which is wrong. The fix: before unioning, resolve the class body value_ty through the descriptor protocol (i.e., check if it's a descriptor and extract its __get__ return type), so that the union is str | str = str. Specifically, in the block starting with if matches!(field_definition, ClassFieldDefinition::AssignedInBody { .. }) && !init_assignments.is_empty(), the first element pushed to types should be the descriptor-resolved type rather than the raw value_ty. Alternatively, add a guard: if descriptor.is_some(), skip the entire init_assignments unioning block, since descriptor fields already have well-defined types from the descriptor protocol.

Files: pyrefly/lib/alt/class/class_field.rs
Confidence: high
Affected projects: mkdocs
Fixes: bad-argument-type
The descriptor variable is computed earlier (around line 1740-1750 area) from the original class body value. When cached_property(_get_dest_path) is assigned in the class body, pyrefly recognizes it as a descriptor and would normally resolve it to str via __get__. However, the new init_assignments block at line 1752 has the condition && descriptor.is_none(), which should prevent this issue — but the problem is that descriptor was computed from the original value_ty before the init_assignments block runs. Looking more carefully at the code: the condition descriptor.is_none() IS present in the guard. This means the descriptor case should be excluded. But the regression report says the type becomes ((self, ...) -> str) | str, suggesting the raw function type _get_dest_path is leaking through. This happens because cached_property(_get_dest_path) when used as a class-level assignment (not decorator) may not be recognized as a descriptor in this code path — the descriptor check may only trigger for @cached_property decorator syntax. So the fix should be: in the init_assignments block, before pushing value_ty into types, check if value_ty is a cached_property (or any descriptor type) and resolve it through __get__ first. Or more robustly: skip the init_assignments unioning when the class body value is a descriptor type, by checking the type itself rather than relying on the descriptor variable which may not be set for non-decorator cached_property usage. This would eliminate all 5 pyrefly-only errors in mkdocs.


Was this helpful? React with 👍 or 👎

Classification by primer-classifier (1 heuristic, 6 LLM)

@github-actions

Copy link
Copy Markdown

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]
@github-actions

Copy link
Copy Markdown

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: no-matching-overload. caused by finish_class_and_get_field_definitions(). 5 improvement(s) across bokeh, apprise, aiohttp, strawberry, core.

Project Verdict Changes Error Kinds Root Cause
mkdocs ❌ Regression +5 no-matching-overload finish_class_and_get_field_definitions()
bokeh ✅ Improvement -1 bad-assignment finish_class_and_get_field_definitions()
apprise ✅ Improvement +5, -6 bad-assignment, bad-return finish_class_and_get_field_definitions()
aiohttp ✅ Improvement +1 bad-assignment finish_class_and_get_field_definitions()
strawberry ✅ Improvement +4 bad-override pyrefly/lib/binding/scope.rs
dd-trace-py ➖ Neutral +1, -1 bad-argument-type
core ✅ Improvement +1 bad-override finish_class_and_get_field_definitions()
Detailed analysis

❌ Regression (1)

mkdocs (+5)

The PR introduces a new feature to widen attribute types by considering __init__ assignments alongside class body definitions. However, it fails to properly handle cached_property descriptors. dest_uri = cached_property(_get_dest_path) is a descriptor that, when accessed on an instance, should resolve to str (the return type of _get_dest_path). The __init__ method conditionally assigns self.dest_uri = dest_uri where dest_uri is of type str (narrowed from str | None by the if dest_uri is not None guard). Pyrefly appears to incorrectly resolve the cached_property descriptor, exposing the raw function type of _get_dest_path(self: Self@File, use_directory_urls: bool | None = None) -> str — rather than just str. This function type gets unioned with the str from the __init__ assignment, producing a union type that includes the function signature. This causes all downstream usages of self.dest_uri (passed to posixpath.normpath, posixpath.split, urlquote, posixpath.join, dict.setdefault) to fail overload resolution since they expect str, not function | str. The errors at lines 395 and 400 occur inside _get_url, which reads self.dest_uri and passes the result to posixpath.split and urlquote — these fail for the same reason. The url attribute (url = cached_property(_get_url)) similarly has its type affected when accessed. All 5 errors are false positives — the code is correct and works fine at runtime because cached_property properly resolves to the return type of the wrapped function when accessed on an instance.
Attribution: The change in pyrefly/lib/alt/class/class_field.rs (the new block starting around line 1796 that collects init_assignments and unions their types with the class body value type) is the direct cause. For dest_uri = cached_property(_get_dest_path), the PR's new logic unions the cached_property's underlying function type with the str from self.dest_uri = dest_uri in __init__, producing ((self: Self@File, ...) -> str) | str. The code in pyrefly/lib/binding/scope.rs (finish_class_and_get_field_definitions()) now collects method-defined attributes and passes them through, and the descriptor.[is_none()](https://github.com/facebook/pyrefly/blob/main/pyrefly/lib/alt/class/class_field.rs) guard in class_field.rs doesn't catch this because cached_property may not be recognized as a descriptor at that point in the logic.

✅ Improvement (5)

bokeh (-1)

The removed error was a false positive. Bokeh uses a descriptor-based property system where NullStringSpec is a descriptor, and value() returns a valid value for that descriptor. The PR's approach of widening attribute types based on __init__ assignments correctly resolves this by inferring a union type that accommodates both the class body and __init__ assignments.
Attribution: The change in pyrefly/lib/binding/scope.rs (finish_class_and_get_field_definitions()) now collects MethodDefinedAttribute entries from __init__ and associates them with class-body-defined fields. The change in pyrefly/lib/alt/class/class_field.rs then uses these init_assignments to widen the inferred type by unioning the class body type with __init__ assignment types. This causes the label attribute's type to be widened to include the Value type from the __init__ assignment, eliminating the bad-assignment error.

apprise (+5, -6)

The PR correctly widens attribute types based on init assignments. The new bad-return errors are real bugs: (1) the NotifyFormat isinstance check is genuinely wrong (should be PersistentStoreMode), confirmed by pyright; (2) _tzinfo can be None but the property declares non-optional return. The removed errors are either replaced by better-located errors or had worse type inference (Unknown). The kodi.py changes are neutral (same count, just literal promotion). Net: 5 added errors catching real issues, 6 removed errors that were either false positives or replaced by better errors.
Attribution: The changes in pyrefly/lib/binding/scope.rs (finish_class_and_get_field_definitions()) and pyrefly/lib/alt/class/class_field.rs now collect __init__ assignments and widen the inferred attribute type to be the union. This causes the bad-assignment errors to disappear (the assignment is now valid under the widened type) but surfaces bad-return errors where the widened type doesn't match the declared return type of properties.

aiohttp (+1)

This is a false positive (regression). The PR's new attribute type widening correctly identifies that _source_traceback can be StackSummary | None, but this creates a downstream error because the context dict's value type is inferred too narrowly from its literal initializer. The dict is passed to call_exception_handler(dict[str, Any]), so any value type should be acceptable. The presence of Unknown in the inferred type also indicates an inference limitation. Neither mypy nor pyright flag this.
Attribution: The change in pyrefly/lib/binding/scope.rs (finish_class_and_get_field_definitions()) and pyrefly/lib/alt/class/class_field.rs now unions class-level and __init__ assignments for attribute type inference. This widened _source_traceback from None to StackSummary | None. Previously, the type was just None, so after the is not None guard the branch was unreachable (type narrowed to Never), making the assignment vacuously valid. Now the branch is reachable with type StackSummary | Unknown, which conflicts with the narrowly-inferred dict value type. The Unknown component also suggests an inference issue in the new widening code path.

strawberry (+4)

The parent AsyncBaseHTTPView defines allow_queries_via_get as @property @abc.abstractmethod def allow_queries_via_get(self) -> bool. The child classes override it with a plain attribute allow_queries_via_get = True and reassign in __init__. Per the typing spec (https://typing.readthedocs.io/en/latest/spec/class-compat.html), a mutable attribute CAN override a read-only property, and mypy does not flag this (consistent with [mypy: no] in the error annotations). However, pyright intentionally flags this as an inconsistent override ([pyright: yes]), considering that changing a read-only property to a mutable attribute alters the mutability contract of the interface. Since pyrefly's errors are annotated with [pyright: yes], these new errors represent pyrefly aligning with pyright's stricter interpretation rather than being false positives. The PR's new logic for widening attribute types based on __init__ assignments likely changed how the attribute is categorized internally, causing pyrefly to now detect this override pattern that pyright also flags.
Attribution: The PR changes in pyrefly/lib/binding/scope.rs and pyrefly/lib/alt/class/class_field.rs now consider __init__ assignments when inferring attribute types. This likely changed the internal representation of allow_queries_via_get from a simple class variable to a class+method-defined attribute, which triggered the bad-override check against the parent's abstract property definition.

core (+1)

The bad-override error for ScriptEntity.icon is flagged because the parent class hierarchy (Entity, via ToggleEntity and BaseScriptEntity) defines icon as a @property returning str | None, while ScriptEntity overrides it with a plain class-level attribute (icon = None). This is an inconsistent override manner — replacing a property with a plain attribute. The error is also reported by pyright (pyright: yes), confirming it reflects a genuine override inconsistency rather than a false positive. While the PR's type-widening logic may have changed the inferred type of the attribute (unioning None from the class body with the __init__ assignment), None | Any simplifies to Any which is type-compatible, so the type itself is not the source of the error. The root cause is the attribute-kind mismatch (property vs. plain attribute) in the override.
Attribution: The change in pyrefly/lib/binding/scope.rs (finish_class_and_get_field_definitions()) now collects __init__ assignments as MethodDefinedAttribute and passes them to class_field.rs where analyze_class_field() unions the class-body type with __init__ assignment types. This widened the inferred type of ScriptEntity.icon from None to None | <return type of cfg.get(CONF_ICON)>, triggering the bad-override check against the parent's icon type.

➖ Neutral (1)

dd-trace-py (+1, -1)

Same errors at same locations with same error kinds — message wording changed, no behavioral impact.

Suggested fixes

Summary: 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 analyze_class_field() in pyrefly/lib/alt/class/class_field.rs (around line 1796), the guard descriptor.[is_none()](https://github.com/facebook/pyrefly/blob/main/pyrefly/lib/alt/class/class_field.rs) checks whether the current class body value has a descriptor, but cached_property may not be recognized as a descriptor at that point. The fix should also check whether the class body value type is a callable/function type that would be wrapped by a descriptor like cached_property. Specifically, before entering the widening block, resolve the class body value_ty through descriptor protocol first, or add an additional guard that skips widening when the class body value is a cached_property (or any data descriptor). The condition && descriptor.[is_none()](https://github.com/facebook/pyrefly/blob/main/pyrefly/lib/alt/class/class_field.rs) should be strengthened to also check if value_ty is a cached_property type. Concretely: after computing descriptor earlier in the function, if descriptor is Some(...), the code already skips widening. The issue is that cached_property is not being detected as a descriptor. Check whether cached_property instances are being resolved through self.[get_descriptor()](https://github.com/facebook/pyrefly/blob/main/pyrefly/lib/alt/class/class_field.rs) or equivalent — if not, ensure that cached_property is treated as a descriptor so that descriptor.[is_some()](https://github.com/facebook/pyrefly/blob/main/pyrefly/lib/alt/class/class_field.rs) is true, which will cause the widening block to be skipped. Alternatively, a simpler targeted fix: in the widening block's guard condition, add a check like && !matches!(value_ty, Type::ClassType(ct) if self.is_cached_property_or_property(ct)) or check if the class-body-defined value's type is a known descriptor class (property, cached_property, etc.) and skip widening in that case.

Files: pyrefly/lib/alt/class/class_field.rs
Confidence: high
Affected projects: mkdocs
Fixes: bad-argument
The descriptor.is_none() guard is supposed to prevent widening for descriptor-typed attributes, but cached_property is apparently not being detected as a descriptor at that point in the logic. When dest_uri = cached_property(_get_dest_path) is defined in the class body, the value_ty includes the raw function type. The init assigns self.dest_uri = dest_uri (str). The widening logic unions these, producing ((self: Self@File, ...) -> str) | str instead of just str. If cached_property were properly recognized as a descriptor, the descriptor.is_none() check would be false and widening would be skipped, preserving the correct descriptor-resolved type. This would eliminate all 5 pyrefly-only errors in mkdocs.


Was this helpful? React with 👍 or 👎

Classification by primer-classifier (1 heuristic, 6 LLM)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment