Skip to content

Conversation

@mohasarc
Copy link
Contributor

@mohasarc mohasarc commented Nov 4, 2025

Hi @AryazE,

I’ve noticed that InPlaceSortAnalysis incorrectly raises warnings for Set objects, even though sets can't be sorted and don’t have an in-place .sort() method.

For example:

a = set(range(0, 1001))
sorted(a)  # <--- This would currently raise a warning, but it shouldn't.

I’ve implemented a simple fix for this. Please have a look and let me know if any further changes are needed.

@AryazE AryazE self-requested a review November 4, 2025 09:34
Copy link
Collaborator

@AryazE AryazE 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 the PR.
Yes, this seems like a problem.
However, I think the fix you suggested restricts the checker too much. I think any collection or object that has .sort and has a length (too check for long sequences) should be caught.
I have made a suggestion for the change.
Could you also extend the tests with such cases?

if function is sorted:
# we have to keep the list in memory to keep id(pos_args[0]) stable ? nope!
if hasattr(pos_args[0], "__len__") and len(pos_args[0]) > self.threshold:
if type(pos_args[0]) is list and len(pos_args[0]) > self.threshold:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
if type(pos_args[0]) is list and len(pos_args[0]) > self.threshold:
if hasattr(pos_args[0], "sort") and hasattr(pos_args[0], "__len__") and len(pos_args[0]) > self.threshold:

@mohasarc
Copy link
Contributor Author

mohasarc commented Nov 4, 2025

I think your suggestion makes perfect sense, though we also need to update line 48 to perform the same check:

image

should be:

if len(self.stored_lists) > 0 and hasattr(val, "sort") and hasattr(val, "__len__"):

I'll perform both changes now

@mohasarc
Copy link
Contributor Author

mohasarc commented Nov 4, 2025

I made the changes. I also made a small refactor that I thought would be nice to reduce repetition.

@mohasarc mohasarc requested a review from AryazE November 4, 2025 20:01
Copy link
Collaborator

@AryazE AryazE left a comment

Choose a reason for hiding this comment

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

Thanks for the update.
I have renamed the function and added some tests.

@AryazE AryazE merged commit 1073a51 into sola-st:main Nov 5, 2025
4 checks passed
@mohasarc
Copy link
Contributor Author

mohasarc commented Nov 5, 2025

It look great, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants