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

C409 now makes code slower #12912

Open
hauntsaninja opened this issue Aug 15, 2024 · 7 comments
Open

C409 now makes code slower #12912

hauntsaninja opened this issue Aug 15, 2024 · 7 comments
Labels
preview Related to preview mode features rule Implementing or modifying a lint rule

Comments

@hauntsaninja
Copy link
Contributor

Introduced in #12657

tuple(i for i in range(10000)) is like 50% slower than tuple([i for i in range(10000)]). I think it's still fine to have this as a lint, but it's now impossible to configure ruff to distinguish between the two.

This is a thematically similar complaint to #8884

@hauntsaninja
Copy link
Contributor Author

hauntsaninja commented Aug 15, 2024

Oh this is also similar to #10838

@charliermarsh
Copy link
Member

These changes are all in preview and so we can still decide to change them before stabilizing. If we revert that change, though, we should do the same for the list rule variants.

I don’t feel strongly about it. I would also be open to making it configurable. Perhaps the setting should state whether the user prefers a comprehension or a generator, and enforce it one way or another? Or would the setting just ignore comprehensions?

@AlexWaygood AlexWaygood added rule Implementing or modifying a lint rule preview Related to preview mode features labels Aug 15, 2024
@hauntsaninja
Copy link
Contributor Author

hauntsaninja commented Aug 15, 2024

By list rule variant do you mean C410? I think that one is fine because it never introduces a generator comprehension.

It's maybe more dangerous to enforce list comprehensions over generator comprehensions. Also for probably mostly theoretical memory reasons, despite the perf impact, people typically seem more eager to go from list -> generator than generator -> list. So maybe I lean towards a setting to leave list comprehensions alone.

Or actually, maybe the slowening parts of C409 and C419 should be split into separate rules. The new rule that applies to comprehensions could then have a setting for whether you prefer list comprehensions or generator comprehensions (defaulting to generator). I think this would let everyone configure their ideal behaviour.

@charliermarsh
Copy link
Member

Sorry, I misspoke. I thought there was a rule that changed:

list([f(x) for x in foo])

To:

list(f(x) for x in foo)

But no, the fix there is [f(x) for x in foo], so that's fine.

@charliermarsh
Copy link
Member

I generally agree that this behavior should be configurable (or even removed). What do you think, @AlexWaygood?

@AlexWaygood
Copy link
Member

AlexWaygood commented Aug 16, 2024

I agree that these should either be two rules (one that makes your code slower and one that doesn't), or one configurable rule (where the default is to only emit the recommendations that do not make your code slower).

Some people prefer to always remove inner parentheses in calls like tuple([x for x in foo]) because they find it annoying when they're not necessary for semantics, and/or they feel that the inner parentheses are a "micro-optimisation" that you should only worry about in very performance-sensitive code. Other people prefer to always include them, because they want their code to be as fast as possible. I think both are reasonable points of view, but we shouldn't mix recommendations that have no negative performance implications with ones that could seriously slow down your code in the same rule. (At least, not with our default settings.)

@Avasam
Copy link

Avasam commented Aug 17, 2024

tuple(i for i in range(10000)) is like 50% slower than tuple([i for i in range(10000)]).

Oh this is also similar to #10838

And similar to the exact kind of example I requested there: #11839
I wouldn't mind merging my Feature Request with an already open issue.

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

No branches or pull requests

4 participants