From ef6f6d2886242b2fbc841e875e1ebf6c8d840fc8 Mon Sep 17 00:00:00 2001 From: Sylvestre Ledru Date: Wed, 19 Feb 2020 08:50:35 +0000 Subject: [PATCH] Bug 1361341 - Add rust clippy to mozlint r=ahal Differential Revision: https://phabricator.services.mozilla.com/D58250 --HG-- extra : moz-landing-system : lando --- Cargo.toml | 1 + docs/code-quality/lint/linters/clippy.rst | 31 +++ taskcluster/ci/source-test/python.yml | 4 + tools/lint/clippy.yml | 15 ++ tools/lint/clippy/__init__.py | 199 ++++++++++++++++++ tools/lint/test/files/clippy/test1/Cargo.toml | 17 ++ tools/lint/test/files/clippy/test1/bad.rs | 14 ++ tools/lint/test/files/clippy/test1/bad2.rs | 17 ++ tools/lint/test/files/clippy/test1/good.rs | 6 + tools/lint/test/files/clippy/test2/Cargo.toml | 8 + .../lint/test/files/clippy/test2/src/bad_1.rs | 15 ++ .../lint/test/files/clippy/test2/src/bad_2.rs | 17 ++ tools/lint/test/python.ini | 4 +- tools/lint/test/test_clippy.py | 89 ++++++++ 14 files changed, 436 insertions(+), 1 deletion(-) create mode 100644 docs/code-quality/lint/linters/clippy.rst create mode 100644 tools/lint/clippy.yml create mode 100644 tools/lint/clippy/__init__.py create mode 100644 tools/lint/test/files/clippy/test1/Cargo.toml create mode 100644 tools/lint/test/files/clippy/test1/bad.rs create mode 100644 tools/lint/test/files/clippy/test1/bad2.rs create mode 100644 tools/lint/test/files/clippy/test1/good.rs create mode 100644 tools/lint/test/files/clippy/test2/Cargo.toml create mode 100644 tools/lint/test/files/clippy/test2/src/bad_1.rs create mode 100644 tools/lint/test/files/clippy/test2/src/bad_2.rs create mode 100644 tools/lint/test/test_clippy.py diff --git a/Cargo.toml b/Cargo.toml index e436c4a2bef9..91e8d5f6102e 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -39,6 +39,7 @@ exclude = [ "media/mp4parse-rust/mp4parse_capi", "media/mp4parse-rust/mp4parse_fallible", "xpcom/rust/gkrust_utils", + "tools/lint/test/files/clippy", "tools/fuzzing/rust", ] diff --git a/docs/code-quality/lint/linters/clippy.rst b/docs/code-quality/lint/linters/clippy.rst new file mode 100644 index 000000000000..7c9ac6b19bc5 --- /dev/null +++ b/docs/code-quality/lint/linters/clippy.rst @@ -0,0 +1,31 @@ +clippy +====== + +`clippy`_ is the tool for Rust static analysis. + +Run Locally +----------- + +The mozlint integration of clippy can be run using mach: + +.. parsed-literal:: + + $ mach lint --linter clippy + +.. note:: + + clippy expects a path or a .rs file. It doesn't accept Cargo.toml + as it would break the mozlint workflow. + +Configuration +------------- + +To enable clippy on new directory, add the path to the include +section in the `clippy.yml `_ file. + + +Sources +------- + +* `Configuration (YAML) `_ +* `Source `_ diff --git a/taskcluster/ci/source-test/python.yml b/taskcluster/ci/source-test/python.yml index d8f85a0a3e16..070a35ca40f5 100644 --- a/taskcluster/ci/source-test/python.yml +++ b/taskcluster/ci/source-test/python.yml @@ -15,6 +15,10 @@ job-defaults: linux64.*: docker-image: {in-tree: "lint"} max-run-time: 3600 + env: + RUSTFMT: /build/rust/bin/rustfmt + RUSTUP_HOME: /build/rust + CARGO_HOME: /build/rust default: max-run-time: 3600 treeherder: diff --git a/tools/lint/clippy.yml b/tools/lint/clippy.yml new file mode 100644 index 000000000000..ac2ecb150f84 --- /dev/null +++ b/tools/lint/clippy.yml @@ -0,0 +1,15 @@ +--- +clippy: + description: Lint rust + include: + # Clippy expects a Cargo configuration + - js/src/frontend/binast/ + - js/src/wasm/cranelift/ + - dom/media/gtest/ + - dom/webauthn/libudev-sys/ + extensions: + - rs + support-files: + - 'tools/lint/clippy/**' + type: external + payload: clippy:lint diff --git a/tools/lint/clippy/__init__.py b/tools/lint/clippy/__init__.py new file mode 100644 index 000000000000..e0622ed254be --- /dev/null +++ b/tools/lint/clippy/__init__.py @@ -0,0 +1,199 @@ +# This Source Code Form is subject to the terms of the Mozilla Public +# License, v. 2.0. If a copy of the MPL was not distributed with this +# file, You can obtain one at http://mozilla.org/MPL/2.0/. + +import json +import os +import signal +import subprocess + +from mozfile import which +from mozlint import result +from mozlint.pathutils import get_ancestors_by_name +from mozprocess import ProcessHandler + + +CLIPPY_NOT_FOUND = """ +Could not find clippy! Install clippy and try again. + + $ rustup component add clippy + +And make sure that it is in the PATH +""".strip() + + +CARGO_NOT_FOUND = """ +Could not find cargo! Install cargo. + +And make sure that it is in the PATH +""".strip() + + +def parse_issues(log, config, issues, path, onlyIn): + results = [] + for issue in issues: + + try: + detail = json.loads(issue.decode("utf-8")) + if "message" in detail: + p = detail['target']['src_path'] + detail = detail["message"] + if "level" in detail: + if ((detail["level"] == "error" or detail["level"] == "failure-note") + and not detail["code"]): + log.debug("Error outside of clippy." + "This means that the build failed. Therefore, skipping this") + log.debug("File = {} / Detail = {}".format(p, detail)) + continue + # We are in a clippy warning + l = detail["spans"][0] + if onlyIn and onlyIn not in p: + # Case when we have a .rs in the include list in the yaml file + log.debug("{} is not part of the list of files '{}'".format(p, onlyIn)) + continue + res = { + "path": p, + "level": detail["level"], + "lineno": l["line_start"], + "column": l["column_start"], + "message": detail["message"], + "hint": detail["rendered"], + "rule": detail["code"]["code"], + "lineoffset": l["line_end"] - l["line_start"], + } + results.append(result.from_config(config, **res)) + + except json.decoder.JSONDecodeError: + log.debug("Could not parse the output:") + log.debug("clippy output: {}".format(issue)) + continue + + return results + + +def get_cargo_binary(log): + """ + Returns the path of the first rustfmt binary available + if not found returns None + """ + cargo_home = os.environ.get("CARGO_HOME") + if cargo_home: + log.debug("Found CARGO_HOME in {}".format(cargo_home)) + cargo_bin = os.path.join(cargo_home, "bin", "cargo") + if os.path.exists(cargo_bin): + return cargo_bin + log.debug("Did not find {} in CARGO_HOME".format(cargo_bin)) + return None + return which("cargo") + + +def is_clippy_installed(binary): + """ + Check if we are running the deprecated rustfmt + """ + try: + output = subprocess.check_output( + [binary, "clippy", "--help"], + stderr=subprocess.STDOUT, + universal_newlines=True, + ) + except subprocess.CalledProcessError as e: + output = e.output + + if "Checks a package" in output: + return True + + return False + + +class clippyProcess(ProcessHandler): + def __init__(self, config, *args, **kwargs): + self.config = config + kwargs["stream"] = False + ProcessHandler.__init__(self, *args, **kwargs) + + def run(self, *args, **kwargs): + orig = signal.signal(signal.SIGINT, signal.SIG_IGN) + ProcessHandler.run(self, *args, **kwargs) + signal.signal(signal.SIGINT, orig) + + +def run_process(log, config, cmd): + log.debug("Command: {}".format(cmd)) + proc = clippyProcess(config, cmd) + proc.run() + try: + proc.wait() + except KeyboardInterrupt: + proc.kill() + + return proc.output + + +def lint(paths, config, fix=None, **lintargs): + log = lintargs["log"] + cargo = get_cargo_binary(log) + + if not cargo: + print(CARGO_NOT_FOUND) + if 'MOZ_AUTOMATION' in os.environ: + return 1 + return [] + + if not is_clippy_installed(cargo): + print(CLIPPY_NOT_FOUND) + if 'MOZ_AUTOMATION' in os.environ: + return 1 + return [] + + cmd_args_clean = [cargo] + cmd_args_clean.append("clean") + + cmd_args_common = ["--manifest-path"] + cmd_args_clippy = [ + cargo, + 'clippy', + '--message-format=json', + ] + + results = [] + + for p in paths: + # Quick sanity check of the paths + if p.endswith("Cargo.toml"): + print("Error: expects a directory or a rs file") + print("Found {}".format(p)) + return 1 + + for p in paths: + onlyIn = [] + path_conf = p + log.debug("Path = {}".format(p)) + if os.path.isfile(p): + # We are dealing with a file. We remove the filename from the path + # to find the closest Cargo file + # We also store the name of the file to be able to filter out other + # files built by the cargo + p = os.path.dirname(p) + onlyIn = path_conf + + if os.path.isdir(p): + # Sometimes, clippy reports issues from other crates + # Make sure that we don't display that either + onlyIn = p + + cargo_files = get_ancestors_by_name('Cargo.toml', p, lintargs['root']) + p = cargo_files[0] + + log.debug("Path translated to = {}".format(p)) + # Needs clean because of https://github.com/rust-lang/rust-clippy/issues/2604 + clean_command = cmd_args_clean + cmd_args_common + [p] + run_process(log, config, clean_command) + + # Create the actual clippy command + base_command = cmd_args_clippy + cmd_args_common + [p] + output = run_process(log, config, base_command) + + results += parse_issues(log, config, output, p, onlyIn) + + return sorted(results, key=lambda issue: issue.path) diff --git a/tools/lint/test/files/clippy/test1/Cargo.toml b/tools/lint/test/files/clippy/test1/Cargo.toml new file mode 100644 index 000000000000..92d5072eca7c --- /dev/null +++ b/tools/lint/test/files/clippy/test1/Cargo.toml @@ -0,0 +1,17 @@ +[package] +name = "hello_world" # the name of the package +version = "0.1.0" # the current version, obeying semver +authors = ["Alice ", "Bob "] + +[[bin]] +name ="good" +path = "good.rs" + +[[bin]] +name ="bad" +path = "bad.rs" + +[[bin]] +name ="bad2" +path = "bad2.rs" + diff --git a/tools/lint/test/files/clippy/test1/bad.rs b/tools/lint/test/files/clippy/test1/bad.rs new file mode 100644 index 000000000000..c403fba60312 --- /dev/null +++ b/tools/lint/test/files/clippy/test1/bad.rs @@ -0,0 +1,14 @@ +fn main() { + // Statements here are executed when the compiled binary is called + + // Print text to the console + println!("Hello World!"); + // Clippy detects this as a swap and considers this as an error + let mut a=1; + let mut b=1; + + a = b; + b = a; + + +} diff --git a/tools/lint/test/files/clippy/test1/bad2.rs b/tools/lint/test/files/clippy/test1/bad2.rs new file mode 100644 index 000000000000..a4236a2de7d8 --- /dev/null +++ b/tools/lint/test/files/clippy/test1/bad2.rs @@ -0,0 +1,17 @@ +fn main() { + // Statements here are executed when the compiled binary is called + + // Print text to the console + println!("Hello World!"); + let mut a; + let mut b=1; + let mut vec = Vec::new(); + vec.push(1); + vec.push(2); + + + for x in 5..10 - 5 { + a = x; + } + + } diff --git a/tools/lint/test/files/clippy/test1/good.rs b/tools/lint/test/files/clippy/test1/good.rs new file mode 100644 index 000000000000..9bcaee67b733 --- /dev/null +++ b/tools/lint/test/files/clippy/test1/good.rs @@ -0,0 +1,6 @@ +fn main() { + // Statements here are executed when the compiled binary is called + + // Print text to the console + println!("Hello World!"); +} diff --git a/tools/lint/test/files/clippy/test2/Cargo.toml b/tools/lint/test/files/clippy/test2/Cargo.toml new file mode 100644 index 000000000000..b0ac9920885c --- /dev/null +++ b/tools/lint/test/files/clippy/test2/Cargo.toml @@ -0,0 +1,8 @@ +[package] +name = "hello_world_2" # the name of the package +version = "0.2.0" # the current version, obeying semver +authors = ["Alice ", "Bob "] + +[[bin]] +name = "fake_lib1" +path = "src/bad_1.rs" diff --git a/tools/lint/test/files/clippy/test2/src/bad_1.rs b/tools/lint/test/files/clippy/test2/src/bad_1.rs new file mode 100644 index 000000000000..2fe06302020e --- /dev/null +++ b/tools/lint/test/files/clippy/test2/src/bad_1.rs @@ -0,0 +1,15 @@ +mod bad_2; + +fn main() { + // Statements here are executed when the compiled binary is called + + // Print text to the console + println!("Hello World!"); + // Clippy detects this as a swap and considers this as an error + let mut a=1; + let mut b=1; + + a = b; + b = a; + +} diff --git a/tools/lint/test/files/clippy/test2/src/bad_2.rs b/tools/lint/test/files/clippy/test2/src/bad_2.rs new file mode 100644 index 000000000000..f77de330b4c1 --- /dev/null +++ b/tools/lint/test/files/clippy/test2/src/bad_2.rs @@ -0,0 +1,17 @@ +fn foo() { + // Statements here are executed when the compiled binary is called + + // Print text to the console + println!("Hello World!"); + let mut a; + let mut b=1; + let mut vec = Vec::new(); + vec.push(1); + vec.push(2); + + + for x in 5..10 - 5 { + a = x; + } + + } diff --git a/tools/lint/test/python.ini b/tools/lint/test/python.ini index 71003cab550d..3c7f61119582 100644 --- a/tools/lint/test/python.ini +++ b/tools/lint/test/python.ini @@ -16,4 +16,6 @@ skip-if = os == "win" [test_rst.py] [test_codespell.py] skip-if = os == "win" || os == "mac" # codespell installed on Linux -[test_yaml.py] \ No newline at end of file +[test_yaml.py] +[test_clippy.py] +skip-if = os == "win" || os == "mac" # only installed on Linux diff --git a/tools/lint/test/test_clippy.py b/tools/lint/test/test_clippy.py new file mode 100644 index 000000000000..25cf4f8d6636 --- /dev/null +++ b/tools/lint/test/test_clippy.py @@ -0,0 +1,89 @@ +import mozunit + + +LINTER = 'clippy' + + +def test_basic(lint, config, paths): + results = lint(paths("test1/")) + print(results) + assert len(results) > 7 + + assert "is never read" in results[0].message or 'but never used' in results[0].message + assert results[0].level == "warning" + assert results[0].lineno == 7 + assert results[0].column == 13 + assert results[0].rule == "unused_assignments" + assert results[0].relpath == "test1/bad.rs" + assert "tools/lint/test/files/clippy/test1/bad.rs" in results[0].path + + assert "value assigned to `b` is never read" in results[1].message + assert results[1].level == "warning" + assert results[1].relpath == "test1/bad.rs" + + assert "this looks like you are trying to swap `a` and `b`" in results[2].message + assert results[2].level == "error" + assert results[2].relpath == "test1/bad.rs" + assert results[2].rule == "clippy::almost_swapped" + assert "or maybe you should use `std::mem::replace`" in results[2].hint + + assert "variable does not need to be mutable" in results[6].message + assert results[6].relpath == "test1/bad2.rs" + assert results[6].rule == "unused_mut" + + assert "this range is empty so this for loop will never run" in results[7].message + assert results[7].level == "error" + assert results[7].relpath == "test1/bad2.rs" + assert results[7].rule == "clippy::reverse_range_loop" + + +def test_error(lint, config, paths): + results = lint(paths("test1/Cargo.toml")) + # Should fail. We don't accept Cargo.toml as input + assert results == 1 + + +def test_file_and_path_provided(lint, config, paths): + results = lint(paths("./test2/src/bad_1.rs", "test1/")) + # even if clippy analyzed it + # we should not have anything from bad_2.rs + # as mozlint is filtering out the file + print(results) + assert len(results) > 16 + assert "value assigned to `a` is never read" in results[0].message + assert results[0].level == "warning" + assert results[0].lineno == 7 + assert results[0].column == 13 + assert results[0].rule == "unused_assignments" + assert results[0].relpath == "test1/bad.rs" + assert "tools/lint/test/files/clippy/test1/bad.rs" in results[0].path + assert "value assigned to `a` is never read" in results[0].message + assert results[8].level == "warning" + assert results[8].lineno == 9 + assert results[8].column == 13 + assert results[8].rule == "unused_assignments" + assert results[8].relpath == "test2/src/bad_1.rs" + assert "tools/lint/test/files/clippy/test2/src/bad_1.rs" in results[8].path + for r in results: + assert "bad_2.rs" not in r.relpath + + +def test_file_provided(lint, config, paths): + results = lint(paths("./test2/src/bad_1.rs")) + # even if clippy analyzed it + # we should not have anything from bad_2.rs + # as mozlint is filtering out the file + print(results) + assert len(results) > 8 + assert results[0].level == "warning" + assert results[0].lineno == 9 + assert results[0].column == 13 + assert results[0].rule == "unused_assignments" + assert results[0].relpath == "test2/src/bad_1.rs" + assert "tools/lint/test/files/clippy/test2/src/bad_1.rs" in results[0].path + for r in results: + assert "bad_2.rs" not in r.relpath + + +if __name__ == '__main__': + mozunit.main()