fix Preliminary support for class decorators #2752#2773
fix Preliminary support for class decorators #2752#2773asukaminato0721 wants to merge 2 commits intofacebook:mainfrom
Conversation
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.
There was a problem hiding this comment.
Pull request overview
This PR enhances Pyrefly’s solver to type-check class decorator application so the decorated class name can be rebound to the decorator’s return type (e.g., @remote turning a class name into an ActorHandle[...]), and adds a regression test covering this behavior.
Changes:
- Apply non-dataclass-transform class decorators during
Binding::ClassDeftype solving to infer the post-decoration value type. - Extend
untype_opt/expr_untypehandling forType::KwCalland name lookups to better support the new decorator behavior. - Add a new decorator test case ensuring a class decorator can rebind the class value.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
pyrefly/lib/test/decorators.rs |
Adds a regression testcase for a class decorator that rebinds the class symbol to an ActorHandle[T]. |
pyrefly/lib/alt/solve.rs |
Implements class-decorator application during solving and adjusts untyping/name handling to support the new behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| None, | ||
| None, | ||
| ); | ||
| if self.untype_opt(ty.clone(), range, errors).is_some() { |
There was a problem hiding this comment.
untype_opt is being used here purely as a predicate to detect whether the decorator result is a type form, but untype_opt calls canonicalize_all_class_types, which can emit user-facing errors (e.g., implicit-Any errors for tuple/Callable). That can lead to spurious errors at class decoration sites when a decorator returns something that happens to be untypeable, even though we’re not actually interpreting the result as an annotation. Consider running this check with an ErrorStyle::Never collector (via error_swallower()), or adding a side-effect-free helper for “is this a type form?” so the predicate doesn’t report errors.
| if self.untype_opt(ty.clone(), range, errors).is_some() { | |
| let mut swallower = error_swallower(); | |
| if self | |
| .untype_opt(ty.clone(), range, &mut swallower) | |
| .is_some() | |
| { |
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.
This comment has been minimized.
This comment has been minimized.
@migeed-z why does this get classified as "improvement" lol |
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.
|
Thank you for your work on this but wouldn't real-world decorators (@deprecated, @wraps, @lazy_lexicographic_ordering, etc.) have return types that resolve to Unknown or generic callables, so the original class type must be preserved. This is what is causing the regressions. Tagging @samwgoldman / @stroxler because it is their domain. This may also be worth scoping down to only support decorators whose return type resolves to a concrete class/type-form, instead of unknowns which could be picked in a follow-up. |
|
Diff from mypy_primer, showing the effect of this PR on open source code: jax (https://github.com/google/jax)
+ ERROR jax/experimental/array_serialization/serialization_test.py:820:1-821:38: `Unknown` is not assignable to upper bound `type[Any]` of type variable `Typ` [bad-specialization]
+ ERROR jax/experimental/array_serialization/serialization_test.py:950:5-951:38: `Unknown` is not assignable to upper bound `type[Any]` of type variable `Typ` [bad-specialization]
+ ERROR jax/experimental/array_serialization/serialization_test.py:963:5-35: `UserPytreeAPITest.test_custom_node_registration.P` is not assignable to upper bound `Hashable` of type variable `H` [bad-specialization]
+ ERROR jax/experimental/array_serialization/serialization_test.py:968:5-969:43: `Unknown` is not assignable to upper bound `type[Any]` of type variable `Typ` [bad-specialization]
spark (https://github.com/apache/spark)
+ ERROR python/pyspark/testing/tests/test_skip_class.py:20:1-15: Argument `type[SkipClassTests]` is not assignable to parameter `reason` with type `str` in function `unittest.case.skip` [bad-argument-type]
|
Primer Diff Classification❌ 1 regression(s) | ✅ 1 improvement(s) | 2 project(s) total | +5 errors 1 regression(s) across jax. error kinds:
Detailed analysis❌ Regression (1)jax (+4)
All errors are pyrefly-only (neither mypy nor pyright flags them), and all are regressions from the new decorator handling code.
✅ Improvement (1)spark (+1)
Suggested fixesSummary: The new class decorator analysis in solve.rs doesn't handle Unknown/failed inference results gracefully, causing 4 pyrefly-only regressions in jax where decorators like @partial(register_dataclass, ...) and @register_static produce Unknown or trigger false constraint violations instead of preserving the original class type. 1. In the Binding::ClassDef branch in solve.rs (around the new decorator loop, ~line 5020-5050), after calling call_infer() to get decorated_ty, add a guard that checks if decorated_ty is Unknown (or contains Unknown). When the decorator inference fails and returns Unknown, the code should fall back to preserving the original class type (ty = self.heap.mk_class_def(cls.dupe())) rather than proceeding to the callable/untype checks. Specifically, after
2. In the Binding::ClassDef branch in solve.rs, the decorator loop should also handle the case where call_infer produces errors during type variable bound checking but the decorator is identity-like (returns the same type as input). Specifically, for the @register_static case, the decorator has signature like
Was this helpful? React with 👍 or 👎 Classification by primer-classifier (2 LLM) |
Summary
Fixes #2752
applying class decorators to the runtime class-value bindingm while preserving the original class object when a decorator still returns a usable type form.
Test Plan
add test