PSA: Automated code analysis now also in Phabricator

[Jan Keromnes]

TL;DR -- “reviewbot” is now enabled in Phabricator. It reports potential
defects in pending patches for Firefox.

Last year, we announced Code Review Bot (“reviewbot”, née “clangbot”), a
Taskcluster bot that analyzes every patch submitted to MozReview, in order
to automatically detect and report code defects *before* they land in
Nightly:

https://groups.google.com/d/msg/mozilla.dev.platform/TFfjCRdGz_E/8leqTqvBCAAJ

Developer feedback has been very positive, and the bot has caught many
defects, thus improving the quality of Firefox.

Today, we’re happy to announce that reviewbot analyzes every patch
submitted to Phabricator as well.

Here is an example of an automated review in Phabricator:
https://phabricator.services.mozilla.com/D2120

Additionally, we’ve made a number of improvements to this bot over the past
months. Notably:
- Enabled several clang-tidy checks to catch more C/C++ defects (e.g.
performance and security issues)
- Integrated Mozlint in order to catch JS/Python/wpt defects as well
- Fixed several bugs, like the lowercase code snippets in comments
- We’re now detecting up to 5x more defects in some patches

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

As for next steps, we’re currently discussing a few ideas for the project’s
future, including:
- Catch more bugs by comparing defects before & after a patch is applied
(currently, we report defects located on lines that are directly modified
by the patch)
- Evaluate which defect types are generally being fixed or ignored
- Evaluate analyzing Rust code with rust-clippy
- Help with coding style by leveraging clang-format
- Integrate more deeply with Phabricator, e.g. by reporting a build status
for our analysis
- Integrate our analysis with Try, in order to unify our various CI and
code analysis tools

Many thanks to everyone who helped make this a reality: Bastien, who did
most of the implementation and bug fixing, Marco, Andi, Calixte, Sylvestre,
Ahal, the Release Management Analysis team and the Engineering Workflow
team.

Jan

[Andrew Halberstadt]

Congrats, thanks to everyone involved for getting this working!

I really like the idea of comparing errors with and without the patch,
this would let us lint code where linting isn't explicitly enabled in
mach lint/CI. One caveat to doing is that the review bot would need
to make it very clear which issues are "optional" (aka can safely be
ignored) and which ones are "required" (aka would cause a backout
if they don't get fixed).

I have some ideas of making this distinction directly in mozlint by
treating all "warnings" as optional fixes and all "errors" as required
fixes. See https://bugzilla.mozilla.org/show_bug.cgi?id=1460856

Andrew

[Eric Rescorla]

This is amazing and looks super-useful. Really looking forward to seeing
what else we can add in this area!

-Ekr

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

[Ted Mielczarek]

On Tue, Jul 17, 2018, at 9:22 AM, Jan Keromnes wrote:
> TL;DR -- “reviewbot” is now enabled in Phabricator. It reports potential
> defects in pending patches for Firefox.

Great work! This sounds super useful!

-Ted

[Jean-Yves Avenard]

Hi

> On 17 Jul 2018, at 3:22 pm, Jan Keromnes <j...@mozilla.com> wrote:
>
> TL;DR -- “reviewbot” is now enabled in Phabricator. It reports potential defects in pending patches for Firefox.
>

> Last year, we announced Code Review Bot (“reviewbot”, née “clangbot”), a Taskcluster bot that analyzes every patch submitted to MozReview, in order to automatically detect and report code defects *before* they land in Nightly:
>

> https://groups.google.com/d/msg/mozilla.dev.platform/TFfjCRdGz_E/8leqTqvBCAAJ <https://groups.google.com/d/msg/mozilla.dev.platform/TFfjCRdGz_E/8leqTqvBCAAJ>

>
> Developer feedback has been very positive, and the bot has caught many defects, thus improving the quality of Firefox.

This is great … Thank you

When did this become active?

Can existing diff be forced to be scanned if they weren’t before?

JY

[Jan Keromnes]

Thanks all for the enthusiasm, we're also excited about what this can do
for us.

> When did this become active?

Last year on MozReview, yesterday on Phabricator.

> Can existing diff be forced to be scanned if they weren’t before?

The easiest way to force a re-scan is to re-upload the patch (e.g. after
rebasing it).

Note that the bot doesn't publish anything if no defect was detected. A
complete analysis generally takes a few minutes.

Jan

Am 17.07.2018 16:55 schrieb "Jean-Yves Avenard" <jyav...@mozilla.com>:

Hi


On 17 Jul 2018, at 3:22 pm, Jan Keromnes <j...@mozilla.com> wrote:

TL;DR -- “reviewbot” is now enabled in Phabricator. It reports potential
defects in pending patches for Firefox.

Last year, we announced Code Review Bot (“reviewbot”, née “clangbot”), a
Taskcluster bot that analyzes every patch submitted to MozReview, in order
to automatically detect and report code defects *before* they land in
Nightly:

https://groups.google.com/d/msg/mozilla.dev.platform/TFfjCRdGz_E/8leqTqvBCAAJ

[Mike Hommey]

On Tue, Jul 17, 2018 at 05:06:07PM +0200, Jan Keromnes wrote:
> Thanks all for the enthusiasm, we're also excited about what this can do
> for us.
>
> > When did this become active?
>
> Last year on MozReview, yesterday on Phabricator.
>
> > Can existing diff be forced to be scanned if they weren’t before?
>
> The easiest way to force a re-scan is to re-upload the patch (e.g. after
> rebasing it).
>
> Note that the bot doesn't publish anything if no defect was detected. A
> complete analysis generally takes a few minutes.

It might be better to have some indicator that the analysis *did* run,
because otherwise, you can't tell if you should wait longer for the
result or whether your code is clean.

Mike

[Jan Keromnes]

> It might be better to have some indicator that the analysis *did* run,
> because otherwise, you can't tell if you should wait longer for the
> result or whether your code is clean.

Originally, our bot *did* publish a comment when analysis didn't find any
defects in a patch, but this was causing a lot of bugmail noise:
https://groups.google.com/d/msg/mozilla.dev.platform/TFfjCRdGz_E/uCQx4XHZCAAJ

So we decided that it was better to publish comments only when *some*
defects were found.

However, we're now considering adding a custom Phabricator build status
indicator to show when our analysis is running, or if it has succeeded or
failed:

> - Integrate more deeply with Phabricator, e.g. by reporting a build
status for our analysis

This would be a lighter / less chatty integration than publishing comments.

Additionally, our analysis generally finishes in under 8 minutes, so if you
haven't heard from the bot in a while it means your patch is probably fine:
https://p.datadoghq.com/sb/NsBIKn-240d11775d25dce0ef2c47d4b7ce0ae0

Jan