pylint icon indicating copy to clipboard operation
pylint copied to clipboard

Patch ``sys.path`` on a per-file basis

Open DanielNoord opened this issue 3 years ago • 7 comments

  • [x] Add yourself to CONTRIBUTORS if you are a new contributor.
  • [x] Add a ChangeLog entry describing what your PR does.
  • [x] If it's a new feature, or an important bug fix, add a What's New entry in doc/whatsnew/<current release.rst>.
  • [x] Write a good description on what the PR does.

Type of Changes

Type
:bug: Bug fix

Description

Closes https://github.com/PyCQA/pylint/issues/7339

I agree with @gcbirzan-plutoflume that the updated test outputs seem like they are actually beter. The change itself also makes sense imo, as Python also fixes sys.path once. See the issue for some further discussion.

DanielNoord avatar Aug 25 '22 14:08 DanielNoord

Pull Request Test Coverage Report for Build 2948717043

  • 18 of 19 (94.74%) changed or added relevant lines in 3 files are covered.
  • 16 unchanged lines in 1 file lost coverage.
  • Overall coverage increased (+0.01%) to 95.318%

Changes Missing Coverage Covered Lines Changed/Added Lines %
pylint/lint/utils.py 11 12 91.67%
<!-- Total: 18 19
Files with Coverage Reduction New Missed Lines %
pylint/checkers/variables.py 16 97.36%
<!-- Total: 16
Totals Coverage Status
Change from base Build 2926961300: 0.01%
Covered Lines: 16939
Relevant Lines: 17771

💛 - Coveralls

coveralls avatar Aug 25 '22 15:08 coveralls

@DanielNoord Have you tested this with namespace packages? Will it be a problem that is_namespace() is cached in astroid?

jacobtylerwalls avatar Aug 27 '22 19:08 jacobtylerwalls

@DanielNoord Have you tested this with namespace packages? Will it be a problem that is_namespace() is cached in astroid?

Hmm, I'm not sure. Do you think it should?

The primer failures show that this has significant performance regressions so I'm going to investigate (and fix) those before merging.

DanielNoord avatar Aug 28 '22 16:08 DanielNoord

I guess I was wondering if we should be doing less caching: if a module name that was cached by astroid's is_namespace may or may not be a namespace depending on whether sys.path was patched in pylint.

jacobtylerwalls avatar Aug 29 '22 11:08 jacobtylerwalls

I guess I was wondering if we should be doing less caching: if a module name that was cached by astroid's is_namespace may or may not be a namespace depending on whether sys.path was patched in pylint.

If anything I think this should reduce the occurrence of this issue (the original issue describes a similar issue actually).

My thinking of this is as follows. Take file A1, A2 and B1. A en B represent two packages which are both supplied on the command line, and after expand_modules A turns out to have two files. Currently we would patch sys.path with both A and B, which is what is creating the issue as described in the original issue. As far as I can see (for now) whether or not A is a namespace package only depends on the file structure of package A. The patching of B onto sys.path should never correctly make A namespace and can only do so incorrectly as in the issue. Since we know patch sys.path with A or A1/A2 if we can be even more specific (something,) vs. (file.filepath,) we never lint A1/A2 while patching B. This should avoid the previous issues.

The only thing I can think of that would be problematic is if package A imports package B before we lint package B and we determine during that import that B is namespace. However, I think that if we determine that it is namespace then it is likely correct: if you execute package A normally the interpreter also wouldn't magically patch package B onto your sys.path I think. So even if we cache some package as namespace before actually linting it is likely correct?

Not sure if I have explained myself enough here.

Also, lru_cache doesn't seem to work sadly 😢

DanielNoord avatar Aug 29 '22 11:08 DanielNoord

🤖 According to the primer, this change has no effect on the checked open source code. 🤖🎉

This comment was generated for commit 008b3485993544b81e6c1ce1f0952cdbafa4b4d2

github-actions[bot] avatar Aug 29 '22 12:08 github-actions[bot]

Not really sure what to do with this PR. It's clear that creating the contextmanager for every file has significant performance impact, but I also can't think of a solution without doing so...

The contextmanager itself also isn't that large so there isn't really anything to refactor to make it quicker to set up..

DanielNoord avatar Sep 03 '22 12:09 DanielNoord