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.

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

Implement perflint #4789

Open
6 of 11 tasks
q0w opened this issue Jun 1, 2023 · 12 comments
Open
6 of 11 tasks

Implement perflint #4789

q0w opened this issue Jun 1, 2023 · 12 comments
Labels
accepted Ready for implementation plugin Implementing a known but unsupported plugin

Comments

@q0w
Copy link

q0w commented Jun 1, 2023 •
edited by charliermarsh
Loading

https://github.com/tonybaloney/perflint - Python Linter for performance anti patterns.

  • PERF101: Unnecessary list() on already iterable type (unnecessary-list-cast)
  • PERF102: Incorrect iterator method for dictionary (incorrect-dictionary-iterator)
  • PERF201: Loop invariant statement (loop-invariant-statement)
  • PERF202: Global name usage in a loop (loop-global-usage)
  • PERF203: Try..except blocks have a significant overhead. Avoid using them inside a loop (loop-try-except-usage)
  • PERF204: Looped slicing of bytes objects is inefficient. Use a memoryview() instead (memoryview-over-bytes)
  • PERF205: Importing the "%s" name directly is more efficient in this loop. (dotted-import-in-loop)
  • PERF301: Use tuple instead of list for a non-mutated sequence. (use-tuple-over-list)
  • PERF401: Use a list comprehension instead of a for-loop (use-list-comprehension)
  • PERF402: Use a list copy instead of a for-loop (use-list-copy)
  • PERF403: Use a dictionary comprehension instead of a for-loop (use-dict-comprehension)
@charliermarsh charliermarsh added the plugin Implementing a known but unsupported plugin label Jun 1, 2023
@qdegraaf qdegraaf mentioned this issue Jun 2, 2023
Merged
2 tasks
@q0w
Copy link
Author

q0w commented Jun 7, 2023

Also rules from https://github.com/tonybaloney/anti-patterns
like tonybaloney/anti-patterns#4 and tonybaloney/anti-patterns#5

@qdegraaf
Copy link
Contributor

qdegraaf commented Jun 7, 2023

If that's a separate plugin it's best to make a different issue for that. As for perflint, I will wait until the first PR is merged before looking at further rules.

@dhruvmanila dhruvmanila mentioned this issue Jun 10, 2023
Closed
charliermarsh pushed a commit that referenced this issue Jun 13, 2023
@qdegraaf
[perflint] Add perflint plugin, add first rule PERF102 (#4821)
a477720 
## Summary

Adds boilerplate for implementing the
[perflint](https://github.com/tonybaloney/perflint/) plugin, plus a
first rule.

## Test Plan

Fixture added for PER8102

## Issue link

Refers: #4789
konstin pushed a commit that referenced this issue Jun 13, 2023
@qdegraaf @konstin
[perflint] Add perflint plugin, add first rule PERF102 (#4821)
b67e9ea 
## Summary

Adds boilerplate for implementing the
[perflint](https://github.com/tonybaloney/perflint/) plugin, plus a
first rule.

## Test Plan

Fixture added for PER8102

## Issue link

Refers: #4789
@qdegraaf
Copy link
Contributor

qdegraaf commented Jun 15, 2023 •
edited
Loading

Rules checklist:

EDIT: Added to Issue body

@evanrittenhouse
Copy link
Contributor

@qdegraaf I can pick up some of these - are there any that you're particularly interested in? Don't want to overstep

@charliermarsh
Copy link
Member

Thanks @qdegraaf - I went ahead and added that to the issue body :)

@qdegraaf
Copy link
Contributor

@qdegraaf I can pick up some of these - are there any that you're particularly interested in? Don't want to overstep

Awesome! I've got an almost complete PERF101. Otherwise haven't picked up anything yet. I'll open a draft PR for PERF101 and otherwise take your pick! Feel free to ping me on the Discord if you want to collaborate or check availability.

@qdegraaf qdegraaf mentioned this issue Jun 15, 2023
Merged
@evanrittenhouse
Copy link
Contributor

evanrittenhouse commented Jun 17, 2023 •
edited
Loading

I can grab PERF203 and PERF301 to start.

@evanrittenhouse evanrittenhouse mentioned this issue Jun 17, 2023
Merged
@qdegraaf
Copy link
Contributor

qdegraaf commented Jun 22, 2023 •
edited
Loading

I'll start looking at 401 and 402

EDIT: Picked up 403 in a separate PR as well. I can leave the 2X and 3X to you if you like @evanrittenhouse

This was referenced Jun 22, 2023
Merged
Closed
charliermarsh added a commit that referenced this issue Jun 22, 2023
@qdegraaf @charliermarsh
[perflint] Add PERF101 with autofix (#5121)
38e618c 
## Summary

Adds PERF101 which checks for unnecessary casts to `list` in for loops. 

NOTE: Is not fully equal to its upstream implementation as this
implementation does not flag based on type annotations
(i.e.):
```python
def foo(x: List[str]):
    for y in list(x):
        ...
```

With the current set-up it's quite hard to get the annotation from a
function arg from its binding. Problem is best considered broader than
this implementation.

## Test Plan

Added fixture. 

## Issue links

Refers: #4789

---------

Co-authored-by: Charlie Marsh <charlie.r.marsh@gmail.com>
@evanrittenhouse
Copy link
Contributor

@qdegraaf You can grab 3X as well! I'll do 2X, if that works for you?

@q0w
Copy link
Author

q0w commented Jun 23, 2023

If that's a separate plugin it's best to make a different issue for that.

Rules from perflint are derived from tonybaloney/anti-patterns.
These rules ( tonybaloney/anti-patterns#4 and tonybaloney/anti-patterns#5) are not yet implemented in perflint (tonybaloney/perflint#31)

So these 2 rules (join with generator and regex with IGNORECASE) should be part of perflint or RUF rules?

@qdegraaf
Copy link
Contributor

qdegraaf commented Jun 23, 2023 •
edited
Loading

If that's a separate plugin it's best to make a different issue for that.

Rules from perflint are derived from tonybaloney/anti-patterns. These rules ( tonybaloney/anti-patterns#4 and tonybaloney/anti-patterns#5) are not yet implemented in perflint (tonybaloney/perflint#31)

So these 2 rules (join with generator and regex with IGNORECASE) should be part of perflint or RUF rules?

@charliermarsh what's the protocol for this. I think it makes functional sense to add it to perflint but not sure if Ruff is ok with expanding upon upstream plugin scope for direct ports.

@qdegraaf You can grab 3X as well! I'll do 2X, if that works for you?

Cool, works for me! If I get round to them at some point in the near future I'll make sure to announce it here. Feel free to change your mind in the meantime if you find time before me.

charliermarsh pushed a commit that referenced this issue Jun 26, 2023
@evanrittenhouse
[perflint] Implement try-except-in-loop (PERF203) (#5166)
190bed1 
## Summary

Implements PERF203 from #4789, which throws if a `try/except` block is
inside of a loop. Not sure if we want to extend the diagnostic to the
`except` as well, but I thought that that may get a little messy. We may
also want to just throw on the word `try` - open to suggestions though.

## Test Plan
`cargo test`
charliermarsh pushed a commit that referenced this issue Jul 3, 2023
@qdegraaf
[perflint] Add PERF401 and PERF402 rules (#5298)
93b2bd7 
## Summary

Adds `PERF401` and `PERF402` mirroring `W8401` and `W8402` from
https://github.com/tonybaloney/perflint

Implementation is not super smart but should be at parity with upstream
implementation judging by:
https://github.com/tonybaloney/perflint/blob/c07391c17671c3c9d5a7fd69120d1f570e268d58/perflint/comprehension_checker.py#L42-L73

It essentially checks:

- If the body of a for-loop is just one statement
- If that statement is an `if` and the if-statement contains a call to
`append()` we flag `PERF401` and suggest a list comprehension
- If that statement is a plain call to `append()` or `insert()` we flag
`PERF402` and suggest `list()` or `list.copy()`

I've set the violation to only flag the first append call in a long
`if-else` statement for `PERF401`. Happy to change this to some other
location or make it multiple violations if that makes more sense.

## Test Plan

Fixtures were added with the relevant scenarios for both rules

## Issue Links

Refers: #4789
@charliermarsh charliermarsh added the accepted Ready for implementation label Jul 10, 2023
@flying-sheep
Copy link
Contributor

flying-sheep commented Jul 14, 2023 •
edited
Loading

PERF203 should only trigger when there’s no loop flow control (break, continue) in the try-catch statement. E.g. I don’t think there’s a better way to write the following:

for repo in ['R-patched', 'cran', 'bioc']:
    try:
        pkg_cache = self._fetch_cache(repo, pkg)
        break
    except HTTPError:
        pass
else:
    return None

@qdegraaf qdegraaf mentioned this issue Jul 27, 2023
Merged
@evanrittenhouse evanrittenhouse mentioned this issue Aug 13, 2023
Closed
charliermarsh pushed a commit that referenced this issue Sep 15, 2023
@qdegraaf
[perflint] Add PERF403 (#6132)
067a4ac 
## Summary

Adds `PERF403` mirroring `W8403` from
https://github.com/tonybaloney/perflint

## Test Plan

Fixtures were added based on perflint tests

## Issue Links

Refers: #4789
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
accepted Ready for implementation plugin Implementing a known but unsupported plugin
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@flying-sheep @charliermarsh @qdegraaf @q0w @evanrittenhouse