webkitpy: integrate pylint into check-webkit-style, part I
Created attachment 172457 [details] Patch
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.
The errors in that file are intentional; the output shows that this change works :).
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.
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?
(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.
(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.
(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).
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)?
Created attachment 173140 [details] output from running pep8 across webkitpy (non-test) code
Created attachment 173141 [details] output from running pylint across webkitpy (non-test) code
(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.
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.
(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.
I guess we can always ignore false positives.
(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.
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.
(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.
Change landed as http://trac.webkit.org/changeset/134309 .