Enforce __slots__ validation for attribute writes#2695
Enforce __slots__ validation for attribute writes#2695Adist319 wants to merge 2 commits intofacebook:mainfrom
Conversation
Adds type checking enforcement for Python's `__slots__` mechanism. When a class defines `__slots__`, writing to attributes not declared in the slots now produces a `missing-attribute` error. Handles tuple, list, and single-string literal forms of `__slots__`, as well as `@dataclass(slots=True)`. Enforcement is correctly suppressed when any ancestor is unslotted, `__dict__` appears in slots, or the class defines a custom `__setattr__`. Properties and descriptors are also exempted since they operate at the class level rather than instance storage.
|
Diff from mypy_primer, showing the effect of this PR on open source code: rich (https://github.com/Textualize/rich)
+ ERROR rich/text.py:1325:13-23: Object of class `Text` has no attribute `plain` (not declared in `__slots__`) [missing-attribute]
dd-trace-py (https://github.com/DataDog/dd-trace-py)
+ ERROR ddtrace/_trace/span.py:231:14-25: Object of class `Span` has no attribute `duration_ns` (not declared in `__slots__`) [missing-attribute]
+ ERROR ddtrace/_trace/span.py:298:18-25: Object of class `Span` has no attribute `service` (not declared in `__slots__`) [missing-attribute]
+ ERROR ddtrace/_trace/span.py:490:14-19: Object of class `Span` has no attribute `error` (not declared in `__slots__`) [missing-attribute]
core (https://github.com/home-assistant/core)
+ ERROR homeassistant/components/bluetooth/manager.py:93:14-20: Object of class `HomeAssistantBluetoothManager` has no attribute `_debug` (not declared in `__slots__`) [missing-attribute]
+ ERROR homeassistant/components/bluetooth/manager.py:182:14-26: Object of class `HomeAssistantBluetoothManager` has no attribute `_all_history` (not declared in `__slots__`) [missing-attribute]
+ ERROR homeassistant/components/bluetooth/manager.py:182:33-53: Object of class `HomeAssistantBluetoothManager` has no attribute `_connectable_history` (not declared in `__slots__`) [missing-attribute]
pip (https://github.com/pypa/pip)
+ ERROR src/pip/_vendor/rich/text.py:1323:13-23: Object of class `Text` has no attribute `plain` (not declared in `__slots__`) [missing-attribute]
|
Primer Diff Classification❌ 3 regression(s) | ✅ 1 improvement(s) | 4 project(s) total 3 regression(s) across rich, core, pip. error kinds:
Detailed analysis❌ Regression (3)rich (+1)
core (+3)
Looking at the specific errors:
These would indeed cause AttributeError at runtime due to Python's slots mechanism, but mypy and pyright do not flag such patterns because they treat slots as a runtime optimization detail rather than a type checking concern. The ecosystem standard is to allow these patterns in static analysis. The HomeAssistant codebase is a mature, well-tested project. These attribute assignments work at runtime, likely because the parent class This represents pyrefly being stricter than the established ecosystem standard, creating false positive noise without practical type safety value.
pip (+1)
✅ Improvement (1)dd-trace-py (+3)
Suggested FixSummary: New slots enforcement is stricter than ecosystem standard, causing regressions in 3 projects despite catching genuine runtime bugs in others. 1. In
2. In
Was this helpful? React with 👍 or 👎 Classification by primer-classifier (4 LLM) |
|
The mypy primer error delta looks a lot more reasonable this time! @rchen152 this looks reasonable to me. I tagged you for a quick look as well :) |
This comment was marked as outdated.
This comment was marked as outdated.
Summary: Adds type checking enforcement for Python's `__slots__` mechanism. When a class defines `__slots__`, writing to attributes not declared in the slots now produces a `missing-attribute` error. Fixes facebook#630. Supported `__slots__` forms: - Tuple literals: `__slots__ = ("x", "y")` - List literals: `__slots__ = ["x", "y"]` - Single string: `__slots__ = "x"` - `dataclass(slots=True)` Enforcement is correctly suppressed when: - Any ancestor in the MRO is unslotted (inherits `__dict__`) - `"__dict__"` appears in slots - The class or an ancestor defines a custom `__setattr__` - The attribute is accessed through a property setter or descriptor `__set__` The implementation checks three code paths where attribute writes occur: 1. **Instance attribute creation** (`self.y = 2` in methods) — caught during class field validation in `class_field.rs` 2. **External writes to missing attrs** (`c.y = 3` where `y` is unknown) — caught in `attr.rs` via the `GetAttr`/not-found paths 3. **External writes to class-level attrs not in slots** (`c.y = 3` where `y: int` is a bare annotation) — caught in `attr.rs` via the `ClassAttribute` path Test Plan: - 17 targeted test cases covering enforcement, suppression, and edge cases - `cargo test -p pyrefly test_slots` — all 17 pass - `cargo test -p pyrefly -- attributes::` — all 175 pass, no regressions - `cargo test -p pyrefly -- dataclasses::` — all 106 pass, no regressions Differential Revision: D95826013 Pulled By: migeed-z
rchen152
left a comment
There was a problem hiding this comment.
Sorry for the delay! Agree that this approach looks reasonable. The one piece of big-picture feedback I'd give is that it looks like we have two different methods of slots extraction (extracting from the type and extracting from the expression). Other than dataclass(slots=True), are there any cases we want to support in which the slots can't be extracted from the expression? If not, IMO it would be simpler to just always do that, rather than having it as a fallback.
yangdanny97
left a comment
There was a problem hiding this comment.
Review automatically exported from Phabricator review in Meta.
rchen152
left a comment
There was a problem hiding this comment.
Review automatically exported from Phabricator review in Meta.
Summary: Adds type checking enforcement for Python's `__slots__` mechanism. When a class defines `__slots__`, writing to attributes not declared in the slots now produces a `missing-attribute` error. Fixes facebook#630. Supported `__slots__` forms: - Tuple literals: `__slots__ = ("x", "y")` - List literals: `__slots__ = ["x", "y"]` - Single string: `__slots__ = "x"` - `dataclass(slots=True)` Enforcement is correctly suppressed when: - Any ancestor in the MRO is unslotted (inherits `__dict__`) - `"__dict__"` appears in slots - The class or an ancestor defines a custom `__setattr__` - The attribute is accessed through a property setter or descriptor `__set__` The implementation checks three code paths where attribute writes occur: 1. **Instance attribute creation** (`self.y = 2` in methods) — caught during class field validation in `class_field.rs` 2. **External writes to missing attrs** (`c.y = 3` where `y` is unknown) — caught in `attr.rs` via the `GetAttr`/not-found paths 3. **External writes to class-level attrs not in slots** (`c.y = 3` where `y: int` is a bare annotation) — caught in `attr.rs` via the `ClassAttribute` path Pull Request resolved: facebook#2695 Test Plan: - 17 targeted test cases covering enforcement, suppression, and edge cases - `cargo test -p pyrefly test_slots` — all 17 pass - `cargo test -p pyrefly -- attributes::` — all 175 pass, no regressions - `cargo test -p pyrefly -- dataclasses::` — all 106 pass, no regressions Reviewed By: rchen152, yangdanny97 Differential Revision: D95907248 Pulled By: migeed-z fbshipit-source-id: 17912ffadd2a7559129ec1c9c41b2e6257cd325d
Summary
Adds type checking enforcement for Python's
__slots__mechanism. When a class defines__slots__, writing to attributes not declared in the slots now produces amissing-attributeerror.Fixes #630.
Supported
__slots__forms:__slots__ = ("x", "y")__slots__ = ["x", "y"]__slots__ = "x"@dataclass(slots=True)Enforcement is correctly suppressed when:
__dict__)"__dict__"appears in slots__setattr____set__The implementation checks three code paths where attribute writes occur:
self.y = 2in methods) — caught during class field validation inclass_field.rsc.y = 3whereyis unknown) — caught inattr.rsvia theGetAttr/not-found pathsc.y = 3wherey: intis a bare annotation) — caught inattr.rsvia theClassAttributepathTest Plan
cargo test -p pyrefly test_slots— all 17 passcargo test -p pyrefly -- attributes::— all 175 pass, no regressionscargo test -p pyrefly -- dataclasses::— all 106 pass, no regressions