1894570 - Bogus clang-tidy warnings when Phabricator doesn't know the baseline commit of a patch

Status: NEW

(Developer Infrastructure :: Source Code Analysis, defect)

Description

[Botond Ballo [:botond]]

I've noticed that reviewbot can show bogus clang-tidy warnings on Phabricator revisions where Phabricator doesn't know the baseline commit of a patch (because the parent of the patch is a non-public commit, e.g. a local patch for a different bug).

I've posted some analysis of an example at https://phabricator.services.mozilla.com/D208238#7152546. Quoting the most relevant part:

I think what's happening is:

• Phabricator shows line numbers as they appear in the diff (so basically reflecting line numbers in my local source tree where the patch is applied)
• However, if Phabricator can't figure out the baseline revision of the patch stack (which in this case it couldn't, because I had it applied locally on top of another patch that hasn't landed) then for the purposes of running the clang-tidy job, it applies the patch stack on top of latest m-c, thereby introducing some line number skew
• When Phabricator decides which of the diagnostics from the clang-tidy job to surface on the patch, it filters them based on line numbers that are modified in the patch. In cases of line number skew, this can result in showing diagnostics that actually pertain to other lines in the code.

To be clear, what's "bogus" about the warnings is not that they don't exist in the file, but that reviewbot shows them as pertaining to a different line than they actually do (so e.g. the warning might reference a variable that's supposedly on that line, but there is actually no such variable on that line). In addition, usually the line they actually pertain to is in the "context" part of the diff rather than the modified lines, so it's a pre-existing warning which reviewbot wouldn't show if it got the line number correct.

It would be nice to try to avoid this as such warnings can be quite confusing. One approach could be to compare the content of the source line the diagnostic applies to (in the source tree where the patch was applied to run clang-tidy), to the content of the source line in the diff, and if they don't match then filter out the diagnostic.

Comment 1

[BugBot [:suhaib / :marco/ :calixte]]

The severity field is not set for this bug.
:andi, could you have a look please?

For more information, please visit BugBot documentation.