Do not allow calling NotImplemented#2992
Do not allow calling NotImplemented#2992Louisvranderick wants to merge 1 commit intofacebook:mainfrom
Conversation
yangdanny97
left a comment
There was a problem hiding this comment.
The commit summary is right but the actual changes are for something else entirely, i think you got your commits mixed up
01b5d24 to
73c25c2
Compare
|
Sorry about that I had stacked an unrelated commit on this branch. It’s clean now: only the #2918 NotImplemented change remains after rebasing on latest main and force-pushing. |
5623998 to
99fa21b
Compare
stroxler
left a comment
There was a problem hiding this comment.
I think the files (see the Changes view on the PR) are still not right.
It looks to me like there are multiple enum-related features mixed into the changes associated with this PR (for example, no @dataclass decorator on Enum)
|
@stroxler are you working on this? or else i can work on it. |
|
@iamPulakesh I assigned the PR review to myself, but I'm not working on the issue. I'm not sure whether @Louisvranderick is planning to do more work - at the moment I think there are some issues with unrelated code in the PR (and tests failing) |
Thanks for the update @stroxler! I looked at the issue and i think the fix could be handled by overriding the stub to drop the Any base, though I need to verify something else first. If @Louisvranderick doesn't get back to it, i'll be happy to work on it. I'll open a PR on this tomorrow. |
Applied my fixes #3101. Please take a look |
NotImplementedType stubs inherit Any, so has_base_any made the singleton callable via the implicit-Any call path. Exclude types.NotImplementedType and builtins._NotImplementedType from that branch. Tests: callable/calls regressions; return NotImplemented unchanged. Made-with: Cursor
ed59b58 to
df1fc7a
Compare
|
Thanks @yangdanny97 @stroxler — you were right the branch had unrelated enum / Happy for you to keep iterating on the broader enum / dataclass behavior separately; this PR is intentionally minimal so it can land without that scope. (There is also #3101 with another approach to the same issue — if maintainers prefer one implementation, I am fine closing this in favor of that.) |
|
Diff from mypy_primer, showing the effect of this PR on open source code: scipy (https://github.com/scipy/scipy)
+ ERROR scipy/stats/tests/test_qmc.py:538:20-29: Expected a callable, got `NotImplementedType` [not-callable]
+ ERROR scipy/stats/tests/test_qmc.py:543:24-33: Expected a callable, got `NotImplementedType` [not-callable]
|
Primer Diff Classification❌ 1 regression(s) | 1 project(s) total | +2 errors 1 regression(s) across scipy. error kinds:
Detailed analysis❌ Regression (1)scipy (+2)
Suggested fixesSummary: The PR correctly prevents NotImplemented() from being called, but causes false positives when NotImplemented is used as a sentinel placeholder in base classes that subclasses override with callable types. 1. In the callable checking logic in
Was this helpful? React with 👍 or 👎 Classification by primer-classifier (1 LLM) |
Summary
NotImplemented is a singleton, not callable, but bundled stdlib stubs declare NotImplementedType with an Any base. Pyrefly then treated instances as implicitly callable when no call was found (has_base_any → implicit Any call target). This change skips that implicit-call path for types.NotImplementedType and builtins._NotImplementedType, so NotImplemented(...) is reported as not callable. Adds a regression test in pyrefly/lib/test/callable.rs.
Fixes #2918
Test Plan
cargo test -p pyrefly test_notimplemented_not_callable
cargo test -p pyrefly test_return_notimplemented (ensure return NotImplemented behavior unchanged)
cargo test -p pyrefly test_no_missing_return_for_stubs
python3 test.py --no-test --no-conformance (no generated conformance files touched for this change)