Page MenuHomePhabricator

[clangd] Respect clang-tidy suppression comments

Authored by nridge on Apr 21 2019, 1:30 PM.

Diff Detail


Event Timeline

nridge created this revision.Apr 21 2019, 1:30 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 21 2019, 1:30 PM
nridge edited reviewers, added: ilya-biryukov; removed: hokein.Apr 25 2019, 9:58 PM

Changing reviewers as I understand @hokein is on vacation.

Thanks for doing this!

395 ↗(On Diff #196013)

There may be a trap here for clangd.

When building an AST with a preamble (which is when we run clang-tidy checks), the source code of the main file is mapped in the SourceManager as usual, but headers are not.
An attempt to get the buffer results in the SourceManager reading the file from disk. If the content has changed then all sorts of invariants break and clang will typically crash.

@ilya-biryukov knows the details here better than me.

Of course this is only true for trying to read from header file locations, and we only run clang-tidy over main-file-decls. But:

  • clang-tidy checks can emit diagnostics anywhere, even outside the AST range they run over directly
  • while StoreDiags does filter out diagnostics that aren't in the main file, this is pretty nuanced (e.g. a note in the main file is sufficient to include) and this logic runs after the filtering added by this patch
  • the way that LineIsMarkedWithNOLINTinMacro loops suggests that even if the diagnostic is in the main-file, we'll look for NOLINT in other files

Maybe this means we can't accurately calculate suppression for clang-tidy warnings in macro locations, or outside the main file. I think *always* filtering these out (on the clangd side) might be OK.

254 ↗(On Diff #196013)

the interaction between ClangdDiagnosticConsumer and StoreDiags seems a little mysterious - I think the total behavior would be easier to understand (and debug) if StoreDiags knew about the filtering.

At the same time, I see why you want to keep the clang-tidy-specific logic in this file.

Maybe we could add a filterDiagnostics(function<bool(Diagnostic&)>) or so into StoreDiags, similar to contributeFixes? That way the policy stuff (which diagnostics to filter) stays here, and the mechanism (e.g. tracking associated notes) is visible in StoreDiags.

nridge updated this revision to Diff 197040.Apr 28 2019, 3:06 PM
nridge marked 3 inline comments as done.

Address review comments

nridge added inline comments.Apr 28 2019, 3:07 PM
395 ↗(On Diff #196013)

Thanks for pointing this out! This should be addressed in the updated patch.

Friendly review ping.

Thanks, this looks nice. Main ideas here:

  • it'd be nice to get rid of the subclassing in the diag consumer if we can
  • i'm not sure the logic around LastErrorWasIgnored is completely accurate
  • can we add a unit test for this? The existing clang-tidy diag tests are in unittests/DiagnosticsTests.cpp
243 ↗(On Diff #197040)

Thanks, this is much cleaner.

I think we don't need the subclass at all though - just a filter function here, and call setFilter after setting up clang-tidy?

338 ↗(On Diff #197040)

I believe you can just call setFilter at the end of this block

309 ↗(On Diff #197040)

do we need to set LastErrorWasIgnored here (in the case that it's not a note)?

I guess it's pretty unlikely that e.g. a problem with the command-line will get a note that has a source location...

314 ↗(On Diff #197040)

Can you add a comment here, about notes attached to suppressed errors? This code is under-commented (sorry), but I think it would aid understanding.

319 ↗(On Diff #197040)

If I read the code right, we're now applying the filter to a note (whose primary diagnostic was ignored), which we shouldn't. We're also updating the LastErrorWasIgnored flag, which we shouldn't for notes.

I think we might prefer to merge the new logic with the bit below the lambdas, which already looks at note/non-note status.


if (!isNote(...)) {
  LastErrorWasIgnored = !Filter(...);
  if (LastErrorWasIgnored)
} else {
  // Handle a note...
  if (LastErrorWasIgnored)
118 ↗(On Diff #197040)

nit: IsInsideMainFile is information already inside the diagnostic.

Diagnostics.cpp does compute this information (in an unneccesarily verbose way IMO) but I'd prefer not to expose that in this public interface - the filter function can recompute.

122 ↗(On Diff #197040)

I know I suggested this name, but just an idea...

The difficulty with filter is it's unclear whether true means "this passes the filter" or "this is filtered out". It may be clearer to invert the sense and call this suppress.

Anyway, totally up to you, can definitely live with filter.

132 ↗(On Diff #197040)

I think we're talking about last non-note diagnostic, right? Warnings count here I think.

At the risk of being verbose, maybe LastPrimaryDiagnosticWasIgnored?

nridge updated this revision to Diff 199551.May 14 2019, 9:22 PM
nridge marked 9 inline comments as done.

Address review comments, except for the derived class one, about which I have a question

nridge added inline comments.May 14 2019, 9:23 PM
243 ↗(On Diff #197040)

I rely on the derived class more in the follow-up patch D61841 where I override HandleDiagnostic(). Should I still remove it from this patch?

309 ↗(On Diff #197040)

If that does happen, then it looks like storing the note in that case is a pre-existing behaviour, which this patch just preserves.

132 ↗(On Diff #197040)

I copied the name from ClangTidyDiagnosticConsumer::LastErrorWasIgnored, but sure, I can rename this as suggested.

sammccall accepted this revision.May 15 2019, 2:06 AM

Thanks, this looks good. I think we should avoid the subclassing, but any of {generalize in the next patch, different approach for the next patch, subclass for now and clean up later} seems fine.

243 ↗(On Diff #197040)

Sorry about not understanding the interaction here, I wasn't aware of the warnings-to-errors work.
Yes, I'd prefer to avoid the subclassing for that too.

A couple of options spring to mind here:

  • generalize from suppressDiagnostics(callback -> bool) to adjustLevel(callback -> DiagnosticLevel) or similar, where Ignored is the current suppression behavior.
  • apply warnings-to-errors at the end, at the clangd::Diag level. This captures that a diag came from clang-tidy, and the check name, so seems feasible.

(sorry about the churn here, I wasn't aware of the followup patch)

115 ↗(On Diff #199551)

nit: this really doesn't seem worth adding to a public interface, is D.hasSourceManager() && D.getSourceManager().isWrittenInMainFile(D.getLocation()) really too verbose?

(I don't think the helper function in Diagnostics.cpp is justified either, but unrelated to this patch)

771 ↗(On Diff #199551)

nit: can we group this with the other ClangTidy test?
TEST(DiagnosticTest, ClangTidySuppressionComment) and move it up in the file?

This revision is now accepted and ready to land.May 15 2019, 2:06 AM
nridge updated this revision to Diff 199954.May 16 2019, 6:46 PM
nridge marked 5 inline comments as done.

Address latest review comments

nridge added a comment.EditedMay 17 2019, 8:05 AM

Could someone merge this and D61841 now that they're approved? Thanks!

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMay 18 2019, 9:05 PM

Thank you MaskRay!