Skip to content

Conversation

mbyrnepr2
Copy link
Member

Refs pylint-dev/pylint#4987

Type of Changes

Type
πŸ› Bug fix
βœ“ ✨ New feature
πŸ”¨ Refactoring
πŸ“œ Docs

Description

The result of this merge-request is to let Pylint analyse __init__.pyi files.

Closes pylint-dev/pylint#4987

@codecov
Copy link

codecov bot commented May 15, 2023

Codecov Report

Merging #2182 (bde093b) into main (835de84) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #2182   +/-   ##
=======================================
  Coverage   92.60%   92.61%           
=======================================
  Files          94       94           
  Lines       10798    10812   +14     
=======================================
+ Hits         9999    10013   +14     
  Misses        799      799           
Flag Coverage Ξ”
linux 92.36% <85.71%> (+<0.01%) ⬆️
pypy 87.56% <85.71%> (+0.01%) ⬆️
windows 92.20% <85.71%> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Ξ”
astroid/interpreter/_import/spec.py 97.40% <100.00%> (ΓΈ)
astroid/modutils.py 86.01% <100.00%> (+0.09%) ⬆️

... and 2 files with indirect coverage changes

@jacobtylerwalls jacobtylerwalls added this to the 3.0.0a4 milestone May 15, 2023
@Pierre-Sassoulas Pierre-Sassoulas added the Enhancement ✨ Improvement to a component label May 15, 2023
Copy link
Member

@Pierre-Sassoulas Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great !

@mbyrnepr2 mbyrnepr2 merged commit 12c3d15 into pylint-dev:main May 16, 2023
@mbyrnepr2 mbyrnepr2 deleted the pylint_issue_4987_pyi_files branch May 16, 2023 08:30
@sighingnow
Copy link

sighingnow commented May 30, 2023

Only recognizing __init__.pyi is not sufficient for many cases, as ImportlibFinder._SUFFIXES doesn't contain .pyi so interface files like a/b/c.pyi won't take into effect.

Could we get such cases fixed as well?

/cc @Pierre-Sassoulas @jacobtylerwalls

@mbyrnepr2
Copy link
Member Author

mbyrnepr2 commented May 30, 2023

Thank you @sighingnow. I think I would need a small working example to be sure I understand what the issue is.
Let me see if I understood you correctly with the following example. Does this illustrate the issue from your point of view?:

(venv310) $ tree not_working_ok/
not_working_ok/
└── b
    └── c.pyi

1 directory, 1 file
(venv310) $ tree working_ok/
working_ok/
└── b
    └── c.py

1 directory, 1 file
(venv310) $ cat working_ok/b/c.py 
x = 42
(venv310) $ diff working_ok/b/c.py not_working_ok/b/c.pyi 
(venv310) $ 
(venv310) $ pylint working_ok/
************* Module b.c
working_ok/b/c.py:1:0: C0114: Missing module docstring (missing-module-docstring)
working_ok/b/c.py:1:0: C0103: Constant name "x" doesn't conform to UPPER_CASE naming style (invalid-name)
working_ok/b/c.py:1:0: W0612: Unused variable 'x' (unused-variable)

-----------------------------------
Your code has been rated at 0.00/10

(venv310) $ pylint not_working_ok/
(venv310) $ <should be pylint messages here but there is not>
(venv310) $ 
(venv310) $ pylint not_working_ok/b/c.pyi 
************* Module c
not_working_ok/b/c.pyi:1:0: C0114: Missing module docstring (missing-module-docstring)
not_working_ok/b/c.pyi:1:0: C0103: Constant name "x" doesn't conform to UPPER_CASE naming style (invalid-name)
not_working_ok/b/c.pyi:1:0: W0612: Unused variable 'x' (unused-variable)

Note that while looking into this I see that https://github.com/pylint-dev/pylint/blob/main/pylint/lint/pylinter.py#L603 will need to be updated to include "pyi" also; that will fix my example above although I'm not 100% sure if it matches your use-case.

@sighingnow
Copy link

@mbyrnepr2 Thanks for the case, that's exactly what I mean. I succeed by locally hacking ImportlibFinder._SUFFIXES to adding .pyi before .py. Not sure if there are any further troubles.

Background: I use mypy-protobuf to generate those pyi files for protobuf files, and the above hack successfully addressed the "no-name-in-module" error.

@mbyrnepr2
Copy link
Member Author

Perhaps you could open a pull-request with your working hack @sighingnow because I've tried this also and I don't see it working unfortunately.
In the meantime I'll open a pull-request in Pylint because I think that will be necessary.

@sighingnow
Copy link

Hi @mbyrnepr2,

I have opened the pull request:

I misunderstood the case above and would like to give a minimal example of my problem (sorry for the incorrect comment above).

I have (I use head to cat all files):

..../working_ok $ head -n 100 ./* 
==> ./c.py <==
def _init():
    import sys
    setattr(sys.modules[__name__], 'x', 42)
_init()

==> ./c.pyi <==
x: int = 42

==> ./d.py <==
from . import c
print('x from c: ', c.x)

==> ./__init__.py <==

From the case above you could see that c has defined x with an init function (protobuf works in a similar way) and we tell that c has x in the pyi file c.pyi. Without the hack in #2195, pylint says:

$ pylint d.py
************* Module working_ok.d
d.py:1:0: C0114: Missing module docstring (missing-module-docstring)
d.py:2:20: E1101: Module 'working_ok.c' has no 'x' member (no-member)

------------------------------------------------------------------
Your code has been rated at 0.00/10 (previous run: 0.00/10, +0.00)

After the hack in #2195, the error disappears:

$ pylint d.py
************* Module working_ok.d
d.py:1:0: C0114: Missing module docstring (missing-module-docstring)

------------------------------------------------------------------
Your code has been rated at 5.00/10 (previous run: 0.00/10, +5.00)

Note the hack in #2195 only resolves the problem above, and is not enough to support other cases (not very familiar with the internal of pylint).

@sighingnow
Copy link

Further tweaks the case above (adding y to d.py which doesn't exists in c.pyi as well), now pylint complaints as expected for y:

$ head -n 100 ./*
==> ./c.py <==
def _init():
    import sys
    setattr(sys.modules[__name__], 'x', 42)
_init()

==> ./c.pyi <==
x: int = 42

==> ./d.py <==
from . import c
print('x from c: ', c.x)
print('y from c: ', c.y)

==> ./__init__.py <==

@mbyrnepr2
Copy link
Member Author

mbyrnepr2 commented May 30, 2023

So, in other words if a module c.py creates a name x dynamically (which Pylint typically won't be able to understand) then when Pylint can't find the name in c.py it could check if the name exists in a c.pyi module and if it does, trust the c.pyi as the source of truth that the x name exists in c.py and hence there would be less no-member false positives. Correct me if I've misunderstood :)

To be honest I didn't consider that dynamically assigned names would be in-scope for pylint-dev/pylint#4987. My understanding is that the scope was more narrow than that - i.e. to make it possible for Pylint to be able to lint .pyi files (previously they were ignored or not picked-up by astroid when searching for modules).

@Pierre-Sassoulas @jacobtylerwalls what do you think?

@sighingnow
Copy link

to make it possible for Pylint to be able to lint .pyi files (previously they were ignored or not picked-up by astroid when searching for modules).

Agree that lint both .py and .pyi would be the correct solution. See also related issue: pylint-dev/pylint#6281

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement ✨ Improvement to a component
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[feature] Being able to use stub pyi files in pylint
4 participants