shipit_static_analysis: Enable `before/after` comparison in production

[jankeromnes]

We are currently experimenting with a better heuristic to detect which code defects were caused by a given patch (as opposed to code defects that were already present in the code before the patch).

Currently, our bot uses IN_PATCH (i.e. "only report defects that are on lines directly modified by the patch"). While good enough, this heuristic misses new defects that are adjacent in the control graph (e.g. if you modify a function, you might indirectly cause a defect at a call site somewhere else in the code).

Our new approach is called BEFORE_AFTER, and runs analysis twice: once before applying the patch, and once more after the patch is applied, in order to detect which defects are actually new.

To enable BEFORE_AFTER in production, we should evaluate its results on staging, and verify that they are indeed superior, and also bug free.

TODO:

• 1. Verify that BEFORE_AFTER works on staging, without any bugs
• 2. Determine whether BEFORE_AFTER is a clear improvement over IN_PATCH (we'll likely want to combine both in order to catch more bugs)
• 3. Enable BEFORE_AFTER in production
• 4. Verify that it also works well in production, and that developers are happy with the results

[jankeromnes]

Regarding 1. Verify that BEFORE_AFTER works on staging, without any bugs, we believe that:

• the bot reported 6 extra defects that were not in the patch (pre-existing issues, should not be reported):
  • modernize-raw-string-literal
  • modernize-use-equals-default * 5

from #1166 (comment) may be a BEFORE_AFTER bug (i.e. these defects are considered "newly introduced by the patch", even though they look like pre-existing defects).

Additionally, running the same analysis on staging but with IN_PATCH instead of BEFORE_AFTER will reveal further differences between these heuristics. I'll do that soon.

[jankeromnes]

Regarding 2. Determine whether BEFORE_AFTER is a clear improvement over IN_PATCH, I'm worried that BEFORE_AFTER will work too well, and that we will miss opportunities for developers to fix pre-existing defects, because it will no longer report pre-existing defects on lines modified by a developer.

E.g. if I refactor a line containing a serious defect, I'm happy if the bot complains about this defect, even though I didn't actually cause it. That way, I can drive-by fix it along with any other nits, and thus improve the Firefox source code.

This is a minor problem, but if the bot stops surfacing these "improvement opportunities", I'm afraid that we'll miss serious defects that could easily be fixed by someone stopping by. Maybe we could somehow combine IN_PATCH and BEFORE_AFTER?

@sylvestre @La0 any thoughts?

[sylvestre]

In general, developers are like "oh, this isn't really my problem or what I am trying to fix now".

In a perfect world, we would be able to separate ride along defects and new defect.

Maybe we would start a discussion on dev-platform about that?

[jankeromnes]

New validation run to find before/after bugs:

• 1/2: PHID-DIFF-sj6o2ldoy3qrly32tf7x https://tools.taskcluster.net/task-inspector/#EUINu1tdT9eq9cFtBEQbKA
• 2/2: PHID-DIFF-3reimpzvzlbnn7hhmjrc https://tools.taskcluster.net/task-inspector/#N6C9GhV-Ty2ZdugKl9Jezw

EDIT: This validates staging, which is currently 9 days / 18 commits behind master (might have been smarter to push to staging first, e.g. because it's missing a Rust update)

[jankeromnes]

Both failed. Then I deployed master to staging, and re-uploaded:

• 1/2: PHID-DIFF-q5imboxwn3nvorgjoisi https://tools.taskcluster.net/task-inspector/#ZiWfvHJ3S0qxE-h9pBuvBw
• 2/2: PHID-DIFF-5tgaftrsvnjrezdov5an https://tools.taskcluster.net/task-inspector/#cDD__u_oTQ-6fYtZV6T0wQ

EDIT: This revealed #1591. I also forgot to replace IN_PATCH with BEFORE_AFTER in the staging secret, so probably my validation run wasn't useful here anyway (is_new is no on all detected defects, which seems wrong)

[jankeromnes]

Again:

• 1/2, PHID-DIFF-3drb6zzj4ezbssrxdluu, https://tools.taskcluster.net/task-inspector/#fUSW_VRGTJ-Zza0B0PcTZg
• 2/2, PHID-DIFF-ffrmwh6qhekydjukpozu, https://tools.taskcluster.net/task-inspector/#K8SWxa5BRESxGSygYkvueg

This time with BEFORE_AFTER.

EDIT: Both Tasks timed out, without logs.

[jankeromnes]

Again:

• 1/2, PHID-DIFF-55yik5comrvux3wcuixz, https://tools.taskcluster.net/task-inspector/#bp0vrxRPQVKs7wC8C9ziBg
• 2/2, PHID-DIFF-3imcuyzrmmhdpgrr5w6r, https://tools.taskcluster.net/task-inspector/#GgYHF5ywShWzK28Mej6S2Q

[jankeromnes]

5th time is the charm:

• 1/2, https://phabricator-dev.allizom.org/D714, PHID-DIFF-ikyf37p6ka6esdx7iupi, https://tools.taskcluster.net/task-inspector/#cACr-2drSR2rORpcf-JQZQ (found 143 defects, while we expect about 20)
• 2/2, https://phabricator-dev.allizom.org/D715, PHID-DIFF-fwfco4xauckjwpx3p25d, https://tools.taskcluster.net/task-inspector/#MNEWcIS1TM2A85iYh3DTkA (found 126 defects, while we expect about 5)

I won't have time to look in depth into these results, but it would be useful to go through all the reported defects one by one, and verify whether it's actually a new defect caused by the patch, or if it's a pre-existing defect that should not have been reported as new.

Hint: you can look at the JSON report artifact in each task, as it will contain more details on the detected defects, such as the is_new flag, which should represent if the defect was pre-existing or if it's caused by the patch.

Also, it would be good to go through the patch itself, and see if all the defects it introduces are reported, and whether they're reported as new or not (in general, before in a comment next to a defect in the patch should mean is_new: False in the defect, and after in a comment should mean is_new: True).

[jankeromnes]

Attempted another staging validation with 99a13b6 deployed... and it failed:

• QfLd7Z2SQJaireYqMBnPUQ | 1/2. PHID-DIFF-7c3wyz42vvsz54sghexw | Error: unknown
• FRJKWSj8TKuLigrMIr_d_A | 2/2. PHID-DIFF-fq6cdeuk5t5gkd7zi7br | Error: unknown

In both tasks the error is:

static_analysis_bot.workflow: Run ClangFormat
static_analysis_bot.clang.format: Running ./mach clang-format (cmd='gecko-env ./mach --log-no-times clang-format')
+ exec ./mach --log-no-times clang-format
datadog.api: 202 POST https://api.datadoghq.com/api/v1/series (320.7631ms)
static_analysis_bot.workflow: Run MozLint
datadog.api: 202 POST https://api.datadoghq.com/api/v1/series (321.8524ms)
static_analysis_bot.cli: Static analysis failure (revision=PHID-DIFF-7c3wyz42vvsz54sghexw error=ValueError('too many values to unpack (expected 2)',))
Traceback (most recent call last):
  File "/nix/store/9wp9sxhplxx9hh59xw8vi9s1ldpzcykb-python3.6-mozilla-staticanalysis-bot-1.0.0/bin/..static-analysis-bot-wrapped-wrapped", line 12, in <module>
    sys.exit(main())
  File "/nix/store/8akzn6hhq0dg211vdabyn59hqa99l8qy-python3.6-Click-7.0/lib/python3.6/site-packages/click/core.py", line 764, in __call__
    return self.main(*args, **kwargs)
  File "/nix/store/8akzn6hhq0dg211vdabyn59hqa99l8qy-python3.6-Click-7.0/lib/python3.6/site-packages/click/core.py", line 717, in main
    rv = self.invoke(ctx)
  File "/nix/store/8akzn6hhq0dg211vdabyn59hqa99l8qy-python3.6-Click-7.0/lib/python3.6/site-packages/click/core.py", line 956, in invoke
    return ctx.invoke(self.callback, **ctx.params)
  File "/nix/store/8akzn6hhq0dg211vdabyn59hqa99l8qy-python3.6-Click-7.0/lib/python3.6/site-packages/click/core.py", line 555, in invoke
    return callback(*args, **kwargs)
  File "/nix/store/p2v4azvgp1vphcjqzi65mv5g33fvybl1-python3.6-python3.6-mozilla-cli-common-1.0.0/lib/python3.6/site-packages/cli_common/cli.py", line 35, in wrapper
    return func(*args, **kwargs)
  File "/nix/store/bwfygfcdvis9wd1c1v51xwnwhw1hx0a0-python3-3.6.6/lib/python3.6/contextlib.py", line 52, in inner
    return func(*args, **kwds)
  File "/nix/store/9wp9sxhplxx9hh59xw8vi9s1ldpzcykb-python3.6-mozilla-staticanalysis-bot-1.0.0/lib/python3.6/site-packages/static_analysis_bot/cli.py", line 116, in main
    w.run(revision)
  File "/nix/store/9wp9sxhplxx9hh59xw8vi9s1ldpzcykb-python3.6-mozilla-staticanalysis-bot-1.0.0/lib/python3.6/site-packages/static_analysis_bot/workflow.py", line 213, in run
    before_patch, _ = self.detect_issues(analyzers, revision)
ValueError: too many values to unpack (expected 2)
datadog.api: 202 POST https://api.datadoghq.com/api/v1/series (353.1992ms)
[taskcluster 2018-11-23 11:43:53.077Z] === Task Finished ===

EDIT: Filed as #1699.