Skip to content

Fix set in-place operators with self argument #6661

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
youknowone merged 1 commit into from
Jan 9, 2026
Merged

Fix set in-place operators with self argument #6661

youknowone merged 1 commit into from
Jan 9, 2026

Conversation

@HueCodes
Copy link
Contributor

@HueCodes HueCodes commented Jan 6, 2026 •
edited by youknowone
Loading

Fixes #3881
close #3912
close #4424
close #4708

When a set performs in-place operations with itself (s &= s, s -= s, s ^= s), the operators now handle the self-reference case correctly:

  • __iand__: no-op (intersection with self is unchanged)
  • __isub__: clear (difference with self is empty)
  • __ixor__: clear (symmetric difference with self is empty)

Summary by CodeRabbit

  • Bug Fixes

    • Corrected behavior of in-place set operations (&=, -=, ^=) when a set operates on itself, preventing unintended modifications.

  • Tests

    • Added test cases validating in-place set operations when operating on the same set instance.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Jan 6, 2026 •
edited
Loading

📝 Walkthrough

Walkthrough

The changes add self-reference checks to in-place set operations (__iand__, __isub__, __ixor__) in the VM builtins to prevent redundant operations when a set operates on itself. A helper method detects self-references, and corresponding test cases validate the corrected behavior.

Changes

Cohort / File(s) Change Summary
Set In-Place Operations Self-Reference Handling
crates/vm/src/builtins/set.rs		 Added is_same_object(&self, other: &PyObject) -> bool helper method to detect self-references. Modified __iand__, __isub__, and __ixor__ to short-circuit when the operand is the same object: intersection with self remains unchanged, difference and XOR with self clear the set.
Test Cases for Set In-Place Self Operations
extra_tests/snippets/builtin_set.py		 Added test cases for in-place set operations on self: a &= a (unchanged), a -= a (clears), a ^= a (clears), with assertions verifying expected results.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

Suggested reviewers

  • arihant2math

Poem

🐰 Whiskers twitch with glee,
Sets now know when self they see—
a &= a stays true,
a -= a makes it new (empty!),
Pythonic magic, tried and true! ✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1
❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 60.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and specifically describes the main change: fixing set in-place operators when used with self argument, matching the primary objective.
Linked Issues check ✅ Passed The PR successfully implements all three required fixes from issue #3881: iand handles self-reference correctly (no-op), isub clears the set, and ixor clears the set, matching CPython behavior.
Out of Scope Changes check ✅ Passed All changes are directly scoped to the linked issue requirements: conditionals in in-place set operators and test cases validate the self-reference behavior.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

ShaharNaveh
ShaharNaveh approved these changes Jan 6, 2026
View reviewed changes
Copy link
Collaborator

@ShaharNaveh ShaharNaveh left a comment

Choose a reason for hiding this comment

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

lgtm

@fanninpm
Copy link
Contributor

fanninpm commented Jan 6, 2026

Could you please rebase this branch on a fresh copy of main?

@HueCodes
Copy link
Contributor Author

HueCodes commented Jan 6, 2026

Yes! I will rebase this afternoon when I get home. Thanks for the quick response.

@HueCodes HueCodes force-pushed the fix-set-inplace-self-ops branch from 7a72db2 to 36bace4 Compare January 7, 2026 00:02
youknowone
youknowone requested changes Jan 8, 2026
View reviewed changes
Copy link
Member

@youknowone youknowone left a comment

Choose a reason for hiding this comment

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

Thank you for contributing! The operation parts are looking perfect!
I left a suggestion to replace is_same_object to common RustPython internal mechanism. Please check it.

crates/vm/src/builtins/set.rs Outdated
Comment on lines 1318 to 1320
fn is_same_object(&self, other: &PyObject) -> bool {
self.object.is(other)
}
Copy link
Member

@youknowone youknowone Jan 8, 2026

Choose a reason for hiding this comment

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

We have a mechanism to do this.

is is not a method of PyObject, but an object ofAsObject. And PyObject is implementing AsObject through Borrow.

AnySet can do the same thing.
impl Borrow<PyObject> for AnySet {}
will enable AsObject for AnySet. Then set.is_same_object(zelf.as_object()) above can be simply written as set.is(zelf.as_object())

@HueCodes HueCodes force-pushed the fix-set-inplace-self-ops branch from 36bace4 to dcbf4fb Compare January 9, 2026 05:23
@HueCodes
Copy link
Contributor Author

HueCodes commented Jan 9, 2026

Done.

youknowone
youknowone approved these changes Jan 9, 2026
View reviewed changes
Copy link
Member

@youknowone youknowone left a comment

Choose a reason for hiding this comment

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

Great! Thank you so much for contributing!

@youknowone youknowone merged commit 83a0dea into RustPython:main Jan 9, 2026
13 checks passed
terryluan12 pushed a commit to terryluan12/RustPython that referenced this pull request Jan 15, 2026
@HueCodes @terryluan12
Fix set in-place operators with self argument (RustPython#6661)
4f66dec 
Co-authored-by: Hugh <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@youknowone youknowone youknowone approved these changes

+1 more reviewer

@ShaharNaveh ShaharNaveh ShaharNaveh approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

set intersection_update, difference_update, symmetric_difference_update should check self is equal to given arg

4 participants

@HueCodes @fanninpm @youknowone @ShaharNaveh