Bug 101285 – webkitpy: integrate pylint into check-webkit-style, part I

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Dirk Pranke]

webkitpy: integrate pylint into check-webkit-style, part I

[Dirk Pranke]

Created attachment 172457 [details]
Patch

[WebKit Review Bot]

Attachment 172457 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Tools/ChangeLog', u'Tools/Scripts/lint-web..." exit_code: 1
Tools/Scripts/webkitpy/style/checkers/python_unittest_input.py:4:  trailing whitespace  [pep8/W291] [5]
Tools/Scripts/webkitpy/style/checkers/python_unittest_input.py:4:  Undefined variable 'error'  [pylint/E0602] [5]
Total errors found: 2 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Dirk Pranke]

The errors in that file are intentional; the output shows that this change works :).

[Dirk Pranke]

Note that the advantage to running pylint in addition to pep8 is that we will catch a *lot* more issues. The downside is that pylint is a lot slower.

We probably don't need to run both, but I haven't done enough side-by-side testing to be sure yet. Most (if not all) of the pep8 errors are pylint warnings, so we won't see duplication of messages, I think, though I'm not sure about actual syntax errors.

[Ojan Vafai]

Comment on attachment 172457 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=172457&action=review

> Tools/Scripts/webkitpy/style/checkers/python.py:100
> +        self._script_dir = os.path.abspath(os.path.dirname(os.path.dirname(os.path.dirname(os.path.dirname(__file__)))))

Can we instead walk up the path until we find a known directory and then walk down the the script_dir if the script_dir is not the known directory?

[Ryosuke Niwa]

(In reply to comment #4)
> Note that the advantage to running pylint in addition to pep8 is that we will catch a *lot* more issues. The downside is that pylint is a lot slower.

Do we really want to run this thing? pep8 is already way too noisy for me.

[Dirk Pranke]

(In reply to comment #6)
> (In reply to comment #4)
> > Note that the advantage to running pylint in addition to pep8 is that we will catch a *lot* more issues. The downside is that pylint is a lot slower.
> 
> Do we really want to run this thing? pep8 is already way too noisy for me.

The way I have it set now, I believe it'll only report real errors. (see, e.g., the pylint/E0602 warning from the style checker).

It's hard for me to respond to this comment otherwise as a general stance. If you can point to specific warnings we can discuss them or turn them off. I don't want to get noise either, but I don't think we normally do.

[Dirk Pranke]

(In reply to comment #5)
> (From update of attachment 172457 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=172457&action=review
> 
> > Tools/Scripts/webkitpy/style/checkers/python.py:100
> > +        self._script_dir = os.path.abspath(os.path.dirname(os.path.dirname(os.path.dirname(os.path.dirname(__file__)))))
> 
> Can we instead walk up the path until we find a known directory and then walk down the the script_dir if the script_dir is not the known directory?

Yeah, I'll change it to do something like that. We should really have a common hook for this (this is duplicating work in the layout_tests.port.base classes now and there's similar code in checkout that relies on the scm as well).

[Tony Chang]

Can you give some examples of the difference in coverage between the pep8 checker and the pylint checker (as currently configured).  Maybe you can run both over all of webkitpy and post the results (and maybe include running time to give a sense of how much slower pylint is)?

[Dirk Pranke]

Created attachment 173140 [details]
output from running pep8 across webkitpy (non-test) code

[Dirk Pranke]

Created attachment 173141 [details]
output from running pylint across webkitpy (non-test) code

[Dirk Pranke]

(In reply to comment #9)
> Can you give some examples of the difference in coverage between the pep8 checker and the pylint checker (as currently configured).  Maybe you can run both over all of webkitpy and post the results (and maybe include running time to give a sense of how much slower pylint is)?

Sure:

% find . -name '*.py' | egrep -v 'thirdparty|_unittest|_integrationtest' | wc -l
294

% time python thirdparty/autoinstalled/pep8.py --ignore=E501 $(find . -name '*.py' | egrep -v 'thirdparty|_unittest|_integrationtest') > pep8_errors.txt
./common/checkout/changelog.py:32:17: E261 at least two spaces before inline comment
./common/checkout/checkout_mock.py:72:1: E302 expected 2 blank lines, found 1
./common/checkout/diff_test_data.py:35:1: W291 trailing whitespace
./common/checkout/scm/scm.py:60:1: E303 too many blank lines (3)
./common/config/irc.py:29:7: E225 missing whitespace around operator
./common/config/ports.py:80:22: W601 .has_key() is deprecated, use 'in'
./common/host_mock.py:81:1: W391 blank line at end of file
./common/message_pool.py:292:33: W602 deprecated form of raising exception
./common/net/bugzilla/bugzilla_mock.py:172:34: E241 multiple spaces after ':'
./common/net/buildbot/buildbot.py:70:90: E702 multiple statements on one line (semicolon)
./common/net/buildbot/buildbot.py:146:5: E301 expected 1 blank line, found 0
./common/net/irc/ircbot.py:59:46: E202 whitespace before ')'
./style/checkers/cpp.py:267:7: E111 indentation is not a multiple of four
./style/checkers/cpp.py:597:62: E221 multiple spaces before operator
./style/checkers/cpp.py:3452:42: E262 inline comment should start with '# '
./tool/commands/download.py:186:18: E201 whitespace after '{'

real	0m7.112s
user	0m7.072s
sys	0m0.046s

% time lint-webkitpy -E $(find . -name '*.py' | egrep -v 'thirdparty|_unittest|_integrationtest') | grep -v "Instance of 'Popen'" 
common/checkout/scm/scm.py:65: [E1121, SCM.__init__] Too many positional arguments for function call
common/checkout/scm/scm.py:140: [E0213, SCM.find_checkout_root] Method should have "self" as first argument
common/checkout/scm/scm.py:237: [E0211, SCM.remote_merge_base] Method has no argument
common/checkout/scm/scm_mock.py:97: [E0602, MockSCM.commit_message_for_local_commit] Undefined variable 'CommitMessage'
common/checkout/scm/scm_mock.py:100: [E0602, MockSCM.commit_message_for_local_commit] Undefined variable 'CommitMessage'
common/checkout/scm/svn.py:53: [E1101, SVNRepository.has_authorization_for_realm] Instance of 'SVNRepository' has no 'run' member
common/checkout/scm/svn.py:60: [E1101, SVNRepository.has_authorization_for_realm] Instance of 'SVNRepository' has no 'run' member
common/checkout/scm/svn.py:62: [E1101, SVNRepository.has_authorization_for_realm] Instance of 'SVNRepository' has no 'run' member
common/checkout/scm/svn.py:79: [E0602, SVN.__init__] Undefined variable 'svn_info_args'
common/checkout/scm/svn.py:271: [E0602, SVN._bogus_dir_name] Undefined variable 'tempfile'
common/config/contributionareas.py:183: [E0102, ContributionAreas.names] method already defined line 180
common/config/ports.py:46: [E1101, DeprecatedPort.flag] Instance of 'DeprecatedPort' has no 'port_flag_name' member
common/config/ports.py:47: [E1101, DeprecatedPort.flag] Instance of 'DeprecatedPort' has no 'port_flag_name' member
common/net/buildbot/buildbot.py:138: [E1102, Builder.force_build] self._browser.open is not callable
common/net/buildbot/buildbot.py:139: [E1102, Builder.force_build] self._browser.select_form is not callable
common/net/buildbot/buildbot.py:143: [E1102, Builder.force_build] self._browser.submit is not callable
common/net/buildbot/buildbot_mock.py:56: [E0602, MockBuilder.force_build] Undefined variable 'log'
common/newstringio.py:35: [E0102, StringIO] class already defined line 32
common/system/executive_mock.py:171: [E0702, MockExecutive2.run_command] Raising NoneType while only classes, instances or string are allowed
common/system/filesystem_mock.py:111: [E1101, MockFileSystem.copyfile] Module 'errno' has no 'ISDIR' member
common/system/filesystem_mock.py:113: [E1101, MockFileSystem.copyfile] Module 'errno' has no 'ISDIR' member
layout_tests/controllers/manager.py:304: [E1101, Manager._websocket_tests] Instance of 'Manager' has no '_test_files' member
layout_tests/controllers/manager.py:304: [E1101, Manager._websocket_tests] Instance of 'Manager' has no '_is_websocket' member
layout_tests/port/apple.py:77: [E1101, ApplePort.__init__] Instance of 'ApplePort' has no 'VERSION_FALLBACK_ORDER' member
layout_tests/port/apple.py:85: [E1101, ApplePort._skipped_file_search_paths] Instance of 'ApplePort' has no 'VERSION_FALLBACK_ORDER' member
layout_tests/port/apple.py:95: [E1101, ApplePort._generate_all_test_configurations] Instance of 'ApplePort' has no 'VERSION_FALLBACK_ORDER' member
layout_tests/port/apple.py:98: [E1101, ApplePort._generate_all_test_configurations] Instance of 'ApplePort' has no 'ARCHITECTURES' member
layout_tests/port/base.py:574: [E1103, Port.reference_files] Instance of 'list' has no 'get' member (but some types could not be inferred)
layout_tests/port/chromium.py:126: [E1101, ChromiumPort.default_baseline_search_path] Instance of 'ChromiumPort' has no 'FALLBACK_PATHS' member
layout_tests/port/chromium_android.py:491: [E0213, ChromiumAndroidDriver._command_wrapper] Method should have "self" as first argument
layout_tests/port/chromium_port_testcase.py:106: [E1101, ChromiumPortTestCase.TestAndroidPort.__init__] Module 'layout_tests.port.chromium_win' has no 'ChromiumAndroidPort' member
layout_tests/port/chromium_port_testcase.py:133: [E1101, ChromiumPortTestCase.test_default_configuration] Instance of 'MockOptions' has no 'configuration' member
layout_tests/port/chromium_port_testcase.py:138: [E1101, ChromiumPortTestCase.test_default_configuration] Instance of 'MockOptions' has no 'configuration' member
layout_tests/port/port_testcase.py:89: [E1101, PortTestCase.make_port] Instance of 'PortTestCase' has no 'port_name' member
layout_tests/port/port_testcase.py:438: [E1101, PortTestCase.test_expectations_ordering] Instance of 'MockOptions' has no 'additional_expectations' member
layout_tests/port/port_testcase.py:505: [E0102, PortTestCase.test_expectations_files] method already defined line 117
layout_tests/servers/http_server_base.py:197: [E1101, HttpServerBase._check_that_all_ports_are_available] Module 'errno' has no 'WSAEACCES' member
style/checkers/cpp.py:1081: [E0012] Bad option value 'C6403'
style/checkers/cpp.py:1796: [E0012] Bad option value 'C6403'
tool/commands/abstractlocalservercommand.py:56: [E1102, AbstractLocalServerCommand.execute] self.server is not callable
tool/commands/download.py:160: [E1101, AbstractPatchProcessingCommand.execute] Instance of 'AbstractPatchProcessingCommand' has no '_prepare_to_process' member
tool/commands/download.py:161: [E1101, AbstractPatchProcessingCommand.execute] Instance of 'AbstractPatchProcessingCommand' has no '_fetch_list_of_patches_to_process' member
tool/commands/download.py:168: [E1101, AbstractPatchProcessingCommand.execute] Instance of 'AbstractPatchProcessingCommand' has no '_process_patch' member
tool/commands/earlywarningsystem.py:51: [E1101, AbstractEarlyWarningSystem.__init__] Instance of 'AbstractEarlyWarningSystem' has no 'port_name' member
tool/commands/earlywarningsystem.py:69: [E1101, AbstractEarlyWarningSystem._post_reject_message_on_bug] Instance of 'AbstractEarlyWarningSystem' has no 'port_name' member
tool/commands/expectations.py:42: [E1120, OptimizeExpectations.execute] No value passed for parameter 'expectations_string' in function call
tool/commands/gardenomatic.py:34: [E1003, GardenOMatic.__init__] Bad first argument 'AbstractRebaseliningCommand' given to super class
tool/commands/gardenomatic.py:34: [E0101, GardenOMatic.__init__] Explicit return in __init__
tool/commands/queues.py:355: [E0213, CommitQueue.handle_script_error] Method should have "self" as first argument
tool/multicommandtool.py:154: [E0202, HelpPrintingOptionParser.error] An attribute affected in style.optparser line 343 hide this method
tool/servers/gardeningserver.py:89: [E1120, GardeningHTTPRequestHandler.rollout] No value passed for parameter 'input_string' in function call
tool/servers/gardeningserver.py:142: [E1101, GardeningHTTPRequestHandler.localresult] Instance of 'GardeningHTTPRequestHandler' has no '_send_response' member

real	0m27.077s
user	0m25.924s
sys	0m1.155s

%

As you can see, pylint is ~4x slower than pep8, but both are pretty fast (I doubt we'll care about their speed in practice). It is also reporting a significantly more serious class of problem. I suspect most if not virtually all of those complaints are real bugs.

Anecdotally, also, I suspect 50% or more of the cases where I've had some problem with a patch I've landed recently (last month or two) have been some dumb typo in a case that was hard to catch through unit tests but static analysis like this would've caught.

[Ryosuke Niwa]

Thanks for posting the output. This does indeed looks useful. Maybe we want to make this optional feature at the beginning though to see if we get any false positives.

[Dirk Pranke]

(In reply to comment #13)
> Thanks for posting the output. This does indeed looks useful. Maybe we want to make this optional feature at the beginning though to see if we get any false positives.

I think that since I'm posting more python patches than most other people, if there are false positives, I'll probably trip over them pretty quickly. 

Also, there's no good way to make checks optional in check-webkit-style AFAIK.

[Ryosuke Niwa]

I guess we can always ignore false positives.

[Dirk Pranke]

(In reply to comment #15)
> I guess we can always ignore false positives.

I would prefer it if people followed the instructions we post when there are (cf comment #2), and have people file bugs.

We have mechanisms for suppressing the false positives; you can me doing some of them in the patch I've posted (where I'm suppressing warnings about Popen objects that are happening from what appear to be bugs in pylint).

But, yeah, you can ignore them, too.

[Tony Chang]

Seems like we'll want to run both checkers for now since they don't cover the same type of errors.  Or maybe we can teach pylint to catch the same errors as pep8.

[Dirk Pranke]

(In reply to comment #17)
> Seems like we'll want to run both checkers for now since they don't cover the same type of errors.  Or maybe we can teach pylint to catch the same errors as pep8.

I think pylint will report everything that pep8 does, but I haven't yet been able to go over things warning-by-warning. I expect to eventually cut over to pylint-only.

[Dirk Pranke]

Change landed as http://trac.webkit.org/changeset/134309 .