[PATCH] D60953: [clangd] Respect clang-tidy suppression comments

[Nathan Ridge via Phabricator via cfe-commits]

nridge added a comment.

Thank you MaskRay!


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60953/new/

https://reviews.llvm.org/D60953



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60953: [clangd] Respect clang-tidy suppression comments

[Fangrui Song via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL361112: [clangd] Respect clang-tidy suppression comments 
(authored by MaskRay, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D60953?vs=199954=200167#toc

Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60953/new/

https://reviews.llvm.org/D60953

Files:
  clang-tools-extra/trunk/clang-tidy/ClangTidyDiagnosticConsumer.cpp
  clang-tools-extra/trunk/clang-tidy/ClangTidyDiagnosticConsumer.h
  clang-tools-extra/trunk/clangd/ClangdUnit.cpp
  clang-tools-extra/trunk/clangd/Diagnostics.cpp
  clang-tools-extra/trunk/clangd/Diagnostics.h
  clang-tools-extra/trunk/clangd/unittests/DiagnosticsTests.cpp

Index: clang-tools-extra/trunk/clang-tidy/ClangTidyDiagnosticConsumer.h
===
--- clang-tools-extra/trunk/clang-tidy/ClangTidyDiagnosticConsumer.h
+++ clang-tools-extra/trunk/clang-tidy/ClangTidyDiagnosticConsumer.h
@@ -217,6 +217,23 @@
   bool AllowEnablingAnalyzerAlphaCheckers;
 };
 
+/// Check whether a given diagnostic should be suppressed due to the presence
+/// of a "NOLINT" suppression comment.
+/// This is exposed so that other tools that present clang-tidy diagnostics
+/// (such as clangd) can respect the same suppression rules as clang-tidy.
+/// This does not handle suppression of notes following a suppressed diagnostic;
+/// that is left to the caller is it requires maintaining state in between calls
+/// to this function.
+/// The `CheckMacroExpansion` parameter determines whether the function should
+/// handle the case where the diagnostic is inside a macro expansion. A degree
+/// of control over this is needed because handling this case can require
+/// examining source files other than the one in which the diagnostic is
+/// located, and in some use cases we cannot rely on such other files being
+/// mapped in the SourceMapper.
+bool ShouldSuppressDiagnostic(DiagnosticsEngine::Level DiagLevel,
+  const Diagnostic , ClangTidyContext ,
+  bool CheckMacroExpansion = true);
+
 /// \brief A diagnostic consumer that turns each \c Diagnostic into a
 /// \c SourceManager-independent \c ClangTidyError.
 //
@@ -246,7 +263,7 @@
 
   /// \brief Updates \c LastErrorRelatesToUserCode and LastErrorPassesLineFilter
   /// according to the diagnostic \p Location.
-  void checkFilters(SourceLocation Location, const SourceManager& Sources);
+  void checkFilters(SourceLocation Location, const SourceManager );
   bool passesLineFilter(StringRef FileName, unsigned LineNumber) const;
 
   ClangTidyContext 
Index: clang-tools-extra/trunk/clang-tidy/ClangTidyDiagnosticConsumer.cpp
===
--- clang-tools-extra/trunk/clang-tidy/ClangTidyDiagnosticConsumer.cpp
+++ clang-tools-extra/trunk/clang-tidy/ClangTidyDiagnosticConsumer.cpp
@@ -166,11 +166,13 @@
 
   bool contains(StringRef S) {
 switch (auto  = Cache[S]) {
-  case Yes: return true;
-  case No: return false;
-  case None:
-Result = Globs.contains(S) ? Yes : No;
-return Result == Yes;
+case Yes:
+  return true;
+case No:
+  return false;
+case None:
+  Result = Globs.contains(S) ? Yes : No;
+  return Result == Yes;
 }
 llvm_unreachable("invalid enum");
   }
@@ -387,16 +389,30 @@
   return false;
 }
 
+namespace clang {
+namespace tidy {
+
+bool ShouldSuppressDiagnostic(DiagnosticsEngine::Level DiagLevel,
+  const Diagnostic , ClangTidyContext ,
+  bool CheckMacroExpansion) {
+  return Info.getLocation().isValid() &&
+ DiagLevel != DiagnosticsEngine::Error &&
+ DiagLevel != DiagnosticsEngine::Fatal &&
+ (CheckMacroExpansion ? LineIsMarkedWithNOLINTinMacro
+  : LineIsMarkedWithNOLINT)(Info.getSourceManager(),
+Info.getLocation(),
+Info.getID(), Context);
+}
+
+} // namespace tidy
+} // namespace clang
+
 void ClangTidyDiagnosticConsumer::HandleDiagnostic(
 DiagnosticsEngine::Level DiagLevel, const Diagnostic ) {
   if (LastErrorWasIgnored && DiagLevel == DiagnosticsEngine::Note)
 return;
 
-  if (Info.getLocation().isValid() && DiagLevel != DiagnosticsEngine::Error &&
-  DiagLevel != DiagnosticsEngine::Fatal &&
-  LineIsMarkedWithNOLINTinMacro(Info.getSourceManager(),
-Info.getLocation(), Info.getID(),
-Context)) {
+  if (ShouldSuppressDiagnostic(DiagLevel, Info, Context)) {
 ++Context.Stats.ErrorsIgnoredNOLINT;
 // Ignored a warning, should ignore related notes as well
 LastErrorWasIgnored = true;
Index: 

[PATCH] D60953: [clangd] Respect clang-tidy suppression comments

[Nathan Ridge via Phabricator via cfe-commits]

nridge added a comment.

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60953/new/

https://reviews.llvm.org/D60953



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60953: [clangd] Respect clang-tidy suppression comments

[Nathan Ridge via Phabricator via cfe-commits]

nridge updated this revision to Diff 199954.
nridge marked 5 inline comments as done.
nridge added a comment.

Address latest review comments


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60953/new/

https://reviews.llvm.org/D60953

Files:
  clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
  clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h
  clang-tools-extra/clangd/ClangdUnit.cpp
  clang-tools-extra/clangd/Diagnostics.cpp
  clang-tools-extra/clangd/Diagnostics.h
  clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp

Index: clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
===
--- clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
+++ clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
@@ -207,6 +207,26 @@
"multiple unsequenced modifications to 'y'")));
 }
 
+TEST(DiagnosticTest, ClangTidySuppressionComment) {
+  Annotations Main(R"cpp(
+int main() {
+  int i = 3;
+  double d = 8 / i;  // NOLINT
+  // NOLINTNEXTLINE
+  double e = 8 / i;
+  double f = [[8]] / i;
+}
+  )cpp");
+  TestTU TU = TestTU::withCode(Main.code());
+  TU.ClangTidyChecks = "bugprone-integer-division";
+  EXPECT_THAT(
+  TU.build().getDiagnostics(),
+  UnorderedElementsAre(::testing::AllOf(
+  Diag(Main.range(), "result of integer division used in a floating "
+ "point context; possible loss of precision"),
+  DiagSource(Diag::ClangTidy), DiagName("bugprone-integer-division";
+}
+
 TEST(DiagnosticsTest, Preprocessor) {
   // This looks like a preamble, but there's an #else in the middle!
   // Check that:
@@ -767,6 +787,7 @@
  "a type specifier for all declarations"),
   WithNote(Diag(Header.range(), "error occurred here");
 }
+
 } // namespace
 
 } // namespace clangd
Index: clang-tools-extra/clangd/Diagnostics.h
===
--- clang-tools-extra/clangd/Diagnostics.h
+++ clang-tools-extra/clangd/Diagnostics.h
@@ -128,17 +128,25 @@
 
   using DiagFixer = std::function(DiagnosticsEngine::Level,
const clang::Diagnostic &)>;
+  using DiagFilter =
+  std::function;
   /// If set, possibly adds fixes for diagnostics using \p Fixer.
   void contributeFixes(DiagFixer Fixer) { this->Fixer = Fixer; }
+  /// If set, ignore diagnostics for which \p SuppressionFilter returns true.
+  void suppressDiagnostics(DiagFilter SuppressionFilter) {
+this->SuppressionFilter = SuppressionFilter;
+  }
 
 private:
   void flushLastDiag();
 
   DiagFixer Fixer = nullptr;
+  DiagFilter SuppressionFilter = nullptr;
   std::vector Output;
   llvm::Optional LangOpts;
   llvm::Optional LastDiag;
   llvm::DenseSet IncludeLinesWithErrors;
+  bool LastPrimaryDiagnosticWasSuppressed = false;
 };
 
 } // namespace clangd
Index: clang-tools-extra/clangd/Diagnostics.cpp
===
--- clang-tools-extra/clangd/Diagnostics.cpp
+++ clang-tools-extra/clangd/Diagnostics.cpp
@@ -514,6 +514,12 @@
 // Handle the new main diagnostic.
 flushLastDiag();
 
+if (SuppressionFilter && SuppressionFilter(DiagLevel, Info)) {
+  LastPrimaryDiagnosticWasSuppressed = true;
+  return;
+}
+LastPrimaryDiagnosticWasSuppressed = false;
+
 LastDiag = Diag();
 LastDiag->ID = Info.getID();
 FillDiagBase(*LastDiag);
@@ -528,6 +534,13 @@
 }
   } else {
 // Handle a note to an existing diagnostic.
+
+// If a diagnostic was suppressed due to the suppression filter,
+// also suppress notes associated with it.
+if (LastPrimaryDiagnosticWasSuppressed) {
+  return;
+}
+
 if (!LastDiag) {
   assert(false && "Adding a note without main diagnostic");
   IgnoreDiagnostics::log(DiagLevel, Info);
Index: clang-tools-extra/clangd/ClangdUnit.cpp
===
--- clang-tools-extra/clangd/ClangdUnit.cpp
+++ clang-tools-extra/clangd/ClangdUnit.cpp
@@ -113,12 +113,12 @@
   }
 
   void EndOfMainFile() {
-for (const auto& Entry : MainFileMacros)
+for (const auto  : MainFileMacros)
   Out->push_back(Entry.getKey());
 llvm::sort(*Out);
   }
 
- private:
+private:
   const SourceManager 
   bool InMainFile = true;
   llvm::StringSet<> MainFileMacros;
@@ -332,6 +332,30 @@
 CTContext->setASTContext(>getASTContext());
 CTContext->setCurrentFile(MainInput.getFile());
 CTFactories.createChecks(CTContext.getPointer(), CTChecks);
+ASTDiags.suppressDiagnostics([](
+ DiagnosticsEngine::Level DiagLevel,
+ const clang::Diagnostic ) {
+  if (CTContext) {
+bool IsClangTidyDiag = 

[PATCH] D60953: [clangd] Respect clang-tidy suppression comments

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.

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.




Comment at: clang-tools-extra/clangd/ClangdUnit.cpp:243
+// future).
+class ClangdDiagnosticConsumer : public StoreDiags {
+public:

nridge wrote:
> sammccall wrote:
> > 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?
> 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?
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)



Comment at: clang-tools-extra/clangd/Diagnostics.h:115
 
+/// Check if a diagnostic is inside the main file.
+bool isInsideMainFile(const clang::Diagnostic );

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)



Comment at: clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp:771
+
+TEST(ClangTidy, SuppressionComment) {
+  Annotations Main(R"cpp(

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60953/new/

https://reviews.llvm.org/D60953



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60953: [clangd] Respect clang-tidy suppression comments

[Nathan Ridge via Phabricator via cfe-commits]

nridge added inline comments.



Comment at: clang-tools-extra/clangd/ClangdUnit.cpp:243
+// future).
+class ClangdDiagnosticConsumer : public StoreDiags {
+public:

sammccall wrote:
> 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?
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?



Comment at: clang-tools-extra/clangd/Diagnostics.cpp:309
 
   if (!LangOpts || !Info.hasSourceManager()) {
 IgnoreDiagnostics::log(DiagLevel, Info);

sammccall wrote:
> 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...
If that does happen, then it looks like storing the note in that case is a 
pre-existing behaviour, which this patch just preserves.



Comment at: clang-tools-extra/clangd/Diagnostics.h:132
   llvm::Optional LastDiag;
+  bool LastErrorWasIgnored = false;
 };

sammccall wrote:
> I think we're talking about last non-note diagnostic, right? Warnings count 
> here I think.
> 
> At the risk of being verbose, maybe `LastPrimaryDiagnosticWasIgnored`?
I copied the name from `ClangTidyDiagnosticConsumer::LastErrorWasIgnored`, but 
sure, I can rename this as suggested.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60953/new/

https://reviews.llvm.org/D60953



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60953: [clangd] Respect clang-tidy suppression comments

[Nathan Ridge via Phabricator via cfe-commits]

nridge updated this revision to Diff 199551.
nridge marked 9 inline comments as done.
nridge added a comment.

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60953/new/

https://reviews.llvm.org/D60953

Files:
  clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
  clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h
  clang-tools-extra/clangd/ClangdUnit.cpp
  clang-tools-extra/clangd/Diagnostics.cpp
  clang-tools-extra/clangd/Diagnostics.h
  clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp

Index: clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
===
--- clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
+++ clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
@@ -767,6 +767,24 @@
  "a type specifier for all declarations"),
   WithNote(Diag(Header.range(), "error occurred here");
 }
+
+TEST(ClangTidy, SuppressionComment) {
+  Annotations Main(R"cpp(
+int main() {
+  int i = 3;
+  double d = 8 / i;  // NOLINT
+  // NOLINTNEXTLINE
+  double e = 8 / i;
+  double f = [[8]] / i;
+}
+  )cpp");
+  TestTU TU = TestTU::withCode(Main.code());
+  TU.ClangTidyChecks = "bugprone-integer-division";
+  EXPECT_THAT(TU.build().getDiagnostics(),
+  UnorderedElementsAre(Diag(
+  Main.range(), "result of integer division used in a floating "
+"point context; possible loss of precision")));
+}
 } // namespace
 
 } // namespace clangd
Index: clang-tools-extra/clangd/Diagnostics.h
===
--- clang-tools-extra/clangd/Diagnostics.h
+++ clang-tools-extra/clangd/Diagnostics.h
@@ -112,6 +112,9 @@
 /// Convert from clang diagnostic level to LSP severity.
 int getSeverity(DiagnosticsEngine::Level L);
 
+/// Check if a diagnostic is inside the main file.
+bool isInsideMainFile(const clang::Diagnostic );
+
 /// StoreDiags collects the diagnostics that can later be reported by
 /// clangd. It groups all notes for a diagnostic into a single Diag
 /// and filters out diagnostics that don't mention the main file (i.e. neither
@@ -128,17 +131,25 @@
 
   using DiagFixer = std::function(DiagnosticsEngine::Level,
const clang::Diagnostic &)>;
+  using DiagFilter =
+  std::function;
   /// If set, possibly adds fixes for diagnostics using \p Fixer.
   void contributeFixes(DiagFixer Fixer) { this->Fixer = Fixer; }
+  /// If set, ignore diagnostics for which \p SuppressionFilter returns true.
+  void suppressDiagnostics(DiagFilter SuppressionFilter) {
+this->SuppressionFilter = SuppressionFilter;
+  }
 
 private:
   void flushLastDiag();
 
   DiagFixer Fixer = nullptr;
+  DiagFilter SuppressionFilter = nullptr;
   std::vector Output;
   llvm::Optional LangOpts;
   llvm::Optional LastDiag;
   llvm::DenseSet IncludeLinesWithErrors;
+  bool LastPrimaryDiagnosticWasSuppressed = false;
 };
 
 } // namespace clangd
Index: clang-tools-extra/clangd/Diagnostics.cpp
===
--- clang-tools-extra/clangd/Diagnostics.cpp
+++ clang-tools-extra/clangd/Diagnostics.cpp
@@ -146,6 +146,8 @@
   return Loc.isValid() && M.isWrittenInMainFile(M.getFileLoc(Loc));
 }
 
+} // namespace
+
 bool isInsideMainFile(const clang::Diagnostic ) {
   if (!D.hasSourceManager())
 return false;
@@ -153,6 +155,8 @@
   return isInsideMainFile(D.getLocation(), D.getSourceManager());
 }
 
+namespace {
+
 bool isNote(DiagnosticsEngine::Level L) {
   return L == DiagnosticsEngine::Note || L == DiagnosticsEngine::Remark;
 }
@@ -514,6 +518,12 @@
 // Handle the new main diagnostic.
 flushLastDiag();
 
+if (SuppressionFilter && SuppressionFilter(DiagLevel, Info)) {
+  LastPrimaryDiagnosticWasSuppressed = true;
+  return;
+}
+LastPrimaryDiagnosticWasSuppressed = false;
+
 LastDiag = Diag();
 LastDiag->ID = Info.getID();
 FillDiagBase(*LastDiag);
@@ -528,6 +538,13 @@
 }
   } else {
 // Handle a note to an existing diagnostic.
+
+// If a diagnostic was suppressed due to the suppression filter,
+// also suppress notes associated with it.
+if (LastPrimaryDiagnosticWasSuppressed) {
+  return;
+}
+
 if (!LastDiag) {
   assert(false && "Adding a note without main diagnostic");
   IgnoreDiagnostics::log(DiagLevel, Info);
Index: clang-tools-extra/clangd/ClangdUnit.cpp
===
--- clang-tools-extra/clangd/ClangdUnit.cpp
+++ clang-tools-extra/clangd/ClangdUnit.cpp
@@ -113,12 +113,12 @@
   }
 
   void EndOfMainFile() {
-for (const auto& Entry : MainFileMacros)
+for (const auto  : MainFileMacros)
   

[PATCH] D60953: [clangd] Respect clang-tidy suppression comments

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

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`




Comment at: clang-tools-extra/clangd/ClangdUnit.cpp:243
+// future).
+class ClangdDiagnosticConsumer : public StoreDiags {
+public:

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?



Comment at: clang-tools-extra/clangd/ClangdUnit.cpp:338
   Check->registerMatchers();
 }
   }

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



Comment at: clang-tools-extra/clangd/Diagnostics.cpp:309
 
   if (!LangOpts || !Info.hasSourceManager()) {
 IgnoreDiagnostics::log(DiagLevel, Info);

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...



Comment at: clang-tools-extra/clangd/Diagnostics.cpp:314
 
+  if (LastErrorWasIgnored && DiagLevel == DiagnosticsEngine::Note)
+return;

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.



Comment at: clang-tools-extra/clangd/Diagnostics.cpp:319
 
+  if (Filter && !Filter(DiagLevel, Info, InsideMainFile)) {
+LastErrorWasIgnored = true;

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.

e.g.

```
if (!isNote(...)) {
  flushLastDiag();
  LastErrorWasIgnored = !Filter(...);
  if (LastErrorWasIgnored)
return;
  ...
} else {
  // Handle a note...
  if (LastErrorWasIgnored)
return;
  ...
}



Comment at: clang-tools-extra/clangd/Diagnostics.h:118
+  std::function;
   /// If set, possibly adds fixes for diagnostics using \p Fixer.

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.



Comment at: clang-tools-extra/clangd/Diagnostics.h:122
+  /// If set, ignore diagnostics for which \p Filter returns false.
+  void filterDiagnostics(DiagFilter Filter) { this->Filter = Filter; }
 

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.



Comment at: clang-tools-extra/clangd/Diagnostics.h:132
   llvm::Optional LastDiag;
+  bool LastErrorWasIgnored = false;
 };

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

At the risk of being verbose, maybe `LastPrimaryDiagnosticWasIgnored`?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60953/new/

https://reviews.llvm.org/D60953



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60953: [clangd] Respect clang-tidy suppression comments

[Nathan Ridge via Phabricator via cfe-commits]

nridge added a comment.

Friendly review ping.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60953/new/

https://reviews.llvm.org/D60953



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60953: [clangd] Respect clang-tidy suppression comments

[Nathan Ridge via Phabricator via cfe-commits]

nridge updated this revision to Diff 197040.
nridge marked 3 inline comments as done.
nridge added a comment.

Address review comments


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60953/new/

https://reviews.llvm.org/D60953

Files:
  clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
  clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h
  clang-tools-extra/clangd/ClangdUnit.cpp
  clang-tools-extra/clangd/Diagnostics.cpp
  clang-tools-extra/clangd/Diagnostics.h

Index: clang-tools-extra/clangd/Diagnostics.h
===
--- clang-tools-extra/clangd/Diagnostics.h
+++ clang-tools-extra/clangd/Diagnostics.h
@@ -113,16 +113,23 @@
 
   using DiagFixer = std::function(DiagnosticsEngine::Level,
const clang::Diagnostic &)>;
+  using DiagFilter =
+  std::function;
   /// If set, possibly adds fixes for diagnostics using \p Fixer.
   void contributeFixes(DiagFixer Fixer) { this->Fixer = Fixer; }
+  /// If set, ignore diagnostics for which \p Filter returns false.
+  void filterDiagnostics(DiagFilter Filter) { this->Filter = Filter; }
 
 private:
   void flushLastDiag();
 
   DiagFixer Fixer = nullptr;
+  DiagFilter Filter = nullptr;
   std::vector Output;
   llvm::Optional LangOpts;
   llvm::Optional LastDiag;
+  bool LastErrorWasIgnored = false;
 };
 
 } // namespace clangd
Index: clang-tools-extra/clangd/Diagnostics.cpp
===
--- clang-tools-extra/clangd/Diagnostics.cpp
+++ clang-tools-extra/clangd/Diagnostics.cpp
@@ -311,8 +311,17 @@
 return;
   }
 
+  if (LastErrorWasIgnored && DiagLevel == DiagnosticsEngine::Note)
+return;
+
   bool InsideMainFile = isInsideMainFile(Info);
 
+  if (Filter && !Filter(DiagLevel, Info, InsideMainFile)) {
+LastErrorWasIgnored = true;
+return;
+  }
+  LastErrorWasIgnored = false;
+
   auto FillDiagBase = [&](DiagBase ) {
 D.Range = diagnosticRange(Info, *LangOpts);
 llvm::SmallString<64> Message;
Index: clang-tools-extra/clangd/ClangdUnit.cpp
===
--- clang-tools-extra/clangd/ClangdUnit.cpp
+++ clang-tools-extra/clangd/ClangdUnit.cpp
@@ -237,6 +237,40 @@
   const LangOptions 
 };
 
+// A wrapper around StoreDiags to handle suppression comments for
+// clang-tidy diagnostics (and possibly other clang-tidy customizations in the
+// future).
+class ClangdDiagnosticConsumer : public StoreDiags {
+public:
+  ClangdDiagnosticConsumer() {
+filterDiagnostics([this](DiagnosticsEngine::Level DiagLevel,
+ const clang::Diagnostic ,
+ bool IsInsideMainFile) {
+  if (CTContext) {
+bool isClangTidyDiag = !CTContext->getCheckName(Info.getID()).empty();
+// Skip the ShouldSuppressDiagnostic check for diagnostics not in
+// the main file, because we don't want that function to query the
+// source buffer for preamble files. For the same reason, we ask
+// ShouldSuppressDiagnostic not to follow macro expansions, since those
+// might take us into a preamble file as well.
+if (isClangTidyDiag && IsInsideMainFile &&
+tidy::ShouldSuppressDiagnostic(DiagLevel, Info, *CTContext,
+   /* CheckMacroExpansion = */ false)) {
+  return false;
+}
+  }
+  return true;
+});
+  }
+
+  void setClangTidyContext(tidy::ClangTidyContext *CTContext) {
+this->CTContext = CTContext;
+  }
+
+private:
+  tidy::ClangTidyContext *CTContext = nullptr;
+};
+
 } // namespace
 
 void dumpAST(ParsedAST , llvm::raw_ostream ) {
@@ -256,7 +290,7 @@
   const PrecompiledPreamble *PreamblePCH =
   Preamble ? >Preamble : nullptr;
 
-  StoreDiags ASTDiags;
+  ClangdDiagnosticConsumer ASTDiags;
   std::string Content = Buffer->getBuffer();
 
   auto Clang = prepareCompilerInstance(std::move(CI), PreamblePCH,
@@ -294,6 +328,7 @@
 CTContext->setASTContext(>getASTContext());
 CTContext->setCurrentFile(MainInput.getFile());
 CTFactories.createChecks(CTContext.getPointer(), CTChecks);
+ASTDiags.setClangTidyContext(CTContext.getPointer());
 Preprocessor *PP = >getPreprocessor();
 for (const auto  : CTChecks) {
   // FIXME: the PP callbacks skip the entire preamble.
Index: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h
===
--- clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h
+++ clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h
@@ -217,6 +217,23 @@
   bool AllowEnablingAnalyzerAlphaCheckers;
 };
 
+/// Check whether a given diagnostic should be suppressed due to the presence
+/// of a "NOLINT" suppression comment.
+/// This is exposed so that other tools that present clang-tidy diagnostics
+/// (such as 

[PATCH] D60953: [clangd] Respect clang-tidy suppression comments

[Nathan Ridge via Phabricator via cfe-commits]

nridge added inline comments.



Comment at: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp:395
+ DiagLevel != DiagnosticsEngine::Fatal &&
+ LineIsMarkedWithNOLINTinMacro(Info.getSourceManager(),
+   Info.getLocation(), Info.getID(),

sammccall wrote:
> 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.
Thanks for pointing this out! This should be addressed in the updated patch.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60953/new/

https://reviews.llvm.org/D60953



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60953: [clangd] Respect clang-tidy suppression comments

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Thanks for doing this!




Comment at: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp:395
+ DiagLevel != DiagnosticsEngine::Fatal &&
+ LineIsMarkedWithNOLINTinMacro(Info.getSourceManager(),
+   Info.getLocation(), Info.getID(),

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.



Comment at: clang-tools-extra/clangd/ClangdUnit.cpp:254
+  tidy::ShouldSuppressDiagnostic(DiagLevel, Info, *CTContext)) {
+// Ignored a warning, should ignore related notes as well
+LastErrorWasIgnored = true;

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)` 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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60953/new/

https://reviews.llvm.org/D60953



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60953: [clangd] Respect clang-tidy suppression comments

[Nathan Ridge via Phabricator via cfe-commits]

nridge edited reviewers, added: ilya-biryukov; removed: hokein.
nridge added a comment.

Changing reviewers as I understand @hokein is on vacation.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60953/new/

https://reviews.llvm.org/D60953



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60953: [clangd] Respect clang-tidy suppression comments

[Nathan Ridge via Phabricator via cfe-commits]

nridge created this revision.
nridge added a reviewer: hokein.
Herald added subscribers: cfe-commits, kadircet, arphaman, jkorous, MaskRay, 
ioeric.
Herald added a project: clang.

This partially fixes https://bugs.llvm.org/show_bug.cgi?id=41218.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D60953

Files:
  clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
  clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h
  clang-tools-extra/clangd/ClangdUnit.cpp

Index: clang-tools-extra/clangd/ClangdUnit.cpp
===
--- clang-tools-extra/clangd/ClangdUnit.cpp
+++ clang-tools-extra/clangd/ClangdUnit.cpp
@@ -237,6 +237,39 @@
   const LangOptions 
 };
 
+// A wrapper around StoreDiags to handle suppression comments for
+// clang-tidy diagnostics (and possibly other clang-tidy customizations in the
+// future).
+class ClangdDiagnosticConsumer : public StoreDiags {
+public:
+  void HandleDiagnostic(DiagnosticsEngine::Level DiagLevel,
+const clang::Diagnostic ) override {
+if (LastErrorWasIgnored && DiagLevel == DiagnosticsEngine::Note)
+  return;
+
+if (CTContext) {
+  bool isClangTidyDiag = !CTContext->getCheckName(Info.getID()).empty();
+  if (isClangTidyDiag &&
+  tidy::ShouldSuppressDiagnostic(DiagLevel, Info, *CTContext)) {
+// Ignored a warning, should ignore related notes as well
+LastErrorWasIgnored = true;
+return;
+  }
+}
+
+LastErrorWasIgnored = false;
+StoreDiags::HandleDiagnostic(DiagLevel, Info);
+  }
+
+  void setClangTidyContext(tidy::ClangTidyContext *CTContext) {
+this->CTContext = CTContext;
+  }
+
+private:
+  bool LastErrorWasIgnored = false;
+  tidy::ClangTidyContext *CTContext = nullptr;
+};
+
 } // namespace
 
 void dumpAST(ParsedAST , llvm::raw_ostream ) {
@@ -256,7 +289,7 @@
   const PrecompiledPreamble *PreamblePCH =
   Preamble ? >Preamble : nullptr;
 
-  StoreDiags ASTDiags;
+  ClangdDiagnosticConsumer ASTDiags;
   std::string Content = Buffer->getBuffer();
 
   auto Clang = prepareCompilerInstance(std::move(CI), PreamblePCH,
@@ -294,6 +327,7 @@
 CTContext->setASTContext(>getASTContext());
 CTContext->setCurrentFile(MainInput.getFile());
 CTFactories.createChecks(CTContext.getPointer(), CTChecks);
+ASTDiags.setClangTidyContext(CTContext.getPointer());
 Preprocessor *PP = >getPreprocessor();
 for (const auto  : CTChecks) {
   // FIXME: the PP callbacks skip the entire preamble.
Index: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h
===
--- clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h
+++ clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h
@@ -217,6 +217,17 @@
   bool AllowEnablingAnalyzerAlphaCheckers;
 };
 
+/// Check whether a given diagnostic should be suppressed due to the presence
+/// of a "NOLINT" suppression comment.
+/// This is exposed so that other tools that present clang-tidy diagnostics
+/// (such as clangd) can respect the same suppression rules as clang-tidy.
+/// This does not handle suppression of notes following a suppressed diagnostic;
+/// that is left to the caller is it requires maintaining state in between calls
+/// to this function.
+bool ShouldSuppressDiagnostic(DiagnosticsEngine::Level DiagLevel,
+  const Diagnostic ,
+  ClangTidyContext );
+
 /// \brief A diagnostic consumer that turns each \c Diagnostic into a
 /// \c SourceManager-independent \c ClangTidyError.
 //
@@ -246,7 +257,7 @@
 
   /// \brief Updates \c LastErrorRelatesToUserCode and LastErrorPassesLineFilter
   /// according to the diagnostic \p Location.
-  void checkFilters(SourceLocation Location, const SourceManager& Sources);
+  void checkFilters(SourceLocation Location, const SourceManager );
   bool passesLineFilter(StringRef FileName, unsigned LineNumber) const;
 
   ClangTidyContext 
Index: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
===
--- clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
+++ clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
@@ -160,11 +160,13 @@
 
   bool contains(StringRef S) {
 switch (auto  = Cache[S]) {
-  case Yes: return true;
-  case No: return false;
-  case None:
-Result = Globs.contains(S) ? Yes : No;
-return Result == Yes;
+case Yes:
+  return true;
+case No:
+  return false;
+case None:
+  Result = Globs.contains(S) ? Yes : No;
+  return Result == Yes;
 }
 llvm_unreachable("invalid enum");
   }
@@ -381,16 +383,29 @@
   return false;
 }
 
+namespace clang {
+namespace tidy {
+
+bool ShouldSuppressDiagnostic(DiagnosticsEngine::Level DiagLevel,
+  const Diagnostic ,
+