Skip to content

fix special-case NotImplementedType in binop signatures to match runtime semantics #1129#2677

Draft
asukaminato0721 wants to merge 1 commit intofacebook:mainfrom
asukaminato0721:1129
Draft

fix special-case NotImplementedType in binop signatures to match runtime semantics #1129#2677
asukaminato0721 wants to merge 1 commit intofacebook:mainfrom
asukaminato0721:1129

Conversation

@asukaminato0721
Copy link
Contributor

Summary

Fixes #1129

fixed the binop resolution path so NotImplementedType no longer leaks as a possible operator result.

successful dunder calls now strip only the exact NotImplementedType branch, keep searching reflected dunders when needed, and union any concrete results that can actually occur at runtime.

Test Plan

a regression test covering both pure fallback and mixed `int | NotImplementedType behavior.

@meta-cla meta-cla bot added the cla signed label Mar 5, 2026
@asukaminato0721 asukaminato0721 marked this pull request as ready for review March 5, 2026 23:27
Copilot AI review requested due to automatic review settings March 5, 2026 23:27
Copy link

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 fixes issue #1129, where binary operations involving dunder methods that return NotImplementedType would incorrectly leak the NotImplementedType into the inferred result type instead of following Python's runtime semantics (trying the reflected dunder when the forward one signals NotImplemented).

Changes:

  • The try_binop_calls logic in operators.rs is updated to strip NotImplementedType from successful dunder return types, accumulate non-NotImplementedType results, and continue searching for reflected dunders when needed.
  • NotImplementedType is added as a new stdlib entry in Stdlib, gated to Python ≥ 3.10 (consistent with how EllipsisType is handled).
  • A regression test is added covering both the pure-NotImplementedType fallback and the mixed int | NotImplementedType scenario.

Reviewed changes

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

File Description
pyrefly/lib/alt/operators.rs Core fix: strips NotImplementedType from successful dunder results, accumulates partial results, and continues to reflected dunders when needed
crates/pyrefly_types/src/stdlib.rs Adds NotImplementedType as an Option<StdlibResult<ClassType>> stdlib entry, guarded by version >= 3.10
pyrefly/lib/test/operators.rs Regression test covering the bug from issue #1129 for both pure and mixed NotImplementedType return types

💡 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.

Comment on lines 204 to +207
errors.extend(callee_errors);
return ret;
if ret_without_not_implemented != ret {
successful_ret = self.union(successful_ret, ret_without_not_implemented);
continue;
Copy link

Copilot AI Mar 5, 2026

Choose a reason for hiding this comment

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

When a dunder returns int | NotImplementedType, the code extends errors with callee_errors at line 204 and then continues to look for a reflected dunder. If callee_errors is non-empty (e.g., because the method is not callable as a call target), those errors are emitted unconditionally — even if the reflected dunder later succeeds cleanly. This results in spurious error reporting.

The callee_errors extension should be deferred: accumulate them alongside successful_ret and only emit them at the return site (line 209 or line 215), similar to how first_call defers error emission until it is determined whether the call is ultimately the "best" result.

Copilot uses AI. Check for mistakes.
}),
None => ret.clone(),
};
if ret_without_not_implemented.is_never() {
Copy link

Copilot AI Mar 5, 2026

Choose a reason for hiding this comment

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

When all dunder methods exist and have no call errors, but all return only NotImplementedType (so ret_without_not_implemented.is_never() is true for every iteration), the code falls through to the "Cannot find __add__ or __radd__" error message. This message is misleading: the methods do exist, they just always return NotImplemented. A more accurate message could indicate that all matching operator methods always return NotImplemented.

Note: this scenario only arises in uncommon code where every dunder is annotated to always return NotImplementedType.

Suggested change
if ret_without_not_implemented.is_never() {
if ret_without_not_implemented.is_never() {
// All branches of this dunder call resolved to NotImplementedType.
// Record this call as the first attempted call (if none recorded yet)
// so that later error handling can distinguish "methods exist but
// always return NotImplemented" from "no dunder methods found".
if first_call.is_none() {
first_call = Some((callee_errors, call_errors, ret));
}

Copilot uses AI. Check for mistakes.
@github-actions
Copy link

github-actions bot commented Mar 6, 2026

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

werkzeug (https://github.com/pallets/werkzeug)
+ ERROR tests/test_datastructures.py:713:13-33: `|` is not supported between `EnvironHeaders` and `dict[str, str]` [unsupported-operation]
+ ERROR tests/test_datastructures.py:719:13-34: `|=` is not supported between `EnvironHeaders` and `dict[str, str]` [unsupported-operation]

@github-actions
Copy link

github-actions bot commented Mar 6, 2026

Primer Diff Classification

❌ 1 regression(s) | 1 project(s) total

1 regression(s) across werkzeug. error kinds: unsupported-operation. caused by resolve_binop_calls().

Project Verdict Changes Error Kinds Root Cause
werkzeug ❌ Regression +2 unsupported-operation resolve_binop_calls()
Detailed analysis

❌ Regression (1)

werkzeug (+2)

These are false positive errors. The test code shows that EnvironHeaders is expected to raise TypeError for | and |= operations (lines 712 and 718 use pytest.raises(TypeError)). This indicates EnvironHeaders intentionally doesn't support these operators. At runtime, when EnvironHeaders.or returns NotImplemented, Python's operator resolution would try the reflected operation dict.ror, and if that also fails or EnvironHeaders prevents it, a TypeError would be raised as expected. Pyrefly's new NotImplementedType handling is incorrectly flagging this as an unsupported operation when the test explicitly expects and handles this case. The error messages claim the operations are 'not supported', but the test code demonstrates they are handled (by raising TypeError), which is different from being unsupported at the type level.
Attribution: The change to binary operator resolution in pyrefly/lib/alt/operators.rs now properly handles NotImplementedType returns. The new logic in the resolve_binop_calls() function strips NotImplementedType from successful returns and continues searching for reflected dunders, which more accurately models Python's runtime operator resolution protocol.

Suggested Fix

Summary: The new NotImplementedType handling incorrectly flags legitimate NotImplemented returns as type errors when they should result in runtime TypeError.

1. In resolve_binop_calls() in pyrefly/lib/alt/operators.rs, after the main loop, check if all operations returned NotImplementedType by tracking whether any non-NotImplemented returns were found. If all operations returned NotImplemented, return a union that includes the possibility of TypeError being raised at runtime, rather than treating it as a type error.

Files: pyrefly/lib/alt/operators.rs
Confidence: high
Affected projects: werkzeug
Fixes: unsupported-operation
The current logic strips NotImplementedType and continues searching, but doesn't handle the case where all operations legitimately return NotImplemented. In Python's runtime, when all dunder methods return NotImplemented, a TypeError is raised. The type checker should model this as a valid outcome rather than flagging it as an unsupported operation. Expected outcome: eliminates 2 false positive errors in werkzeug where EnvironHeaders intentionally doesn't support | and |= operators.


Was this helpful? React with 👍 or 👎

Classification by primer-classifier (1 LLM)

@asukaminato0721 asukaminato0721 marked this pull request as draft March 6, 2026 00:52
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.

special-case NotImplementedType in binop signatures to match runtime semantics

2 participants