pylint-dev / pylint

It's not just a linter that annoys you!
https://pylint.readthedocs.io/en/latest/
GNU General Public License v2.0
5.19k stars 1.1k forks source link

False-positive `no-member` in comprehension in unreachable code from platform check #7240

Open Avasam opened 1 year ago

Avasam commented 1 year ago

Bug description

# pylint: disable=missing-module-docstring,invalid-name,unnecessary-comprehension

import sys

if sys.platform == "linux":
    import os
    print(os.getgroups())  # no error
    print([group for group in os.getgroups()])  # error
    print({group for group in os.getgroups()})  # error

Configuration

No response

Command used

pylint ./example.py

Pylint output

************* Module example
example.py:8:30: E1101: Module 'os' has no 'getgroups' member (no-member)
example.py:9:30: E1101: Module 'os' has no 'getgroups' member (no-member)

Expected behavior

Platforms and version checks to still work inside comprehensions

Pylint version

pylint 2.14.5
astroid 2.11.7
Python 3.9.13 (tags/v3.9.13:6de2ca5, May 17 2022, 16:36:42) [MSC v.1929 64 bit (AMD64)]

OS / Environment

Windows 10

Additional dependencies

No response

mbyrnepr2 commented 1 year ago

I wasn't able to reproduce this on a couple of different versions, for example the latest:

pylint 2.15.0-dev0
astroid 2.12.2
Python 3.10.4 (v3.10.4:9d38120e33, Mar 23 2022, 17:29:05) [Clang 13.0.0 (clang-1300.0.29.30)]

I notice the line-numbers of the output don't match the example provided; are we looking at the right output @Avasam?

I see this:

************* Module example
example.py:6:10: R1721: Unnecessary use of a comprehension, use list(os.getgroups()) instead. (unnecessary-comprehension)
example.py:7:10: R1721: Unnecessary use of a comprehension, use set(os.getgroups()) instead. (unnecessary-comprehension)

------------------------------------------------------------------
Your code has been rated at 6.67/10 (previous run: 6.67/10, +0.00)
mbyrnepr2 commented 1 year ago

Opened my eyes now :) This would require a new astroid brain, right Pierre? Many os.py functions are Unix-only: https://docs.python.org/3/library/os.html. Oh ignore me I see your control-flow label :)

Pierre-Sassoulas commented 1 year ago

We already have some brains that are os specific, but I think the problem here could be something else. pylint does not have the proper control flow to see that if sys.platform == "linux": means that on Linux the code is going to be ok. (Maybe it means we must put the attribute from all OSes in the astroid brain so we don't have false positives). Some investigations are required, imo.

Avasam commented 1 year ago

@mbyrnepr2 Line numbers are probably off because I forgot to paste the "disable" comments to remove pylint warnings that are not relevant to this issue. I've updated it. But it seems you've figured it out and been able to replicate :)

@Pierre-Sassoulas I am not familiar with Pylint's internal workings. Could you ignore anything that's in a simple, non-obfuscated version/platform check as defined by PEP-0484 ? (https://peps.python.org/pep-0484/#version-and-platform-checking). That's what Pyright does, without having to go as far as analyzing for literal types.

Also notice that in my example above, the error only happens in comprehensions. the simple os.getgroups() does not give me an error.

Pierre-Sassoulas commented 1 year ago

We already do something similar for version check guard so it would be a step further but it makes sense. Especially considering that they are grouped together in pep 484 as you pointed out.

nineteendo commented 1 month ago

Also seems to happen here, it would indeed be easier to ignore code not for the current platform and version:

# pylint: disable=missing-module-docstring,unused-import
import sys

if sys.platform != "win32":
    from signal import SIGCONT, SIGTSTP