Skip to content

Fix set bugs with &= and -= #4424

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.

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Conversation

branai
Copy link
Contributor

@branai branai commented Jan 6, 2023

I changed the logic for intersection_update and difference_update to fix some bugs with set operations that call these functions.

Fixes #3881 #3992

@fanninpm
Copy link
Contributor

fanninpm commented Jan 6, 2023

I believe you can use the following command to test your changes locally:

# this runs all of the tests (not necessary if you know which test is affected)
cargo run --release --features ssl,jit -- -m test -j 1 -u all --slowest --fail-env-changed -v
# this runs only the test suite named "test_whatever" (usually located at `Lib/test/test_whatever.py`)
cargo run --release --features ssl,jit -- -m test -j 1 -u all --slowest --fail-env-changed -v test_whatever

For a list of all resources and more command-line options, you can execute cargo run --release --features ssl,jit -- -m test -h.

@branai
Copy link
Contributor Author

branai commented Jan 6, 2023

If I am failing tests with

# TODO: RUSTPYTHON
    @unittest.expectedFailure

should I remove them?

@DimitrisJim DimitrisJim self-assigned this Jan 6, 2023
@DimitrisJim
Copy link
Member

DimitrisJim commented Jan 6, 2023

So the main idea here is that we want to skip iterations when we intersect/difference/symmetric_difference a set with itself. Though the PR fixes faulty assumptions in intersect_update and difference_update (the assumption that the set(s) we supply as an argument isn't the same set as self) it doesn't do the short-circuiting, i.e s = {*range(20)}; s.intersection_update(s, s, s) would iterate over every element in the arguments when it could just skip them all since s &= s is s.

The best approach is along the lines of #3912, we accept objects as arguments for intersection_update/difference_update (AnySet works fine for iand/isub) and compare these objects with is, this is fast O(1) and allows us to ignore sets or clear them depending on the function.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants