Repository navigation
Allow reflected fallback for in-place set operators - #9182
Conversation
Assisted-by: Codex:gpt-6
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Merge Risk: ⚪ Minimal · up to
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 860ea6e3c7
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| }), | ||
| inplace_subtract: Some(|a, b, vm| { | ||
| if let Some(a) = a.downcast_ref::<PySet>() { | ||
| if let Some(a) = a.downcast_ref::<PySet>() |
There was a problem hiding this comment.
is there any case PySet but not PyAnySet?
There was a problem hiding this comment.
the left operand needs to remain a PySet because these slots call mutating PySet::__i*__ methods. PyAnySet would also accept frozenset, which cannot be modified in place. Augmented assignment on a frozenset uses the regular binary operation and reassigns the result instead.
the right operand is only read, so accepting PyAnySet there is appropriate.
There was a problem hiding this comment.
I am still not getting how it concludes to add PyAnySet check.
if a is downcasted PySet, It can't be a frozenset. isn't it?
What's the role of the PyAnySet check?
There was a problem hiding this comment.
the condition now checks each operand separately:
a(left-hand operand): PySet — this check already existed before this PR. These slots mutate the left operand, so it must be a mutable set. You’re right that a successful downcast already excludes frozenset.b(right-hand operand): PyAnySet — this check is added by this PR. The in-place methods accept either set or frozenset here. For other types, such asdict_keys, the slot should returnNotImplemented. Previously,try_from_object(...)?raisedTypeErrorimmediately, preventing normal binary dispatch from reaching the right operand’s reflected method, such as__ror__.
There was a problem hiding this comment.
oh, you are right. I was watching b as a and very confused 🤦
Thank you for clearly pointing it out!
Found while constructing a parser with Lark, where
set |= OrderedSetraisedTypeErrorinstead of calling the reflected operator.Summary
NotImplementedfrom set's|=,&=,-=, and^=operators for unsupported right operands, allowing their reflected operators to run.AI assistance
Written with Codex (GPT-6), reviewed by a human before submission.
Summary by CodeRabbit