1960928 - `./mach lint --linter clippy` lints the repo multiple times, skips "excluded" projects

Status: RESOLVED FIXED

(Developer Infrastructure :: Lint and Formatting, task)

Description

[Mark Hammond [:markh] [:mhammond]]

There's some confusion about how clippy works in m-c. The immediate problem for app-services is that (a) the final clippy command executed is something like mach cargo clippy with no paths or packages specified and (b) that it is executed multiple times.

The main problem with (a) is that IIUC, clippy is just going to run over the workspace defined in Cargo.toml, but application-services crates will be explicitly excluded from the workspace to avoid needing to vendor their dev dependencies, thus these crates are never checked, even if the paths are explicitly added to Clippy.yml.

Further, I can't reliably get it to work for files already in the tree. Inspired by bug 1959788, I tried adding a trivial lint failure to toolkit/components/glean/api/src/private/mod.rs, but I can't get the problem reported, let alone fixed (as noted in that bug).

It may or may not be the case that fixing this requires a major refactor of that linter - I'm still quite confused about a number of things (including the fact that bug 1959788 exists at all - it seems quite clear --fix never makes it to the clippy command-line. I suspect it's simply the case that I don't really understand clippy and how clippy.yml works.

From Matrix:
me:

If I'm reading https://searchfox.org/mozilla-central/source/tools/lint/clippy/__init__.py#71 correctly, then it always runs plain-old mach cargo clippy without specifying any paths or packages. However, the paths specified on the cmdline are simply filters for the messages (ie, paths are not "check these files" but rather "only report these files"). IIUC, this means it's not possible to lint crates "excluded" from the workspace due to their dev dependencies. Is there something I'm missing here?

me:

(I'm definitely missing something here - according to print/log debugging, Run clippy with = /Users/skip/.mozbuild/srcdirs/mozilla-unified-2057409e14be/_virtualenvs/lint/bin/python /Users/skip/src/moz/mozilla-unified/mach --log-no-times cargo clippy -- --message-format=json is executed many times for a single lint of the repo, which doesn't really make sense)

ahal:

looks like this is a result of shoehorning clippy into mozlint

So mozlint accepts a bunch of paths on the command line. If no paths are specified, it implicitly adds every top level directory as a path and spawns a separate worker subprocess for each one (thus why you see clippy being invoked multiple times). Ideally, each subprocess is only responsible for linting its respective directory, but I'm guessing clippy runs during compilation and fundamentally doesn't work with paths being passed in

Further, it looks like this lint integration is aware of this, so to get around it, it's getting a list of all issues, and then post-filtering only the issues that match one of the passed in paths.

This means that yes, we are running clippy unnecessarily many times. I'm not sure what the best way to solve this would be.
some kind of configuration a lint integration can set to denote it should only have a single subprocess or something like that

Comment 1

[Andrew Halberstadt [:ahal]]

I think we'd need to do something like:

1. Implement a config option that tells mozlint to defer subprocess handling to the lint integration (so it only spawns a single worker subprocess for clippy).
2. Have the clippy lint integration retrieve a list of Cargo.toml files that are in ancestor directories of any of the passed in paths.
3. Compare that list of Cargo.toml files against the excluded paths, and filter as appropriate.
4. Lint the remaining Cargo.toml files, ideally spawning a subprocess for each.
5. Post filter the results to match only issues under the specified paths (as it does now).

That being said, we might also want to revisit whether clippy should be integrated as part of ./mach lint or not, given how different it is from other linters. I'm not involved with Rust code in m-c much though, so I don't think that should be my call.

Comment 2

[Ben Dean-Kawamura [:bdk]]

Attachment: Bug 1960928 - `./mach lint` clippy updates, r=ahal

Make clippy a global linter. This means it only runs once per
workspace, which is good because cargo handles parallelization
internally. There's no reason to split things out into multiple commands.

Updated the global linter config handling to pass a list of files. This
is needed for clippy and we can just ignore them for the android lints.

Updated the clippy linter to work with crates outside of gkrust. For
these crates, we call cargo clippy directly and does not filter by
path name. These crates need to be listed in tools/lint/clippy.yml,
currently this is only uniffi-bindgen-gecko-js, but it's easy to add
more.

Fixed some lint warnings in the mozlint package itself.

Comment 3

[Ben Dean-Kawamura [:bdk]]

Mark also mentioned that this command will succeed when clippy returns a non-zero exit value, but no clippy messages are identified as errors. I took some time to check this out and I believe the reason this happens is because it's using ./mach cargo clippy to run things, which creates a pretty elaborate pipeline that ends up running make to run clippy. By the time control reaches back to python the clippy exit code is no longer available (the mach command always returns 0 as far as I can tell).

It seems possible to fix, but not really necessary for this issue so I left it there. It wouldn't be too hard to fix it for the non-gkrust crates, but that doesn't seem worth it to me considering they represent a very small portion of the Rust code and the clippy command is much less likely to fail for those anyways.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d93b44ff9938
`./mach lint` clippy updates, r=ahal,taskgraph-reviewers

Comment 5

[pstanciu]

https://hg.mozilla.org/mozilla-central/rev/d93b44ff9938