Patch sys.path on a per-file basis

[DanielNoord]

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

Type of Changes

	Type
✓	🐛 Bug fix

Description

Closes #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.

[coveralls]

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%

Files with Coverage Reduction	New Missed Lines	%
pylint/checkers/variables.py	16	97.36%

Totals
Change from base Build 2926961300:	0.01%
Covered Lines:	16939
Relevant Lines:	17771

---

💛 - Coveralls

[Pierre-Sassoulas]

Should we cache this and/or fix_import_path now that we launch the function a lot ?

[DanielNoord]

fix_import_path is an iterator that changes sys.path so I don't think we can cache that.

I'll cache get_python_path.

[DanielNoord]

Oh never mind, I don't think get_python_path ever gets called with the same argument, as it is part of the exploration of modules and we don't "explore the same module twice".

[jacobtylerwalls]

I agree this looks promising. Some trivial cosmetic things.

[jacobtylerwalls]

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

[DanielNoord]

@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.

[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.

[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.

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 😢

[github-actions]

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

This comment was generated for commit 008b348

[DanielNoord]

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..