Skip to content

fix “Decorator @X can only be used on methods” is too strict #2610#2891

Open
asukaminato0721 wants to merge 2 commits intofacebook:mainfrom
asukaminato0721:2610
Open

fix “Decorator @X can only be used on methods” is too strict #2610#2891
asukaminato0721 wants to merge 2 commits intofacebook:mainfrom
asukaminato0721:2610

Conversation

@asukaminato0721
Copy link
Copy Markdown
Contributor

Summary

Fixes #2610

top-level @staticmethod, @classmethod, @property, and @cached_property are no longer treated as invalid just because they appear outside a class body.

Test Plan

update test

Copilot AI review requested due to automatic review settings March 25, 2026 03:08
@meta-cla meta-cla bot added the cla signed label Mar 25, 2026
Copy link
Copy Markdown

Copilot AI left a comment

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 relaxes Pyrefly’s “Decorator @X can only be used on methods” validation so that descriptor-style decorators (@staticmethod, @classmethod, @property, @cached_property) are permitted at module scope, supporting common “factory then assign into class” patterns (e.g., Django/admin helpers), and adds regression coverage for the intended usage.

Changes:

  • Stop emitting InvalidDecorator for top-level @staticmethod, @classmethod, @property, and @cached_property.
  • Update the existing “invalid top-level decorators” test to keep only decorators that remain invalid at module scope.
  • Add a new test case covering top-level descriptor creation and later assignment into classes.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
pyrefly/lib/test/decorators.rs Updates invalid-decorator expectations and adds a new regression test for top-level descriptor decorators assigned into classes.
pyrefly/lib/alt/function.rs Narrows top-level decorator validation to only the decorators that should remain method-only (e.g., @final, @override, enum decorators, @abstractmethod).

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

compute = utility


assert_type(Helper.compute(5), int)
Copy link

Copilot AI Mar 25, 2026

Choose a reason for hiding this comment

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

The new staticmethod scenario only calls Helper.compute(5) via the class. A plain undecorated function assigned as a class attribute is also callable via the class, so this test doesn’t actually validate staticmethod descriptor semantics. Consider also asserting Helper().compute(5) is int (instance access is where @staticmethod differs from a regular function attribute).

Suggested change
assert_type(Helper.compute(5), int)
assert_type(Helper.compute(5), int)
assert_type(Helper().compute(5), int)

Copilot uses AI. Check for mistakes.
Comment on lines +262 to +266
testcase!(
test_top_level_descriptor_decorators,
r#"
from typing import assert_type

Copy link

Copilot AI Mar 25, 2026

Choose a reason for hiding this comment

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

PR description mentions top-level @cached_property is now allowed, but the added regression test covers @property, @staticmethod, and @classmethod only. Adding a top-level @cached_property factory/assignment case here would prevent regressions for that specific decorator.

Copilot uses AI. Check for mistakes.
@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 added size/s and removed size/s labels Mar 28, 2026
@github-actions
Copy link
Copy Markdown

Diff from mypy_primer, showing the effect of this PR on open source code:

ibis (https://github.com/ibis-project/ibis)
- ERROR ibis/backends/impala/metadata.py:266:5-14: Decorator `@property` can only be used on methods. [invalid-decorator]

scikit-learn (https://github.com/scikit-learn/scikit-learn)
- ERROR sklearn/tests/test_metaestimators.py:90:9-18: Decorator `@property` can only be used on methods. [invalid-decorator]
- ERROR sklearn/utils/deprecation.py:106:9-18: Decorator `@property` can only be used on methods. [invalid-decorator]

spack (https://github.com/spack/spack)
- ERROR lib/spack/spack/package_base.py:173:13-25: Decorator `@classmethod` can only be used on methods. [invalid-decorator]
- ERROR lib/spack/spack/package_base.py:190:13-25: Decorator `@classmethod` can only be used on methods. [invalid-decorator]
- ERROR lib/spack/spack/package_base.py:247:13-25: Decorator `@classmethod` can only be used on methods. [invalid-decorator]
- ERROR lib/spack/spack/test/cmd/external.py:80:5-17: Decorator `@classmethod` can only be used on methods. [invalid-decorator]
- ERROR lib/spack/spack/test/cmd/external.py:254:5-17: Decorator `@classmethod` can only be used on methods. [invalid-decorator]
- ERROR lib/spack/spack/test/cmd/external.py:295:5-17: Decorator `@classmethod` can only be used on methods. [invalid-decorator]
- ERROR lib/spack/spack/test/cmd/external.py:326:5-17: Decorator `@classmethod` can only be used on methods. [invalid-decorator]
- ERROR lib/spack/spack/test/cmd/external.py:360:5-17: Decorator `@classmethod` can only be used on methods. [invalid-decorator]
- ERROR lib/spack/spack/test/detection.py:80:5-17: Decorator `@classmethod` can only be used on methods. [invalid-decorator]
- ERROR lib/spack/spack/test/packaging.py:388:5-14: Decorator `@property` can only be used on methods. [invalid-decorator]
- ERROR lib/spack/spack/test/packaging.py:404:5-14: Decorator `@property` can only be used on methods. [invalid-decorator]

spark (https://github.com/apache/spark)
- ERROR python/pyspark/pandas/missing/__init__.py:36:5-14: Decorator `@property` can only be used on methods. [invalid-decorator]
- ERROR python/pyspark/pandas/missing/__init__.py:42:5-14: Decorator `@property` can only be used on methods. [invalid-decorator]

prefect (https://github.com/PrefectHQ/prefect)
- ERROR src/prefect/utilities/dispatch.py:100:1-13: Decorator `@classmethod` can only be used on methods. [invalid-decorator]
- ERROR src/prefect/utilities/pydantic.py:154:9-21: Decorator `@classmethod` can only be used on methods. [invalid-decorator]

@github-actions
Copy link
Copy Markdown

Primer Diff Classification

✅ 5 improvement(s) | 5 project(s) total | -18 errors

5 improvement(s) across ibis, scikit-learn, spack, spark, prefect.

Project Verdict Changes Error Kinds Root Cause
ibis ✅ Improvement -1 invalid-decorator pyrefly/lib/alt/function.rs
scikit-learn ✅ Improvement -2 invalid-decorator pyrefly/lib/alt/function.rs
spack ✅ Improvement -11 invalid-decorator pyrefly/lib/alt/function.rs
spark ✅ Improvement -2 invalid-decorator pyrefly/lib/alt/function.rs
prefect ✅ Improvement -2 invalid-decorator pyrefly/lib/alt/function.rs
Detailed analysis

✅ Improvement (5)

ibis (-1)

This is a clear improvement. The _get_meta function at line 265 is a property factory — it applies @property to a local function and returns the resulting property descriptor. The descriptor is then assigned to class attributes in TableMetadata (lines 315-321), e.g., create_time = _get_meta('info', 'CreateTime'). This is a standard Python pattern. Pyrefly was incorrectly treating @property outside a class body as invalid, which is too strict — @property is just a callable that wraps a function into a descriptor, and it can be used anywhere.
Attribution: The change in pyrefly/lib/alt/function.rs removed the SpecialDecorator::Property(name) (along with StaticMethod, ClassMethod, and CachedProperty) from the match arm that generates the 'can only be used on methods' error. This means these decorators are no longer flagged when used outside a class body. The test file pyrefly/lib/test/decorators.rs confirms this intent by removing the expected errors for @staticmethod, @classmethod, and @property on top-level functions, and adding a new test test_top_level_descriptor_decorators that validates the factory pattern works correctly.

scikit-learn (-2)

Pyrefly was incorrectly flagging @property as invalid when used outside a class body. Python's property is a regular built-in that can be called/used as a decorator anywhere — it creates a descriptor object. The two removed errors were both false positives on legitimate code patterns (factory function returning a property, and method wrapping a property). The PR correctly relaxes this overly strict check.
Attribution: The change in pyrefly/lib/alt/function.rs removed the SpecialDecorator::Property(name) (along with StaticMethod, ClassMethod, and CachedProperty) from the match arm that generates the 'can only be used on methods' error. This directly caused the two invalid-decorator errors on @property outside class bodies to be removed. The test file pyrefly/lib/test/decorators.rs confirms the intent: @property, @staticmethod, @classmethod are no longer flagged when used at top level or in nested functions.

spack (-11)

The removed errors were all false positives. @classmethod, @staticmethod, and @property are valid Python decorators that can be used outside class bodies — they produce descriptor objects. In the Spack codebase, the DetectablePackageMeta.__init__ metaclass method defines @classmethod-decorated functions (lines 173, 190, 247) and later assigns them to cls (lines 258, 268, 275). This is a standard metaclass pattern. Pyrefly was incorrectly restricting these decorators to only appear inside class bodies, which is not required by Python or the typing spec.
Attribution: The change in pyrefly/lib/alt/function.rs removed the SpecialDecorator::StaticMethod, SpecialDecorator::ClassMethod, SpecialDecorator::Property, and SpecialDecorator::CachedProperty cases from the method-only decorator check in validate_decorator_on_method. This means these four decorators are no longer flagged with 'can only be used on methods' when they appear outside a class body. The test file pyrefly/lib/test/decorators.rs confirms this intent by removing the expected errors for @staticmethod, @classmethod, @property at the top level and adding a new test test_top_level_descriptor_decorators showing valid usage patterns.

spark (-2)

The removed errors were false positives. PySpark uses @property inside a regular function (unsupported_property) to create property descriptor objects that are returned and assigned to classes elsewhere. This is valid Python — @property is simply a callable that wraps a function into a property descriptor object, and this works regardless of whether it appears inside a class body or not. The returned property descriptors are then assigned as class attributes in other modules. Pyrefly was incorrectly requiring @property to only appear inside class bodies, but property is a built-in type/descriptor that can be instantiated anywhere. The PR correctly relaxes this restriction for @property (and potentially other descriptor-related decorators like @staticmethod, @classmethod, @cached_property) since these produce valid descriptor objects when used outside class bodies.
Attribution: The change in pyrefly/lib/alt/function.rs removed SpecialDecorator::Property(name) (along with StaticMethod, ClassMethod, and CachedProperty) from the match arms that generate the 'can only be used on methods' error. This means these decorators are no longer flagged as invalid when used outside a class body. The test changes in pyrefly/lib/test/decorators.rs confirm this intent, removing the @property, @staticmethod, @classmethod error expectations and adding a new test test_top_level_descriptor_decorators that validates these decorators work correctly outside class bodies.

prefect (-2)

These were false positives. @classmethod (and @staticmethod, @property) are descriptor factories that produce descriptor objects. Using them outside a class body is a valid and common Python pattern — the resulting descriptor is typically assigned to a class later. In dispatch.py, the classmethod descriptor _register_subclass_of_base_type is assigned to classes via setattr at line 135. In pydantic.py, the classmethod descriptor dispatch_key_from_type_field is assigned via setattr at line 158. Pyrefly was incorrectly treating these as invalid, and the PR correctly removes this overly strict check.
Attribution: The change in pyrefly/lib/alt/function.rs removed SpecialDecorator::ClassMethod, SpecialDecorator::StaticMethod, SpecialDecorator::Property, and SpecialDecorator::CachedProperty from the match arms in the method-only decorator validation. This means these four decorators are no longer flagged with invalid-decorator when used outside a class body. The test file pyrefly/lib/test/decorators.rs confirms the intent by removing the corresponding test expectations and adding a new test_top_level_descriptor_decorators test that validates these decorators work correctly at module level.


Was this helpful? React with 👍 or 👎

Classification by primer-classifier (5 LLM)

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

“Decorator @X can only be used on methods” is too strict

2 participants