1389073 - Investigate adding clang-tidy to mozlint family of linters

Status: RESOLVED DUPLICATE of bug 1670949

(Developer Infrastructure :: Lint and Formatting, enhancement, P2)

Description

[Andrew Halberstadt [:ahal]]

Bug 1328454 is adding a new clang-tidy check under |mach static-analysis|. Once that's landed, I'd like to see if it makes sense to re-implement as a mozlint linter, i.e: ./mach lint -l clang-tidy


There are still enough open questions as to whether mozlint makes sense for this or not and that bug has been delayed several times. So I'm happy to have it land first and then do this investigation later. I suspect adding it to mozlint won't be too much trouble.

Comment 1

[(no longer active)]

In light of bug 1328454 comment 51, I think the desired resolution for this bug is WONTFIX.  What do you think, Andrew?

Comment 2

[Andrew Halberstadt [:ahal]]

Ultimately I want to do whatever is most convenient for developers, I won't fight for something if no one wants it :).

I do agree the runtime could be a concern, and is the main reason I wanted to investigate. Note that if we default to only linting files touched (there is a bug on file), runtime might not be a terrible downside. But other than that, none of the reasons you mention really preclude it from being a mozlint linter. They all do some sort of bootstrapping/installation, and there's a --fix option that enables automatic re-writing. The fact that it's compiled might be a reason, but I think it's more of a philosophical one than anything technical. Fwiw, there are already some "linters" that aren't really linters (like wpt-manifest).

The downside to a separate command is that developers need to remember to run both ./mach static-analysis and ./mach lint to test whether they will be backed out. Currently a dev can simply run ./mach lint --outgoing and know whether their change will break any of the linters. A compromise might be to have |mach static-analysis| dispatch to |mach lint --linter clang-tidy| (this is also what the ./mach eslint command does). We could craft it such that it's completely opaque to developers that mozlint is even being used. 

Anyway, even if clang-tidy is disabled by default due to runtime concerns and we have a separate |mach static-analysis| frontend to run it, I still think it would be a good idea to make it use mozlint. This way we don't need to re-invent the wheel for things like linting files touched (--workdir/--outgoing), editing (--edit) and taskcluster tasks.

So I still think this warrants some investigation before WONTFIXING.

Comment 3

[(no longer active)]

(Sorry for the late response, I didn't see your comment until just now.)

(In reply to Andrew Halberstadt [:ahal] from comment #2)
> The downside to a separate command is that developers need to remember to
> run both ./mach static-analysis and ./mach lint to test whether they will be
> backed out.

FWIW developers have to do a lot more than that to ensure not being backed out.  ;-)  A try push can easily replace running both linters and/or static analysis locally if the developer chooses to do so.

> Currently a dev can simply run ./mach lint --outgoing and know
> whether their change will break any of the linters.

Perhaps I didn't make myself clear.  It is not possible to implement ./mach link --outgoing in any meaningful way for C++ static analysis.  As soon as you happen to touch a header, we can't judge from the patch which files we should try to "build" in the static analysis check.  Even worse when you touch an IDL, WebIDL, etc. or something in the build system.  If you're curious, see bug 1328454 where I tried to make that work with the simple case of touching a header and failed.

The difference between a linter and something that actually requires compiling C++ code is far from philosophical.

> A compromise might be to
> have |mach static-analysis| dispatch to |mach lint --linter clang-tidy|
> (this is also what the ./mach eslint command does). We could craft it such
> that it's completely opaque to developers that mozlint is even being used. 

Why is that better?  For people who have to work on mach static-analysis like myself, that seems strictly worse, because we'd need to learn about all of the mach lint infrastructure and be prone to breaking changes in it.

> Anyway, even if clang-tidy is disabled by default due to runtime concerns
> and we have a separate |mach static-analysis| frontend to run it, I still
> think it would be a good idea to make it use mozlint. This way we don't need
> to re-invent the wheel for things like linting files touched
> (--workdir/--outgoing), editing (--edit) and taskcluster tasks.

As I said, static analysis of changes to working directory isn't possible.

I don't know what --edit does, but if it's something like fixing the code after finding issues with it, I haven't looked into it yet, but with clang-tidy that works in a two stage mode (with clang-apply-replacements) after the static analysis has finished running, not sure if that fits with the way mozlint expects things to work or not?

About taskcluster tasks, what we need was done in 1324315 and seems to be working, so if possible please explain what mozlint does which is better than the already working setup that we have.

> So I still think this warrants some investigation before WONTFIXING.

Sure, but honestly so far all I'm hearing is "you should use mozlint because it's a lint command".

I'm more interested to know what problems does this actually solve, so let's please start with the problems, and not with the solution.  I do understand that it's nicer for things to have one command to invoke them through rather than two in the abstract, but I don't see how this applies to linting and static analysis (and tbh I'm not sure if most people really would expect a lint command to run static analysis in the first place) and for the practical reasons I have explained before if we decide to integrate these two beasts we have to teach mozlint about this "special" linter type first.

Are there other problems that this is solving?

Comment 4

[Andrew Halberstadt [:ahal]]

(In reply to Out sick from comment #3)

> Perhaps I didn't make myself clear.  It is not possible to implement ./mach
> link --outgoing in any meaningful way for C++ static analysis.  As soon as
> you happen to touch a header, we can't judge from the patch which files we
> should try to "build" in the static analysis check.  Even worse when you
> touch an IDL, WebIDL, etc. or something in the build system.  If you're
> curious, see bug 1328454 where I tried to make that work with the simple
> case of touching a header and failed.

As long as `clang-tidy <path/to/file>` works, then so will --outgoing.


> The difference between a linter and something that actually requires
> compiling C++ code is far from philosophical.

I think you might be tripping over the name 'mozlint'. Typically a mozlint, let's call it "checker", does something like this:

1. Gathers a list of paths (e.g by explicitly being passed in, or via --outgoing)
2. Shells out to an external binary, passing in said list of paths (binary may even be called multiple times)
3. Formats output from binary and prints it

That's really it, it's a pretty light wrapper. You could theoretically shoe-horn anything into that model, I could create a "checker" that builds Firefox and lists compile-warnings as errors (not saying that's a good idea, just illustrating a point). In step 1, if `clang-tidy` can't handle .h or .idl files, we simply wouldn't forward .h or .idl files.

The main issue you might have with mozlint is that it's poorly named. It very possibly is.


> > A compromise might be to
> > have |mach static-analysis| dispatch to |mach lint --linter clang-tidy|
> > (this is also what the ./mach eslint command does). We could craft it such
> > that it's completely opaque to developers that mozlint is even being used.
>
> Why is that better?  For people who have to work on mach static-analysis
> like myself, that seems strictly worse, because we'd need to learn about all
> of the mach lint infrastructure and be prone to breaking changes in it.

This sounds like you're volunteering to maintain this indefinitely :). I think it's better because we aren't reinventing the wheel over one another all the time. Otherwise we have to maintain:

2 different mach commands
2 different mozreview/phabricator integrations
2 sets of taskcluster configs
2 sets of editor integrations
2 sets of vcs hooks
etc.

When we add a new feature to one, chances are it would also benefit the other. That's two places that need modifying.


> I don't know what --edit does, but if it's something like fixing the code
> after finding issues with it, I haven't looked into it yet, but with
> clang-tidy that works in a two stage mode (with clang-apply-replacements)
> after the static analysis has finished running, not sure if that fits with
> the way mozlint expects things to work or not?

Edit simply opens files containing errors in your editor. It can be combined with --fix such that it only opens errors that can't be fixed automatically. As long as clang-tidy can output the file name + line number, it'll work fine.


> About taskcluster tasks, what we need was done in 1324315 and seems to be
> working, so if possible please explain what mozlint does which is better
> than the already working setup that we have.

In terms of taskcluster, it doesn't do anything better. But it would simplify the configs.

 
> Sure, but honestly so far all I'm hearing is "you should use mozlint because
> it's a lint command".

And I'm just hearing we shouldn't use mozlint because it can't run static analysis commands (even though it can). Keep in mind 'mozlint' and 'mach lint' are two different things. We can use the former without using the latter. Fwiw, this bug was simply supposed to be about investigating `clang-tidy` which I still haven't had a chance to do. I wasn't planning to upload any code to this bug.


> I'm more interested to know what problems does this actually solve, so let's
> please start with the problems, and not with the solution.  I do understand
> that it's nicer for things to have one command to invoke them through rather
> than two in the abstract, but I don't see how this applies to linting and
> static analysis (and tbh I'm not sure if most people really would expect a
> lint command to run static analysis in the first place) and for the
> practical reasons I have explained before if we decide to integrate these
> two beasts we have to teach mozlint about this "special" linter type first.
>
> Are there other problems that this is solving?

The problems it's solving are poor/inconsistent UX, duplicated work, high maintenance, and difficulty creating new "checkers" (this last one is moot in this case as the work is already done). All these problems are derived from having multiple different commands/integrations/infrastructure.

Comment 5

[Mark Banner (:standard8)]

Duping forward to bug 1670949