Repository navigation
Allow subclass receivers on overloaded constructors - #4722
Divyansh151005 wants to merge 1 commit into
Conversation
|
Hi @Divyansh151005! Thank you for your pull request and welcome to our community. Action RequiredIn 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. ProcessIn 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 If you have received this in error or have any questions, please contact us at cla@meta.com. Thanks! |
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks! |
This comment has been minimized.
This comment has been minimized.
Merging this PR will degrade performance by 43.72%
|
| Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|
| ❌ | invalidate_all |
29.6 ms | 52.6 ms | -43.72% |
Tip
Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.
Comparing Divyansh151005:fix/constructor-overload-subclass-receivers (076572b) with main (9a8b4be)
Footnotes
-
9 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩
This comment has been minimized.
This comment has been minimized.
rchen152
left a comment
There was a problem hiding this comment.
Thanks for the PR! Sorry for the slow review. I think skipping validation altogether is too broad of a change. It causes a couple test failures due to pyrefly no longer detecting legitimate errors: https://github.com/facebook/pyrefly/actions/runs/36653011442/job/109691230864?pr=4722.
Fixes facebook#4701. Overloads of `__init__` and `__new__` may annotate the receiver with a subclass of the defining class so that construction via that subclass selects a more specific overload. Previously pyrefly rejected these receivers as "not a superclass" of the defining class. Only overload variants of constructors accept subclass receivers; the implementation and non-overloaded constructors are validated as before. Receivers of unrelated classes are still rejected, and the `__init__` class-scoped type parameter check still runs on overloads.
076572b to
0f12641
Compare
|
@rchen152 thanks for the review, agreed that was too broad. I've reworked it so constructor overloads are still validated and the only change is that a receiver whose class is a subclass of the defining class is now accepted on overload variants of So the two failures from that run ( |
|
This pull request has been imported. If you are a Meta employee, you can view this in D125874447. (Because this pull request was imported automatically, there will not be any future comments.) |
|
According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅ |
Summary
Overloads of
__init__and__new__may annotate the receiver with a subclass of the defining class, so that constructing via that subclass selects a more specific overload:Pyrefly rejected these receivers as "not a superclass" of the defining class.
This change keeps constructor receiver validation in place and only widens the accepted set: on overload variants of
__init__/__new__, a receiver whose class is a subclass of the defining class is also accepted. Everything else is unchanged:test_init_bad_receiver_annotation,test_new_bad_receiver_annotationunchanged);__init__class-scoped type parameter check still runs on overloads (test_constructor_overload_with_hint,test_constructor_typevar_scope_overloadpass).Fixes #4701
Test Plan
New tests in
pyrefly/lib/test/constructors.rs:test_new_overload_subclass_receiver: thePathexample above, withassert_typeon each constructor call.test_init_overload_subclass_receiver: same pattern for__init__.test_constructor_subclass_receiver_requires_overload: a subclass receiver on a non-overloaded__new__/__init__is still an error.