Skip to content
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

[pylint] - restrict iteration-over-set to only work on sets of literals (PLC0208) #13731

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

diceroll123
Copy link
Contributor

@diceroll123 diceroll123 commented Oct 13, 2024

Summary

Restricts iteration-over-set to only work on sets of literals. There are valid use cases for iterating over a set, for de-duplication purposes.

Fixes #11694

Test Plan

cargo test

@diceroll123
Copy link
Contributor Author

I'm on the fence about still emitting a diagnostic without a fix even for "intentional" use of a set iteration, please advise. 😄

Copy link
Contributor

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

@MichaReiser MichaReiser added rule Implementing or modifying a lint rule preview Related to preview mode features labels Oct 14, 2024
Copy link
Member

@MichaReiser MichaReiser left a comment

Choose a reason for hiding this comment

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

Limiting the rule to only case where we can prove that the values are unique seems reasonable to me. Wdyt @AlexWaygood

But I suggest that we use ComparableExpr to ensure that the literals are unique.

Comment on lines +49 to 54
// Only suggest a fix if all elements are literals.
// This is because we can't determine if the set is used to de-dupe
// the output values of a call, operation, etc.
if !set.iter().all(Expr::is_literal_expr) {
return;
}
Copy link
Member

Choose a reason for hiding this comment

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

Technically, this also applies to literals. E.g. it's possible that someone wrote set(1, 2, 2) where fixing it to [1, 2, 2] would result in a semantic change.

That's why I'm inclined that we should instead use ComparableExpr to test if no two set entries are identical. See e.g.

let comparable_key = ComparableExpr::from(key);

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
preview Related to preview mode features rule Implementing or modifying a lint rule
Projects
None yet
Development

Successfully merging this pull request may close these issues.

PLC0208 should only recommend fixes when iterating over literals
2 participants