pylint-bot / pylint-unofficial

UNOFFICIAL playground for pylint github migration
0 stars 0 forks source link

Using --jobs affects monkey patching detection #502

Open pylint-bot opened 9 years ago

pylint-bot commented 9 years ago

Originally reported by: Pavel Roskin (BitBucket: pavel_roskin)


first.py:

import sys
sys.foo = 0

second.py:

import sys
sys.foo

pylint -E first.py second.py

No output

pylint -E --jobs=2 first.py second.py

************* Module second
E:  2, 0: Module 'sys' has no 'foo' member (no-member)

I'm actually fine if pylint reports that error in every case, as long as there is no import first in second.py before sys.foo is used. The original code that triggered the error message is overengineered and needs fixing.


pylint-bot commented 9 years ago

Original comment by Claudiu Popa (BitBucket: PCManticore, GitHub: @PCManticore):


It's not just with multiple jobs, it fails by reversing the order of the files in the CLI command as well. It doesn't report in the first case, because pylint uses a global cache of modules, astroid.manager.AstroidManager.astroid_cache, so when it processes second.py, sys is already present in this cache and it's already patched and pylint just grabs it from there. In the second case, sys is not there yet and it fails. When --jobs are involved, there's no guaranteed order, plus the fact that each job is a separate process. This suggests a possible fix, to clear the cache after each processed file, but that hits the performance, since the modules will have to be reprocessed over and over again for each new file. I'm inclined to close this as wont-fix, since it's such an edge case.

pylint-bot commented 9 years ago

Original comment by Pavel Roskin (BitBucket: pavel_roskin):


I think many people would prefer pylint to run ten times slower but detect 10% more issues. I'd rather run pylint overnight than release code where pylint failed to find a problem. Of course, slowing down pylint could also discourage people from using it in more casual situations (e.g. checking a trivial code change before committing it).

If running pylint on every file individually would produce more reliable results at the expense of processing time, then maybe it could be an option? With that option enabled, all caches would be cleaned when pylint starts processing a new file.

By the way, a reliable --jobs implementation would offset some of the additional time. Especially if --jobs is set by default to the number of CPUs.

pylint-bot commented 9 years ago

Original comment by Claudiu Popa (BitBucket: PCManticore, GitHub: @PCManticore):


Well, the question is if detecting monkey patching is a real problem. We'll have to investigate what other side effects the cleaning of cache has after all.

pylint-bot commented 9 years ago

Original comment by Claudiu Popa (BitBucket: PCManticore, GitHub: @PCManticore):


An option sounds good.

pylint-bot commented 9 years ago

Original comment by Pavel Roskin (BitBucket: pavel_roskin):


In my case, it wasn't a new information. It was already clear that defining __builtins__._ was a bad approach that needed replacement.

However, I know that running pylint on individual files produces more data than running it on the whole module. I'll see if pylint would find anything useful by checking each file separately. That would produce a more realistic motivation for a possible option.

pylint-bot commented 9 years ago

Original comment by Claudiu Popa (BitBucket: PCManticore, GitHub: @PCManticore):


Ah, that sounds perfect for additional-builtins option.