Intent to Enable: Automated Static Analysis feedback in MozReview

[Jan Keromnes]

TL;DR -- We wrote a static analysis bot for MozReview ("clangbot") and it's
about to complain about any patches that would introduce new C/C++ code
defects to Firefox.

Please report any bugs with the bot here: https://bit.ly/2y9N9Vx

In an effort to improve the quality of Firefox, we want to catch
programming errors *before* they even make it into Nightly. To do this, we
created a TaskCluster bot that runs clang static analysis on every patch
submitted to MozReview. It then quickly reports any code defects directly
on MozReview, thus preventing bad patches from landing until all their
defects are fixed. Currently, its feedback is posted in about 10 minutes
after a patch series is published on MozReview.

Here is an example of an automated clangbot review:
https://reviewboard.mozilla.org/r/171868/#review190602

Our bot relies on three types of clang checkers:

- Mozilla specific checkers
<https://hg.mozilla.org/mozilla-central/file/tip/build/clang-plugin/>. They
detect incorrect Gecko programming patterns which could lead to bugs or
security issues.

- Clang-tidy checkers
<https://clang.llvm.org/extra/clang-tidy/checks/list.html>. They aim to
suggest better programming practices and to improve memory efficiency and
performance.

- Clang-analyzer checkers
<https://clang-analyzer.llvm.org/available_checks.html>. These checks are
more advanced, for example some of them can detect dead code or memory
leaks, but as a typical side effect they have false positives. Because of
that, we have disabled them for now, but will enable some of them in the
near future.

The checkers that are currently enabled rarely generate false positives,
and you can find the complete list of enabled checkers
<https://hg.mozilla.org/mozilla-central/file/tip/tools/clang-tidy/config.yaml>
in the tree. You can also run them on your own code with:

> ./mach static-analysis check path/to/file.cpp

This is only the first step. Next, we would like to catch more classes of
programming errors.

- If you know incorrect Gecko programming patterns which could be detected
by static analysis, please send an email to releas...@mozilla.com or
report a bug in the Rewriting and Analysis
<https://bugzilla.mozilla.org/enter_bug.cgi?product=Core&component=Rewriting%20and%20Analysis>
component.

- In parallel, if you see any additional clang-tidy checkers
<https://clang.llvm.org/extra/clang-tidy/checks/list.html> which could be
valuable for our code base if enabled, please let us know so that we can
evaluate them.

- Finally, we are looking into posting reviews to Phabricator in the near
future as well.

Feedback, questions or suggestions welcome.

Thanks!

Andi, Bastien, Jan and Sylvestre

[Jonathan Kew]

On 04/10/2017 09:17, Jan Keromnes wrote:
> You can also run them on your own code with:
>
>> ./mach static-analysis check path/to/file.cpp

Sounds awesome! I tried this locally to see what it would say about a
random(ish) file in the tree, but it ended with the message:

0:42.99 Could not find artifacts for a toolchain build named
`macosx64-clang-tidy`

which sounds like something's missing. It would be nice if it could
automatically obtain the required tools, or at least offer a hint as to
what I should do to set up a suitable environment.

JK

[Jan Keromnes]

> But it's not analyzing patches that are not using MozReview. Will those
patches be analyzed after landing?

Indeed, our bot doesn't run on patches that are attached to Bugzilla
(Splinter reviews) or directly landed.

However, I believe that the Mozilla checkers we use are also run on Try,
and should cause any offending patches to be backed out.

The purpose of our bot is to catch defects at review time, in order to
reduce the number of new defects getting into the tree, but we're also
working on fixing all defects that are currently present in our code base,
which is a much larger project.

> Sounds awesome! I tried this locally to see what it would say about a
random(ish) file in the tree, but it ended with the message:
>
> 0:42.99 Could not find artifacts for a toolchain build named
`macosx64-clang-tidy`
>
> which sounds like something's missing. It would be nice if it could
automatically obtain the required tools, or at least offer a hint as to
what I should do to set up a suitable environment.

Thank you for trying! Nothing is missing, this is actually a regression:
https://bugzilla.mozilla.org/show_bug.cgi?id=1405570

We were able to work around the bug (we've triggered a manual build, so the
command should work again by now) and we're looking into fixing it as soon
as possible.


On Wed, Oct 4, 2017 at 10:11 AM, Nicholas Nethercote <n.neth...@gmail.com
> wrote:

> This sounds interesting!
>
> But it's not analyzing patches that are not using MozReview. Will those
> patches be analyzed after landing?
>
> Nick

>> in the tree. You can also run them on your own code with:
>>
>> > ./mach static-analysis check path/to/file.cpp
>>

>> This is only the first step. Next, we would like to catch more classes of
>> programming errors.
>>
>> - If you know incorrect Gecko programming patterns which could be detected
>> by static analysis, please send an email to releas...@mozilla.com or
>> report a bug in the Rewriting and Analysis
>> <https://bugzilla.mozilla.org/enter_bug.cgi?product=Core&com
>> ponent=Rewriting%20and%20Analysis>
>> component.
>>
>> - In parallel, if you see any additional clang-tidy checkers
>> <https://clang.llvm.org/extra/clang-tidy/checks/list.html> which could be
>> valuable for our code base if enabled, please let us know so that we can
>> evaluate them.
>>
>> - Finally, we are looking into posting reviews to Phabricator in the near
>> future as well.
>>
>> Feedback, questions or suggestions welcome.
>>
>> Thanks!
>>
>> Andi, Bastien, Jan and Sylvestre

>> _______________________________________________
>> dev-platform mailing list
>> dev-pl...@lists.mozilla.org
>> https://lists.mozilla.org/listinfo/dev-platform
>>
>
>

[Sylvestre Ledru]

We do have tooling which analyze changes after landing (coverity is
probably the most visible but we have tooling based on Clang tidy too), we
do report some of the issues but it takes time (and we only report defects
which seem critical or easy to fix like dead code).

Now, because of the future end of splinter and the move to phabricator, our
hope is that the vast majority of the commits will use this platform and
benefit from this work. As you can imagine, the hard work is not to push to
a review platform ;)

Cheers
Sylvestre



Le mer. 4 oct. 2017 à 10:11, Nicholas Nethercote <n.neth...@gmail.com> a
écrit :

[Boris Zbarsky]

On 10/4/17 3:17 AM, Jan Keromnes wrote:
> Please report any bugs with the bot here: https://bit.ly/2y9N9Vx

Not sure this is a bug, but...

I got bugmail today from this bug saying that a patch (not mine; just a
bug I was cced on) didn't have any problems. Can we consider having the
bot add bug noise only when there _is_ a problem?

-Boris

[Jan Keromnes]

> Not sure this is a bug, but...
>
> I got bugmail today from this bug saying that a patch (not mine; just a
bug I was cced on) didn't have any problems. Can we consider having the
bot add bug noise only when there _is_ a problem?

Indeed this is problematic, and we're very sorry about this. We didn't
anticipate that "no defects" messages would be so noisy, but this does
outweigh the benefits of knowing that static analysis completed without
finding any issues.

We've already disabled this "no defects" comment, and are currently
deploying the fix to production, so the bot should stop sending them soon.

Apologies for the bugmail noise.

[Boris Zbarsky]

On 10/4/17 10:32 AM, Jan Keromnes wrote:
> We've already disabled this "no defects" comment, and are currently
> deploying the fix to production, so the bot should stop sending them soon.

Great, thank you!

No need to apologize, by the way. Bugmail noise happens; thank you for
moving on it quickly.

-Boris

[Ehsan Akhgari]

On 10/04/2017 03:17 AM, Jan Keromnes wrote:
> TL;DR -- We wrote a static analysis bot for MozReview ("clangbot") and it's
> about to complain about any patches that would introduce new C/C++ code
> defects to Firefox.

This is fantastic to see, thank you for making it happen!

[Patrick Brosset]

This sounds awesome Jan.
Are there plans to have something similar for JS code linting (with ESLint)?

On Wed, Oct 4, 2017 at 7:14 PM, Ehsan Akhgari <ehsan....@gmail.com>
wrote:

[Jan Keromnes]

Thanks Patrick.

C/C++ was our top priority because code defects are very costly, but we'd
love to make our static analysis bot support additional languages, so we're
looking into integrating with mozlint [0] (the `./mach lint` wrapper
around eslint, flake8 and wptlint), and I think we should also use clippy
to lint Rust code.

[0] https://firefox-source-docs.mozilla.org/tools/lint/index.html

On Mon, Oct 9, 2017 at 3:43 PM, Patrick Brosset <pbro...@mozilla.com>
wrote:

[Jean-Yves Avenard]

Hi

Do you have ways to prevent running this code on some external frameworks?

Whenever we modify a gtest this causes hundreds of errors about not using a default constructor … (which shouldn’t be an error anyway)

[Mike Hommey]

On Tue, Oct 17, 2017 at 10:38:09AM +0200, Jean-Yves Avenard wrote:
> Hi
>
> Do you have ways to prevent running this code on some external
> frameworks?
>
> Whenever we modify a gtest this causes hundreds of errors about not
> using a default constructor … (which shouldn’t be an error anyway)

https://github.com/mozilla-releng/services/issues/658

Mike

[Jan Keromnes]

Thanks glandium!

Yes, reporting defects that are expanded from macros generates a lot of
noise, and it's not always an issue we can fix (e.g. when the macro comes
from third-party code). So we've decided to stop reporting these, and the
fix was merged in time for tomorrow's push to production.

TL;DR After tomorrow, clangbot will stop reporting defects expanded from
macros.

[zbran...@mozilla.com]

This is awesome! As an engineer who has to work with C++ until we advance Rust bindings, I always feel terrible when my reviewers spend their precious time identifying simple C++ errors in my code.


Seeing the advancements in static analysis for C++, rustfmt and eslint for JS, I'm wondering if there's a place to collect a less strict "best practices" analysis - more similar to rust's clippy than fmt.

In Intl/L10n land, we have a bunch of recommendations that are very hard to enforce since they spread between JS, C++ and soon Rust regarding language selection, manipulation, testing of intl output etc.
I'm wondering if there's a place to get those kind of "automated feedback" patterns.
A few examples of what I have on my mind:

- We'd like people to not use "general.useragent.locale" to manipulate app locale anymore, but rather use Services.locale.getRequestedLocales/setRequestedLocales.
- We'd like to make sure people don't write unit tests that test particular output out of Intl APIs (that makes our tests locked to only work in one language and break every time we update our datasets - that's a big no-no in the intl world)
- We'd like to discourage people from altering app locales, and rather test against updated required locales.
- Soon we'll want to recommend using DOMLoclaization.jsm/Localization.jsm API over StringBundle/DTD.

Those things can be encoded as regexps, but they span across programming languages (XUL, XHTML, HTML, XBL, DTD, JS, C++).

Is there any mozilla-clippy project being planned? :)

zb.

[Gregory Szorc]

We have mozlint (
https://firefox-source-docs.mozilla.org/tools/lint/index.html), which is a
mini framework of sorts for integrating various linters into a common
frontend (`mach lint`). Many of these lints run in CI. It is relatively
easy to add new linters (as the docs detail).

Feel free to supplement existing linters with custom checks or write your
own new linter if appropriate. ahal is the mozlint guru if you have
questions.

[Andrew Halberstadt]

On Tue, Oct 17, 2017 at 4:00 PM <zbran...@mozilla.com> wrote:

> Those things can be encoded as regexps, but they span across programming
> languages (XUL, XHTML, HTML, XBL, DTD, JS, C++).
>

To create a new regex-based linter, simply add a file like this:
https://dxr.mozilla.org/mozilla-central/source/tools/lint/test-disable.yml

It'll get picked up automatically and be enabled on any files that match
the include/exclude directives. Just run:
./mach lint <path/to/dir/or/file>

And anything that matches the regex will spit out an error. The above
link is currently the only regex based linter in-tree, so it could
definitely use some improvements (e.g we could allow a list of regexes
in a single linter). It also only works if the offending string is all on
the
same line, which isn't ideal.

Is there any mozilla-clippy project being planned? :)
>

Yes, see bug 1361342. We were blocking on rust stable support in clippy.
Last I heard that still hadn't landed (though this was several months
ago). If anyone knows the status there please chime in on the bug.


> zb.

[Boris Zbarsky]

On 10/4/17 10:32 AM, Jan Keromnes wrote:

> We've already disabled this "no defects" comment, and are currently
> deploying the fix to production, so the bot should stop sending them soon.

It looks like the "no defects" comment is back? See
https://bugzilla.mozilla.org/show_bug.cgi?id=1149250#c30 for example.

-Boris

[Kartikaya Gupta]

It's being investigated:
https://github.com/mozilla-releng/services/issues/707