377 lines
14 KiB
ReStructuredText
377 lines
14 KiB
ReStructuredText
Adding a New Linter to the Tree
|
|
===============================
|
|
|
|
Linter Requirements
|
|
-------------------
|
|
|
|
For a linter to be integrated into the mozilla-central tree, it needs to have:
|
|
|
|
* Any required dependencies should be installed as part of ``./mach bootstrap``
|
|
* A ``./mach lint`` interface
|
|
* Running ``./mach lint`` command must pass (note, linters can be disabled for individual directories)
|
|
* Taskcluster/Treeherder integration
|
|
* In tree documentation (under ``docs/code-quality/lint``) to give a basic summary, links and any other useful information
|
|
* Unit tests (under ``tools/lint/test``) to make sure that the linter works as expected and we don't regress.
|
|
|
|
The review group in Phabricator is ``#linter-reviewers``.
|
|
|
|
Linter Basics
|
|
-------------
|
|
|
|
A linter is a yaml file with a ``.yml`` extension. Depending on how the type of linter, there may
|
|
be python code alongside the definition, pointed to by the 'payload' attribute.
|
|
|
|
Here's a trivial example:
|
|
|
|
no-eval.yml
|
|
|
|
.. code-block:: yaml
|
|
|
|
EvalLinter:
|
|
description: Ensures the string eval doesn't show up.
|
|
extensions: ['js']
|
|
type: string
|
|
payload: eval
|
|
|
|
Now ``no-eval.yml`` gets passed into :func:`LintRoller.read`.
|
|
|
|
|
|
Linter Types
|
|
------------
|
|
|
|
There are four types of linters, though more may be added in the future.
|
|
|
|
1. string - fails if substring is found
|
|
2. regex - fails if regex matches
|
|
3. external - fails if a python function returns a non-empty result list
|
|
4. structured_log - fails if a mozlog logger emits any lint_error or lint_warning log messages
|
|
|
|
As seen from the example above, string and regex linters are very easy to create, but they
|
|
should be avoided if possible. It is much better to use a context aware linter for the language you
|
|
are trying to lint. For example, use eslint to lint JavaScript files, use ruff to lint Python
|
|
files, etc.
|
|
|
|
Which brings us to the third and most interesting type of linter,
|
|
external. External linters call an arbitrary python function which is
|
|
responsible for not only running the linter, but ensuring the results
|
|
are structured properly. For example, an external type could shell out
|
|
to a 3rd party linter, collect the output and format it into a list of
|
|
:class:`Issue` objects. The signature for this python
|
|
function is ``lint(files, config, **kwargs)``, where ``files`` is a list of
|
|
files to lint and ``config`` is the linter definition defined in the ``.yml``
|
|
file.
|
|
|
|
Structured log linters are much like external linters, but suitable
|
|
for cases where the linter code is using mozlog and emits
|
|
``lint_error`` or ``lint_warning`` logging messages when the lint
|
|
fails. This is recommended for writing novel gecko-specific lints. In
|
|
this case the signature for lint functions is ``lint(files, config, logger,
|
|
**kwargs)``.
|
|
|
|
|
|
Linter Definition
|
|
-----------------
|
|
|
|
Each ``.yml`` file must have at least one linter defined in it. Here are the supported keys:
|
|
|
|
* description - A brief description of the linter's purpose (required)
|
|
* type - One of 'string', 'regex' or 'external' (required)
|
|
* payload - The actual linting logic, depends on the type (required)
|
|
* include - A list of file paths that will be considered (optional)
|
|
* exclude - A list of file paths or glob patterns that must not be matched (optional)
|
|
* extensions - A list of file extensions to be considered (optional)
|
|
* exclude_extensions - A list of file extensions to be excluded (optional)
|
|
* setup - A function that sets up external dependencies (optional)
|
|
* support-files - A list of glob patterns matching configuration files (optional)
|
|
* find-dotfiles - If set to ``true``, run on dot files (.*) (optional)
|
|
* ignore-case - If set to ``true`` and ``type`` is regex, ignore the case (optional)
|
|
|
|
Note that a linter may not have both ``extensions`` and ``exclude_extensions`` specified at the
|
|
same time.
|
|
|
|
In addition to the above, some ``.yml`` files correspond to a single lint rule. For these, the
|
|
following additional keys may be specified:
|
|
|
|
* message - A string to print on infraction (optional)
|
|
* hint - A string with a clue on how to fix the infraction (optional)
|
|
* rule - An id string for the lint rule (optional)
|
|
* level - The severity of the infraction, either 'error' or 'warning' (optional)
|
|
|
|
For structured_log lints the following additional keys apply:
|
|
|
|
* logger - A StructuredLog object to use for logging. If not supplied
|
|
one will be created (optional)
|
|
|
|
|
|
Example
|
|
-------
|
|
|
|
Here is an example of an external linter that shells out to the Python ruff linter,
|
|
let's call the file ``ruff_lint.py`` (`in-tree version <https://searchfox.org/mozilla-central/source/tools/lint/python/ruff.py>`__):
|
|
|
|
.. code-block:: python
|
|
|
|
import json
|
|
import os
|
|
import subprocess
|
|
from collections import defaultdict
|
|
from shutil import which
|
|
|
|
from mozlint import result
|
|
|
|
|
|
RUFF_NOT_FOUND = """
|
|
Could not find ruff! Install ruff and try again.
|
|
""".strip()
|
|
|
|
|
|
def lint(paths, config, **lintargs):
|
|
binary = which('ruff')
|
|
if not binary:
|
|
print(RUFF_NOT_FOUND)
|
|
return 1
|
|
|
|
|
|
cmd = ["ruff", "check", "--force-exclude", "--format=json"] + paths
|
|
output = subprocess.run(cmd, stdout=subprocess.PIPE, env=os.environ).output
|
|
|
|
# all passed
|
|
if not output:
|
|
return []
|
|
|
|
try:
|
|
issues = json.loads(output)
|
|
except json.JSONDecodeError:
|
|
log.error(f"Could not parse output: {output}")
|
|
|
|
results = []
|
|
for issue in issues:
|
|
# convert ruff's format to mozlint's format
|
|
res = {
|
|
"path": issue["filename"],
|
|
"lineno": issue["location"]["row"],
|
|
"column": issue["location"]["column"],
|
|
"lineoffset": issue["end_location"]["row"] - issue["location"]["row"],
|
|
"message": issue["message"],
|
|
"rule": issue["code"],
|
|
"level": "error",
|
|
}
|
|
|
|
if issue["fix"]:
|
|
res["hint"] = issue["fix"]["message"]
|
|
|
|
results.append(result.from_config(config, **res))
|
|
|
|
return {"results": results, "fixed": fixed}
|
|
|
|
Now here is the linter definition that would call it:
|
|
|
|
.. code-block:: yaml
|
|
|
|
ruff:
|
|
description: Python Linter
|
|
include: ["."]
|
|
extensions: ["py"]
|
|
support-files:
|
|
- "**/.ruff.toml"
|
|
- "**/ruff.toml"
|
|
- "**/pyproject.toml"
|
|
type: external
|
|
payload: py.ruff:lint
|
|
|
|
Notice the payload has two parts, delimited by ':'. The first is the module
|
|
path, which ``mozlint`` will attempt to import. The second is the object path
|
|
within that module (e.g, the name of a function to call). It is up to consumers
|
|
of ``mozlint`` to ensure the module is in ``sys.path``. Structured log linters
|
|
use the same import mechanism.
|
|
|
|
The ``support-files`` key is used to list configuration files or files related
|
|
to the running of the linter itself. If using ``--outgoing`` or ``--workdir``
|
|
and one of these files was modified, the entire tree will be linted instead of
|
|
just the modified files.
|
|
|
|
Result definition
|
|
-----------------
|
|
|
|
When generating the list of results, the following values are available.
|
|
|
|
.. csv-table::
|
|
:header: "Name", "Description", "Optional"
|
|
:widths: 20, 40, 10
|
|
|
|
"linter", "Name of the linter that flagged this error", ""
|
|
"path", "Path to the file containing the error", ""
|
|
"message", "Text describing the error", ""
|
|
"lineno", "Line number that contains the error", ""
|
|
"column", "Column containing the error", ""
|
|
"level", "Severity of the error, either 'warning' or 'error' (default 'error')", "Yes"
|
|
"hint", "Suggestion for fixing the error", "Yes"
|
|
"source", "Source code context of the error", "Yes"
|
|
"rule", "Name of the rule that was violated", "Yes"
|
|
"lineoffset", "Denotes an error spans multiple lines, of the form (<lineno offset>, <num lines>)", "Yes"
|
|
"diff", "A diff describing the changes that need to be made to the code", "Yes"
|
|
|
|
|
|
Automated testing
|
|
-----------------
|
|
|
|
Every new checker must have associated tests. If your linter is ``mylinter`` then the
|
|
test file should be named ``tools/lint/test/test_mylinter.py`` and any example files
|
|
named like ``tools/lint/test/files/mylinter/my-example-file``. Be sure that your test
|
|
has been added as a section ``["test_mylinter.py"]`` in the manifest ``tools/lint/test/python.toml``.
|
|
|
|
They should be pretty easy to write as most of the work is managed by the Mozlint
|
|
framework. The key declaration is the ``LINTER`` variable which must match
|
|
the linker declaration.
|
|
|
|
As an example, the `ruff test <https://searchfox.org/mozilla-central/source/tools/lint/test/test_ruff.py>`_ looks like the following snippet:
|
|
|
|
.. code-block:: python
|
|
|
|
import mozunit
|
|
LINTER = 'ruff'
|
|
|
|
def test_lint_ruff(lint, paths):
|
|
results = lint(paths('bad.py'))
|
|
assert len(results) == 2
|
|
assert results[0].rule == 'F401'
|
|
assert results[1].rule == 'E501'
|
|
assert results[1].lineno == 5
|
|
|
|
if __name__ == '__main__':
|
|
mozunit.main()
|
|
|
|
As always with tests, please make sure that enough positive and negative cases
|
|
are covered.
|
|
|
|
To run the tests:
|
|
|
|
.. code-block:: shell
|
|
|
|
$ ./mach python-test --subsuite mozlint
|
|
|
|
To run a specific test:
|
|
|
|
.. code-block:: shell
|
|
|
|
./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>`_.
|
|
|
|
Tracking fixed issues
|
|
---------------------
|
|
|
|
All the linters that provide ``fix support`` returns a dictionary instead of a list.
|
|
|
|
``{"results":result,"fixed":fixed}``
|
|
|
|
* results - All the linting errors it was not able to fix
|
|
* fixed - Count of fixed errors (for ``fix=False`` this is 0)
|
|
|
|
Some linters (example: `codespell <https://searchfox.org/mozilla-central/rev/0379f315c75a2875d716b4f5e1a18bf27188f1e6/tools/lint/spell/__init__.py#145-163>`_) might require two passes to count the number of fixed issues.
|
|
Others might just need `some tuning <https://searchfox.org/mozilla-central/rev/0379f315c75a2875d716b4f5e1a18bf27188f1e6/tools/lint/file-whitespace/__init__.py#28,60,85,112>`_.
|
|
|
|
For adding tests to check your fixed count, add a global variable ``fixed = 0``
|
|
and write a function to add your test as mentioned under ``Automated testing`` section.
|
|
|
|
|
|
Here's an example
|
|
|
|
.. code-block:: python
|
|
|
|
fixed = 0
|
|
|
|
|
|
def test_lint_codespell_fix(lint, create_temp_file):
|
|
# Typo has been fixed in the contents to avoid triggering warning
|
|
# 'informations' ----> 'information'
|
|
contents = """This is a file with some typos and information.
|
|
But also testing false positive like optin (because this isn't always option)
|
|
or stuff related to our coding style like:
|
|
aparent (aParent).
|
|
but detects mistakes like mozilla
|
|
""".lstrip()
|
|
|
|
path = create_temp_file(contents, "ignore.rst")
|
|
lint([path], fix=True)
|
|
|
|
assert fixed == 2
|
|
|
|
|
|
Bootstrapping Dependencies
|
|
--------------------------
|
|
|
|
Many linters, especially 3rd party ones, will require a set of dependencies. It
|
|
could be as simple as installing a binary from a package manager, or as
|
|
complicated as pulling a whole graph of tools, plugins and their dependencies.
|
|
|
|
Either way, to reduce the burden on users, linters should strive to provide
|
|
automated bootstrapping of all their dependencies. To help with this,
|
|
``mozlint`` allows linters to define a ``setup`` config, which has the same
|
|
path object format as an external payload. For example (`in-tree version <https://searchfox.org/mozilla-central/source/tools/lint/ruff.yml>`__):
|
|
|
|
.. code-block:: yaml
|
|
|
|
ruff:
|
|
description: Python linter
|
|
include: ['.']
|
|
extensions: ['py']
|
|
type: external
|
|
payload: py.ruff:lint
|
|
setup: py.ruff:setup
|
|
|
|
The setup function takes a single argument, the root of the repository being
|
|
linted. In the case of ``ruff``, it might look like:
|
|
|
|
.. code-block:: python
|
|
|
|
import subprocess
|
|
from shutil import which
|
|
|
|
def setup(root, **lintargs):
|
|
# This is a simple example. Please look at the actual source for better examples.
|
|
if not which("ruff"):
|
|
subprocess.call(["pip", "install", "ruff"])
|
|
|
|
The setup function will be called implicitly before running the linter. This
|
|
means it should return fast and not produce any output if there is no setup to
|
|
be performed.
|
|
|
|
The setup functions can also be called explicitly by running ``mach lint
|
|
--setup``. This will only perform setup and not perform any linting. It is
|
|
mainly useful for other tools like ``mach bootstrap`` to call into.
|
|
|
|
|
|
Adding the linter to the CI
|
|
---------------------------
|
|
|
|
First, the job will have to be declared in Taskcluster.
|
|
|
|
This should be done in the `mozlint Taskcluster configuration <https://searchfox.org/mozilla-central/source/taskcluster/kinds/source-test/mozlint.yml>`_.
|
|
You will need to define a symbol, how it is executed and on what kind of change.
|
|
|
|
For example, for ruff, the configuration is the following:
|
|
|
|
.. code-block:: yaml
|
|
|
|
py-ruff:
|
|
description: run ruff over the gecko codebase
|
|
treeherder:
|
|
symbol: py(ruff)
|
|
run:
|
|
mach: lint -l ruff -f treeherder -f json:/builds/worker/mozlint.json .
|
|
when:
|
|
files-changed:
|
|
- '**/*.py'
|
|
- '**/.ruff.toml'
|
|
|
|
If the linter requires an external program, you will have to install it in the `setup script <https://searchfox.org/mozilla-central/source/taskcluster/docker/lint/system-setup.sh>`_
|
|
and maybe install the necessary files in the `Docker configuration <https://searchfox.org/mozilla-central/source/taskcluster/docker/lint/Dockerfile>`_.
|
|
|
|
.. note::
|
|
|
|
If the defect found by the linter is minor, make sure that it is logged as
|
|
a warning by setting `{"level": "warning"}` in the
|
|
:class:`~mozlint.result.Issue`. This means the defect will not cause a
|
|
backout if landed, but will still be surfaced by reviewbot at review time,
|
|
or when using `-W/--warnings` locally.
|