The C408 rule seems to fail on a dict with keyword arguments, but from the description of the rule, it should not fail.

$ cat scratch.py 
a = dict(one=1, two=2, three=3)

$ flake8 --version
3.8.2 (flake8-bugbear: 20.1.4, flake8-comprehensions: 3.2.2, mccabe: 0.6.1, pycodestyle: 2.6.0, pyflakes: 2.2.0) CPython 3.8.2
on Darwin

$ flake8 ~/Library/Preferences/PyCharmCE2019.3/scratches/scratch.py 
scratch.py:1:5: C408 Unnecessary dict call - rewrite as a literal.

The dict function has 3 forms: dict(**kwarg), dict(mapping, **kwarg), and dict(iterable, **kwarg). I'm not sure if it makes sense to apply this rule to every form.

Here's one example use case:

def foo(data):
    defaults = {'a': 1, 'b': 2}
    return dict(defaults, **data)

This example could be rewritten to use literals, but it would be less succinct and (I assume, but haven't confirmed) less performant.

When building package for openSUSE, the test suite fails in this one test test_C416_fail_1_list:

[   18s] =================================== FAILURES ===================================
[   18s] ____________________________ test_C416_fail_1_list _____________________________
[   18s]
[   18s] flake8dir = <pytest_flake8dir.Flake8Dir object at 0x7fa392a57160>
[   18s]
[   18s]     def test_C416_fail_1_list(flake8dir):
[   18s]         flake8dir.make_example_py("[x for x in range(5)]")
[   18s]         result = flake8dir.run_flake8()
[   18s]         # Column offset for list comprehensions was incorrect in Python < 3.8.
[   18s]         # See https://bugs.python.org/issue31241 for details.
[   18s]         col_offset = 1 if sys.version_info >= (3, 8) else 2
[   18s] >       assert result.out_lines == [
[   18s]             "./example.py:1:%d: C416 Unnecessary list comprehension - rewrite using list()."
[   18s]             % col_offset,
[   18s]         ]
[   18s] E       AssertionError: assert ['./example.p...sing list().'] == ['./example.p...sing list().']
[   18s] E         At index 0 diff: './example.py:1:1: C416 Unnecessary list comprehension - rewrite using list().' != './example.py:1:2: C416 Unnecessary list comprehension - rewrite using list().'
[   18s] E         Full diff:
[   18s] E           [
[   18s] E         -  './example.py:1:2: C416 Unnecessary list comprehension - rewrite using '
[   18s] E         ?                  ^
[   18s] E         +  './example.py:1:1: C416 Unnecessary list comprehension - rewrite using '
[   18s] E         ?                  ^...
[   18s] E
[   18s] E         ...Full output truncated (3 lines hidden), use '-vv' to show
[   18s]
[   18s] tests/test_flake8_comprehensions.py:840: AssertionError
[   18s] =========================== short test summary info ============================
[   18s] FAILED tests/test_flake8_comprehensions.py::test_C416_fail_1_list - Assertion...
[   18s] ======================== 1 failed, 63 passed in 13.74s =========================

Build log with all details.

Hi,

We have some code that looks like:

def is_valid(vm: str) -> bool:
  return vm in {machine for machine in machines}

And we got this lint error:

C412: Unnecessary (dict/list/set) comprehension - ‘in’ can take a generator.

This code is not particularly performance sensitive, so using a set or a generator probably does not make a big difference (and in fact probably the generator is faster!), but I am concerned that in other cases, applying the lint suggestion might result in a O(n) search rather than an amortised constant time.

In the original PR (#166) @adamchainz mentioned that checking for inclusion in some generators, such as range is optimised and that converting to a list / set and checking for including incurs in more memory copies and slower time overall, but as far as I know this is not true in other cases such as the one we bumped into.

Maybe the lint message could be updated so folks are aware that it might have a performance penalty?

Cheers,

iter()

Example:

even_iterator = iter([i for i in range(1,100) if i%2 == 0])

This can be directly written as:

even_iterator = iter(i for i in range(1,100) if i%2 == 0)

Description

map(f, iterable) has great performance when f is a built-in function, and it makes sense if your function already has a name. But if you need to introduce a lambda, it's better (i.e., more readable and faster) to use a generator expression -- no function calls needed.

Good:

for y in map(my_function_that_already_exists, it):
    ...
ys = list(map(my_function_that_already_exists, it))
ys = set(map(my_function_that_already_exists, it))
etc.

Suggestions:

# Bad:
map(lambda x: x**2, iterable)
# change to:
(x**2 for x in iterable)

# Bad:
list(map(lambda x: x.attr, iterable))
# change to:
[x.attr for x in iterable]
# OR change to:
y = list(map(operator.attrgetter("attr"), iterable))

# Bad:
z = map(lambda x: f(x), iterable)
# change to:
z = map(f, iterable)

Here are some perf numbers (deque(..., maxlen=0) is a fast way to consume an iterable):

PS C:\Users\sween> py -3.8 -m pyperf timeit -s "from collections import deque" "deque(map(lambda x: -x, range(1_000_000)), maxlen=0)"
Mean +- std dev: 85.7 ms +- 1.7 ms

PS C:\Users\sween> py -3.8 -m pyperf timeit -s "from collections import deque" "deque((-x for x in range(1_000_000)), maxlen=0)"

Mean +- std dev: 63.1 ms +- 2.9 ms

Similar things hold for filter().

Hi!

First of all, thanks for creating this plugin! I've been using it for years now and it's great 👏

This PR is more of a question than a clear suggested improvement; I thought about raising an issue to discuss it, but a PR seemed more actionable in case you agree with the rationale.

Changes

So the PR simply defines __slots__ on the plugin class, to pre-declare the class attributes we expect and in turn, make it a little bit more performant.

I did some memory profiling using memory_profiler and from what I can tell, this reduces the memory footprint of the plugin class by 15-20%. I also tested using sys.getsizeof and found basically the same results there.

The drawback of declaring __slots__, is of course that it prevents you from setting new instance variables during runtime, but as far as I can tell (and I could be wrong), flake8 never does this. I don't know of any other flake8-plugins that have this declared, so there could be a reason why this is a bad idea, but they are not clear to me, and the test-suite seems to have passed 🎉

Curious to see what your take on this is 😌

Thanks again 👏

It would be nice if a project can specify that they need '2.6' support, and only rules that dont break 2.6 support would be active.

For example, the set rules in #2 rely on Python 2.7 syntax. As a result, we need to turn off those codes, despite those codes having many good rules that are applicable and valid on Python 2.6, because a few rules are not.

https://gerrit.wikimedia.org/r/#/c/302103/

It would be especially lovely if you supported min-version = 2.6 that flake8-future-import uses, which also allows for other versions to have specific syntax that affects these rules.

This isnt strictly a case for comprehension, but seems to fit nicely with your other errors. sorted([1, 3, 2]) is an unnecessary list, that should be replaced with a tuple as the iterable processor sorted (and any other readonly processor) doesnt need a list.

i.e. this should be

sorted((1, 3, 2))

Python Version

3.7.1

flake8 Version

3.9.2

Package Version

latest

Description

Hi, thanks for adding type hints to the project. The type for ComprehensionChecker.__init__.tree is ast.Module, but should be ast.AST.

I'm happy to open a PR if you like

The following issues the C407 warning (as desired)

results = all([m.delete() for m in q.where_in(self._other_key, ids).get()])

however if you appease it and switch the code to

results = all((m.delete() for m in q.where_in(self._other_key, ids).get()))

or

results = all(m.delete() for m in q.where_in(self._other_key, ids).get())

It causes a subtle bug due to the new laziness gained by the generator expression

This can be evidenced here

>>> all((print(i) for i in range(1, 10)))
1
False
>>> all((print(i) or True for i in range(1, 10)))
1
2
3
4
5
6
7
8
9
True

This warning should probably remind/warn (haha) the user that there is a risk in changing it to follow the warning if the generate has a side effecting function in it

Description

(Thanks for flake8-comprehensions, it's a good plugin) Check for f(x) for x in iterable in any comprehension, and suggest replacing it with map(f, iterable). This is not objectively better like some other rules are, but IMO this is cleaner.

Description

List comprehension can postpone the evaluation of any/all, which can hurt performance for larger iterables. Surprisingly, this was not yet included here as rule.

Example:

def hi():
    print('hi')
    return True

>>> any(hi() for num in [1, 2, 3, 4])
hi

>>> any([hi() for num in [1, 2, 3, 4]])
hi
hi
hi
hi

From this answer

Proposal

Extend the current set of rules with C418 and C419 (any, all), similar to C403 and C404

Hello, I inherited a code with a lot of pattern like this:

for val in list(some_dict.values()):
    do_something(val)

As well as the list comprehension counterpart. As this plugin helped me a lot to catch a lot of other list comprehension and list/set/dict abuse, I wondered if such a pattern could be added to C411 (or another code).

zip https://docs.python.org/3/library/functions.html?highlight=zip#zip

How come I forgot to mention this one in the last issue I don't know, since I use it quite often :)