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.

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

Recognize stub pyi Python files. #2182

Merged
merged 5 commits into from
May 16, 2023

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

astroid/modutils.py Outdated Show resolved Hide resolved
astroid/modutils.py Show resolved Hide resolved
@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