Magic-Mirror/gecko-dev
1
0
Fork 0
mirror of https://github.com/mozilla/gecko-dev.git synced 2025-01-31 19:10:36 +00:00
Code Issues Actions Packages Projects Releases Wiki Activity

Bug 1831312 - Use the mach cargo machinery to lint using clippy. r=glandium,linter-reviewers,ahal

Browse Source 
Differential Revision: https://phabricator.services.mozilla.com/D177156
This commit is contained in:
Arthur Carcano 2023-05-17 11:07:47 +00:00
parent 0326f3d918
commit 640dd3d14e
4 changed files with 78 additions and 348 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

2
docs/code-quality/lint/create.rst
View File

@ -245,7 +245,7 @@ To run a specific test:
.. code-block:: shell
./mach python-test --subsuite mozlint tools/lint/test/test_clippy.py
./mach python-test --subsuite mozlint tools/lint/test/test_black.py
More tests can be `found in-tree <https://searchfox.org/mozilla-central/source/tools/lint/test>`_.

302
tools/lint/clippy/__init__.py
View File

@ -2,136 +2,73 @@
# 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 bisect
import json
import os
import re
import signal
import subprocess
import sys
import six
from mozboot.util import get_tools_dir
from mozfile import which
from mozlint import result
from mozlint.pathutils import get_ancestors_by_name
from mozlint.pathutils import expand_exclusions
from mozprocess import ProcessHandler
from packaging.version import Version
CLIPPY_WRONG_VERSION = """
Clippy is not installed or an older version was detected. Please make sure
clippy is installed and up to date. The minimum required version is {version}.
To install:
$ rustup component add clippy
To update:
$ rustup update
Also ensure 'cargo' is on your $PATH.
""".strip()
CARGO_NOT_FOUND = """
Could not find cargo! Install cargo.
And make sure that it is in the PATH
""".strip()
def in_sorted_list(l, x):
i = bisect.bisect_left(l, x)
return l[i] == x
def parse_issues(log, config, issues, base_path, onlyIn):
results = []
if onlyIn:
onlyIn = os.path.normcase(os.path.normpath(onlyIn))
for issue in issues:
def handle_clippy_msg(config, line, log, base_path, files):
try:
detail = json.loads(six.ensure_text(line))
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))
return
# We are in a clippy warning
if len(detail["spans"]) == 0:
# For some reason, at the end of the summary, we can
# get the following line
# {'rendered': 'warning: 5 warnings emitted\n\n', 'children':
# [], 'code': None, 'level': 'warning', 'message':
# '5 warnings emitted', 'spans': []}
# if this is the case, skip it
log.debug(
"Skipping the summary line {} for file {}".format(detail, p)
)
return
try:
detail = json.loads(six.ensure_text(issue))
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
if len(detail["spans"]) == 0:
# For some reason, at the end of the summary, we can
# get the following line
# {'rendered': 'warning: 5 warnings emitted\n\n', 'children':
# [], 'code': None, 'level': 'warning', 'message':
# '5 warnings emitted', 'spans': []}
# if this is the case, skip it
log.debug(
"Skipping the summary line {} for file {}".format(detail, p)
)
continue
l = detail["spans"][0]
if not in_sorted_list(files, p):
return
p = os.path.join(base_path, l["file_name"])
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"],
}
return result.from_config(config, **res)
l = detail["spans"][0]
p = os.path.join(base_path, l["file_name"])
if onlyIn and onlyIn not in os.path.normcase(os.path.normpath(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
rust_path = os.path.join(get_tools_dir(), "rustc", "bin")
return which("cargo", path=os.pathsep.join([rust_path, os.environ["PATH"]]))
def get_clippy_version(log, cargo):
"""
Check if we are running the deprecated clippy
"""
output = run_cargo_command(
log, [cargo, "clippy", "--version"], universal_newlines=True
)
version = re.findall(r"(\d+-\d+-\d+)", output[0])
if not version:
return False
version = Version(version[0].replace("-", "."))
log.debug("Found version: {}".format(version))
return version
except json.decoder.JSONDecodeError:
log.debug("Could not parse the output:")
log.debug("clippy output: {}".format(line))
return
class clippyProcess(ProcessHandler):
@ -145,117 +82,32 @@ class clippyProcess(ProcessHandler):
signal.signal(signal.SIGINT, orig)
def run_cargo_command(log, cmd, **kwargs):
log.debug("Command: {}".format(cmd))
env = os.environ.copy()
# Cargo doesn't find cargo-clippy on its own if it's not in `$PATH` when
# `$CARGO_HOME` is not set and cargo is not in `~/.cargo`.
env["PATH"] = os.pathsep.join([os.path.dirname(cmd[0]), os.environ["PATH"]])
proc = clippyProcess(cmd, env=env, **kwargs)
proc.run()
try:
proc.wait()
except KeyboardInterrupt:
proc.kill()
return proc.output
def lint(paths, config, fix=None, **lintargs):
files = list(expand_exclusions(paths, config, lintargs["root"]))
files.sort()
log = lintargs["log"]
cargo = get_cargo_binary(log)
if not cargo:
print(CARGO_NOT_FOUND)
if "MOZ_AUTOMATION" in os.environ:
return 1
return []
min_version_str = config.get("min_clippy_version")
min_version = Version(min_version_str)
actual_version = get_clippy_version(log, cargo)
log.debug(
"Found version: {}. Minimum expected version: {}".format(
actual_version, min_version
)
results = []
mach_path = lintargs["root"] + "/mach"
march_cargo_process = subprocess.Popen(
[
sys.executable,
mach_path,
"--log-no-times",
"cargo",
"clippy",
"--",
"--message-format=json",
],
stdout=subprocess.PIPE,
text=True,
)
if not actual_version or actual_version < min_version:
print(CLIPPY_WRONG_VERSION.format(version=min_version_str))
return 1
cmd_args_clean = [cargo]
cmd_args_clean.append("clean")
cmd_args_common = ["--manifest-path"]
cmd_args_clippy = [cargo]
cmd_args_clippy += [
"clippy",
"--message-format=json",
]
for l in march_cargo_process.stdout:
r = handle_clippy_msg(config, l, log, lintargs["root"], files)
if r is not None:
results.append(r)
march_cargo_process.wait()
if fix:
cmd_args_clippy += ["--fix"]
log.error("Rust linting in mach does not support --fix")
lock_files_to_delete = []
for p in paths:
lock_file = os.path.join(p, "Cargo.lock")
if not os.path.exists(lock_file):
lock_files_to_delete.append(lock_file)
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
elif 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_cargo_command(log, clean_command)
# Create the actual clippy command
base_command = cmd_args_clippy + cmd_args_common + [p]
output = run_cargo_command(log, base_command)
# Remove build artifacts created by clippy
run_cargo_command(log, clean_command)
# Source paths in clippy spans, when they are relative, are relative to the
# workspace if there is one.
for cargo_toml in cargo_files:
with open(cargo_toml) as fh:
if "[workspace]" in fh.read():
p = cargo_toml
break
results += parse_issues(log, config, output, os.path.dirname(p), onlyIn)
# Remove Cargo.lock files created by clippy
for lock_file in lock_files_to_delete:
if os.path.exists(lock_file):
os.remove(lock_file)
return sorted(results, key=lambda issue: issue.path)
return results

1
tools/lint/test/python.ini
View File

@ -5,7 +5,6 @@ subsuite = mozlint
[test_black.py]
requirements = tools/lint/python/black_requirements.txt
[test_clang_format.py]
[test_clippy.py]
[test_codespell.py]
[test_eslint.py]
skip-if = os == "win" # busts the tree for subsequent tasks on the same worker (bug 1708591)

121
tools/lint/test/test_clippy.py
View File

@ -1,121 +0,0 @@
import os
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 >= 9
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 "this looks like you are trying to swap `a` and `b`" in results[1].message
assert results[1].level == "error"
assert results[1].relpath == "test1/bad.rs"
assert results[1].rule == "clippy::almost_swapped"
assert "or maybe you should use `std::mem::replace`" in results[1].hint
assert "value assigned to `b` is never read" in results[2].message
assert results[2].level == "warning"
assert results[2].relpath == "test1/bad.rs"
if "variable does not need to be mutable" in results[5].message:
n = 5
else:
n = 6
assert "variable does not need to be mutable" in results[n].message
assert results[n].relpath == "test1/bad2.rs"
assert results[n].rule == "unused_mut"
assert "unused variable: `vec`" in results[n + 1].message
assert results[n + 1].level == "warning"
assert results[n + 1].relpath == "test1/bad2.rs"
assert results[n + 1].rule == "unused_variables"
assert "this range is empty so" in results[8].message
assert results[8].level == "error"
assert results[8].relpath == "test1/bad2.rs"
assert results[8].rule == "clippy::reversed_empty_ranges"
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) > 12
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 >= 9
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 == "error"
assert results[8].lineno == 10
assert results[8].column == 14
assert results[8].rule == "clippy::reversed_empty_ranges"
assert results[8].relpath == "test1/bad2.rs"
assert "tools/lint/test/files/clippy/test1/bad2.rs" in results[8].path
assert results[10].level == "warning"
assert results[10].lineno == 9
assert results[10].column >= 9
assert results[10].rule == "unused_assignments"
assert results[10].relpath == "test2/src/bad_1.rs"
assert "tools/lint/test/files/clippy/test2/src/bad_1.rs" in results[10].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) > 2
assert results[0].level == "warning"
assert results[0].lineno == 9
assert results[0].column >= 9
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
def test_cleanup(lint, paths, root):
# If Cargo.lock does not exist before clippy run, delete it
lint(paths("test1/"))
assert not os.path.exists(os.path.join(root, "test1/target/"))
assert not os.path.exists(os.path.join(root, "test1/Cargo.lock"))
# If Cargo.lock exists before clippy run, keep it after cleanup
lint(paths("test2/"))
assert not os.path.exists(os.path.join(root, "test2/target/"))
assert os.path.exists(os.path.join(root, "test2/Cargo.lock"))
if __name__ == "__main__":
mozunit.main()