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

@@ -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",
 ]
 

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 <file paths>
+
+.. 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 <https://searchfox.org/mozilla-central/source/tools/lint/clippy.yml>`_ file.
+
+
+Sources
+-------
+
+* `Configuration (YAML) <https://searchfox.org/mozilla-central/source/tools/lint/clippy.yml>`_
+* `Source <https://searchfox.org/mozilla-central/source/tools/lint/clippy/__init__.py>`_

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

tools/lint/clippy/__init__.py

@@ -0,0 +1,198 @@
+# 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("clippy output: {}".format(issue))
+            continue
+
+    return results
+
+
+def get_cargo_binary():
+    """
+    Returns the path of the first rustfmt binary available
+    if not found returns None
+    """
+    cargo_home = os.environ.get("CARGO_HOME")
+    if cargo_home:
+        cargo_bin = os.path.join(cargo_home, "bin", "cargo")
+        if os.path.exists(cargo_bin):
+            return cargo_bin
+        else:
+            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()
+
+    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]
+#        print(base_command)
+        output = run_process(log, config, base_command)
+#        print(output)
+        results += parse_issues(log, config, output, p, onlyIn)
+
+    return sorted(results, key=lambda issue: issue.path)

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 <a@example.com>", "Bob <b@example.com>"]
+
+[[bin]]
+name ="good"
+path = "good.rs"
+
+[[bin]]
+name ="bad"
+path = "bad.rs"
+
+[[bin]]
+name ="bad2"
+path = "bad2.rs"
+

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;
+
+
+}

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;
+    }
+
+    }

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!");
+}

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 <a@example.com>", "Bob <b@example.com>"]
+
+[[bin]]
+name = "fake_lib1"
+path = "src/bad_1.rs"

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;
+
+}

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;
+    }
+
+    }

tools/lint/test/python.ini

@@ -16,4 +16,5 @@ skip-if = os == "win"
 [test_rst.py]
 [test_codespell.py]
 skip-if = os == "win" || os == "mac"  # codespell installed on Linux
-[test_yaml.py]
+[test_yaml.py]
+[test_clippy.py]

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()