pylint icon indicating copy to clipboard operation
pylint copied to clipboard

`import-self` only compares the name of a module and doesn't consider the actual module being imported

Open vapier opened this issue 3 years ago • 9 comments

Bug description

# This module is named "gzip".
#!/usr/bin/python3
"""Test"""
import gzip
gzip.main()

Configuration

No response

Command used

pylint gzip

Pylint output

************* Module gzip
gzip:5:0: W0406: Module import itself (import-self)
gzip:7:0: E1101: Module 'gzip' has no 'main' member (no-member)

Expected behavior

pylint shouldn't think a program named "foo" can be imported as "foo" when it doesn't have a .py extension

Pylint version

pylint 2.14.5
astroid 2.11.7
Python 3.9.2 (default, Feb 28 2021, 17:03:44) 
[GCC 10.2.1 20210110]

OS / Environment

Linux

Additional dependencies

No response

vapier avatar Aug 11 '22 03:08 vapier

Hi @vapier thanks for the inquiry. This predates my involvement by a lot, but the blame tells me this is intentional since at least #219.

jacobtylerwalls avatar Aug 20 '22 20:08 jacobtylerwalls

i don't think that conclusion is correct. being able to lint programs doesn't mean they have to be registered as importable modules.

vapier avatar Aug 21 '22 15:08 vapier

Thanks, I was having trouble understanding what you meant by "registered as an importable module." Do you mean import-self should not be emitted? (And not that the entire file should be ignored?)

In that case, having had a look at fac0b37f7cc1b9763d5a0e6f8c86e3c1922dc5e4, I guess we do have a flag we could check to see if this "was just some file" and not something importable ending in .py. (Although with some effort you could still import it, that's not relevant here, since a user who takes great effort to do that is probably not going to complain if we make import-self go away.)

If you don't mind, I'll retitle the issue to focus on import-self.

jacobtylerwalls avatar Aug 21 '22 16:08 jacobtylerwalls

Regarding the no-member in this example, we have #5151 for that.

jacobtylerwalls avatar Aug 21 '22 16:08 jacobtylerwalls

Maybe we should close #5151 in favor of this one and broaden the scope back out to all the other module-related checks (no-member, no-name-in-module) since we never got a response to @timmartin's reply. What do you think @vapier?

jacobtylerwalls avatar Aug 21 '22 16:08 jacobtylerwalls

as long as the issue is resolved (where a program cannot poison the set of importable modules), i'm fine with any approach

vapier avatar Aug 21 '22 21:08 vapier

@jacobtylerwalls Did you look at the code already?

no-member seems to come from: https://github.com/PyCQA/pylint/blob/e142ba8247276d25b3ff47e8f9fed06d0cc4753a/pylint/checkers/typecheck.py#L1063

Which returns the Module object and should probably be fixed in astroid somewhere.

import-self is much simpler: https://github.com/PyCQA/pylint/blob/e142ba8247276d25b3ff47e8f9fed06d0cc4753a/pylint/checkers/imports.py#L843-L844

That's just two strings being compared.

Although import-self might actually need to start using the Module object which would then make this dependent (probably) on the same future fix in astroid.

DanielNoord avatar Aug 21 '22 21:08 DanielNoord

Some very relevant (and funny in hindsight) comments: https://github.com/PyCQA/astroid/commit/6d59ad07d722d01e458aaf8fd14fd7dfc7ebaa6e https://github.com/PyCQA/astroid/commit/9684d0fb05c4721876c2b7274b6af3f3122ff62d

Currently relevant code is stored here: https://github.com/PyCQA/astroid/blob/16530f78c13863f584abcd874de9070d6f275072/astroid/nodes/_base_nodes.py#L140-L145

Edit: Found a fix 😄 Publishing PR to astroid. > https://github.com/PyCQA/astroid/pull/1747

DanielNoord avatar Aug 21 '22 21:08 DanielNoord

This issue has been slightly rescoped, see https://github.com/PyCQA/pylint/issues/5151#issuecomment-1222410308.

The issue about importing the correct module has been fixed and we should now follow this up in pylint by no longer doing the string comparison, but looking at the actual module. I'll add this to the 2.16 milestone so we don't forget to look at this when we bump astroid to 2.13.

Edit: A potential fix should also consider the following edge case and fix it: https://github.com/PyCQA/pylint/issues/7289

DanielNoord avatar Aug 25 '22 08:08 DanielNoord