[n1shanthb]

Summary

Fixes #5294.

Added a bad-function-definition diagnostic for classmethods that don't accept the implicit class argument, including implicit classmethods such as __init_subclass__ and __class_getitem__.

The check handles positional-only parameters, regular positional parameters and *args, without requiring the first parameter to be named cls.

It uses the existing method flags and remaining non-special decorator list, skipping custom-decorated functions and mixed staticmethod/property cases.

Consolidated the regression tests into three groups: missing receivers, accepted signatures and decorator interactions. These cover aliases, implicit classmethods, abstract methods, protocols, overloads and signature-changing decorators.

Decorator identity classification, including shadowed deprecated(...) factories, is deferred to a possible separate PR.

• I am a human, and I fully understand the code I am submitting.

AI assisted with investigation, implementation and testing. I reviewed the changes and understand the implementations. The review-revision summary and validation update in this description were generated with AI assistance.

Test Plan

• Classmethod-filtered tests: cargo test --locked -p pyrefly --lib test_classmethod -- --test-threads=2 — 22 passed, including the three consolidated regression groups.
• Formatting/lint workflow: python3 test.py --no-test --no-tensor-shapes --no-conformance --no-jsonschema --no-extension — passed.
• cargo fmt --all -- --check — passed.
• cargo clippy --locked -p pyrefly --lib --tests — passed with existing warnings in unchanged files.
• git diff --check — passed.
• Broader method/decorator validation: 962 passed, 38 failed. The same selection on a freshly built upstream base commit e5f077b produced 959 passed and the identical 38 failing tests; the three additional passes are the new regression groups.
• Conformance tests in WSL: runner exited successfully; 129/145 cases conformed, with 55 mismatches across 16 cases. All three generated output files were byte-for-byte identical to freshly built upstream e5f077b; no conformance output changes are included.
• The full library suite was not rerun for this review revision. The earlier run on 1a6e4de completed with 9,111 passed, 246 failed and 3 ignored; its failing tests also reproduced on upstream e5f077b.

[meta-cla]

Hi @n1shanthb!

Thank you for your pull request and welcome to our community.

Action Required

In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at cla@meta.com. Thanks!

[meta-cla]

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks!

[meta-cla]

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks!

[kinto0]

Thanks for the PR. The check looks correct, but the implementation can be smaller. A few requests:

1. Use decorators.is_empty() instead of the new flags. After the decorator loop, decorators holds only the decorators that are not special. The property check below already uses this to skip functions with custom decorators. So you can remove has_classmethod_decorator, can_check_classmethod_receiver and the matches! allowlist:

if defining_cls.is_some()
    && flags.is_classmethod
    && !flags.is_staticmethod
    && flags.property_metadata.is_none()
    && decorators.is_empty()
    && def.parameters.posonlyargs.is_empty()
    && def.parameters.args.is_empty()
    && def.parameters.vararg.is_none()

With the allowlist, someone must update this check each time a new SpecialDecorator is added. Also, some of the current conditions repeat or contradict each other. NoTypeCheck is in the allowlist but !flags.has_no_type_check excludes it. The allowlist already excludes StaticMethod and Property, so the is_staticmethod and property_metadata checks are redundant.

2. Include implicit classmethods. def init_subclass() -> None without @classmethod also fails at runtime: it raises TypeError when a subclass is created. If you use flags.is_classmethod, this case is covered with no extra code. Please update the test that expects no error for it.

3. Move the deprecated workaround into its own fix. Decorator.deprecation is set by name during binding. So a local function named deprecated is treated as @deprecated and removed from decorators everywhere, not only in this check. The property check has the same problem. Please remove the local workaround here. The right fix is to classify deprecated from its resolved identity, in a separate PR. Your test_classmethod_receiver_shadowed_deprecated test is a good fit for that PR.

4. Smaller items:

• Can we make the error message more specific, for example: Class method f must accept the class as its first positional parameter.

• Please merge the 7 test cases into about 3: missing receiver, accepted signatures, and decorator interactions. Please also remove the unrelated cases (instance methods, nested functions, module-level functions) from test_classmethod_receiver_capacity.

• After change 1, the property check's comment covers this logic, so you can remove the two new comments.

Thank you!!

[n1shanthb]

Thanks for the review @kinto0! Im working on the changes now and will push an update shortly.

[meta-codesync]

This pull request has been imported. If you are a Meta employee, you can view this in D128006144. (Because this pull request was imported automatically, there will not be any future comments.)

[n1shanthb]

Thanks for reviewing @kinto0. I simplified the check, added implicit classmethod coverage, updated the diagnostic and merged the tests. I also removed the deprecated workaround. Focused tests pass and broader failures match upstream. Could you take another look?

[stroxler]

Review automatically exported from Phabricator review in Meta.

[github-actions]

Closing this PR because linked issue #5294 was closed.