Add error check for HeuristicResolver #155561

MythreyaK · 2025-08-27T06:20:07Z

Fixes #155545.

Is probably a bit rough and could use improvements, happy to address any feedback!

llvmbot · 2025-08-27T06:20:35Z

@llvm/pr-subscribers-clang

Author: Mythreya Kuricheti (MythreyaK)

Changes

Fixes #155545.

Full diff: https://github.com/llvm/llvm-project/pull/155561.diff

1 Files Affected:

(modified) clang/unittests/Sema/HeuristicResolverTest.cpp (+73-2)

diff --git a/clang/unittests/Sema/HeuristicResolverTest.cpp b/clang/unittests/Sema/HeuristicResolverTest.cpp
index 7df25e01e66d4..7264c017dfaf9 100644
--- a/clang/unittests/Sema/HeuristicResolverTest.cpp
+++ b/clang/unittests/Sema/HeuristicResolverTest.cpp
@@ -8,7 +8,12 @@
 #include "clang/Sema/HeuristicResolver.h"
 #include "clang/ASTMatchers/ASTMatchFinder.h"
 #include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/Basic/Diagnostic.h"
+#include "clang/Basic/DiagnosticIDs.h"
 #include "clang/Tooling/Tooling.h"
+#include "llvm/ADT/SmallSet.h"
+#include "llvm/ADT/SmallString.h"
+#include "llvm/ADT/SmallVector.h"
 #include "gmock/gmock-matchers.h"
 #include "gtest/gtest.h"
 
@@ -31,6 +36,69 @@ template <typename InputNode>
 using ResolveFnT = std::function<std::vector<const NamedDecl *>(
     const HeuristicResolver *, InputNode)>;
 
+struct DiagsConsumer : DiagnosticConsumer {
+  struct PrettyDiagnostic {
+    unsigned int ID;
+    DiagnosticsEngine::Level DiagLevel;
+    llvm::SmallString<64> Message;
+
+    template <typename Stream>
+    friend Stream &operator<<(Stream &OS, const PrettyDiagnostic &D) {
+      if (D.DiagLevel == DiagnosticsEngine::Level::Ignored)
+        OS << "Ignored: ";
+      if (D.DiagLevel == DiagnosticsEngine::Level::Note)
+        OS << "Note: ";
+      if (D.DiagLevel == DiagnosticsEngine::Level::Remark)
+        OS << "Remark: ";
+      if (D.DiagLevel == DiagnosticsEngine::Level::Warning)
+        OS << "Warning: ";
+      if (D.DiagLevel == DiagnosticsEngine::Level::Error)
+        OS << "Error: ";
+      if (D.DiagLevel == DiagnosticsEngine::Level::Fatal)
+        OS << "Fatal: ";
+      OS << "ID: " << D.ID << " Message: " << D.Message.str().str() << '\n';
+      return OS;
+    }
+  };
+
+  llvm::SmallSet<unsigned int, 16> IgnoredDiagnostics{};
+  llvm::SmallVector<PrettyDiagnostic, 16> GeneratedDiagnostics{};
+
+  void HandleDiagnostic(DiagnosticsEngine::Level Level,
+                        const Diagnostic &Info) override {
+    DiagnosticConsumer::HandleDiagnostic(Level, Info);
+    const auto ID = Info.getID();
+
+    if (Level >= DiagnosticsEngine::Level::Warning &&
+        !IgnoredDiagnostics.contains(ID)) {
+      llvm::SmallString<64> Message;
+
+      Info.FormatDiagnostic(Message);
+      GeneratedDiagnostics.emplace_back(PrettyDiagnostic{ID, Level, Message});
+    }
+  }
+};
+
+std::unique_ptr<ASTUnit>
+buildASTUnit(llvm::StringRef Code, std::vector<std::string> Args,
+             llvm::SmallSet<unsigned int, 16> IgnoredDiagnostics) {
+  auto VFS = llvm::vfs::getRealFileSystem();
+  DiagnosticOptions DiagOpts{};
+  DiagsConsumer DiagConsumer{};
+  DiagConsumer.IgnoredDiagnostics = IgnoredDiagnostics;
+
+  auto TU = tooling::buildASTFromCodeWithArgs(
+      Code, Args, "input.cc", "clang-tool",
+      std::make_shared<PCHContainerOperations>(),
+      tooling::getClangStripDependencyFileAdjuster(),
+      tooling::FileContentMappings(), &DiagConsumer, VFS);
+
+  auto Diags =
+      CompilerInstance::createDiagnostics(*VFS, DiagOpts, &DiagConsumer, false);
+  EXPECT_TRUE(DiagConsumer.GeneratedDiagnostics.size() == 0);
+  return TU;
+}
+
 // Test heuristic resolution on `Code` using the resolution procedure
 // `ResolveFn`, which takes a `HeuristicResolver` and an input AST node of type
 // `InputNode` and returns a `std::vector<const NamedDecl *>`.
@@ -41,8 +109,10 @@ template <typename InputNode, typename ParamT, typename InputMatcher,
           typename... OutputMatchers>
 void expectResolution(llvm::StringRef Code, ResolveFnT<ParamT> ResolveFn,
                       const InputMatcher &IM, const OutputMatchers &...OMS) {
-  auto TU = tooling::buildASTFromCodeWithArgs(Code, {"-std=c++20"});
-  auto &Ctx = TU->getASTContext();
+
+  auto AST = buildASTUnit(Code, {"-std=c++20"}, {});
+  auto &Ctx = AST->getASTContext();
+
   auto InputMatches = match(IM, Ctx);
   ASSERT_EQ(1u, InputMatches.size());
   const auto *Input = InputMatches[0].template getNodeAs<InputNode>("input");
@@ -105,6 +175,7 @@ TEST(HeuristicResolver, MemberExpr_Overloads) {
     struct S {
       void bar(int);
       void bar(float);
+      void bar(this S);
     };
 
     template <typename T, typename U>

MythreyaK · 2025-08-27T06:35:45Z

clang/unittests/Sema/HeuristicResolverTest.cpp

+
+  auto Diags =
+      CompilerInstance::createDiagnostics(*VFS, DiagOpts, &DiagConsumer, false);
+  EXPECT_TRUE(DiagConsumer.GeneratedDiagnostics.size() == 0);


Could probably use a better check, perhaps one that writes out the errors?

HighCommander4

Thanks for putting this together!

It's a bit unfortunate to have to do this much plumbing to accomplish a relatively simple goal though.

Poking around a bit, I see that ASTUnit's constructor takes a CaptureDiagsKind parameter which, if not None, makes the diagnostics available on the ASTUnit itself (example access).

However, buildASTFromCodeWithArgs() does not expose this parameter (it passes None unconditionally).

WDYT think about giving buildASTFromCodeWithArgs() an additional (optional) CaptureDiagsKind parameter, plumbing it through to the ASTUnit, and then passing CaptureDiagsKind::All here in the test file?

MythreyaK · 2025-08-28T05:49:30Z

Oh sounds good, I'll give that a shot!

MythreyaK · 2025-08-28T09:43:05Z

You were right lol, all that was completely unnecessary. I should've dug around a bit more 😆.

Rebased the whole branch with new changes.

(investigating test failures ...)

MythreyaK · 2025-08-28T09:51:59Z

clang/unittests/Sema/HeuristicResolverTest.cpp

 void expectResolution(llvm::StringRef Code, ResolveFnT<ParamT> ResolveFn,
                      const InputMatcher &IM, const OutputMatchers &...OMS) {
+  llvm::SmallSet<unsigned int, 16> IgnoredDiagnostics{};


We can't use a trailing default parameter here, so thought of adding an IgnoredDiagnostics to the function.

Suggested change

void expectResolution(llvm::StringRef Code, ResolveFnT<ParamT> ResolveFn,

const InputMatcher &IM, const OutputMatchers &...OMS) {

llvm::SmallSet<unsigned int, 16> IgnoredDiagnostics{};

void expectResolution(llvm::StringRef Code,

llvm::SmallSet<unsigned int, 16> IgnoredDiagnostics,

ResolveFnT<ParamT> ResolveFn, const InputMatcher &IM,

const OutputMatchers &...OMS) {

I would hold off on adding something like IgnoreDiagnostics until we actually have a use case for it

HighCommander4 · 2025-08-29T04:21:49Z

clang/include/clang/Tooling/Tooling.h

@@ -238,6 +239,7 @@ std::unique_ptr<ASTUnit> buildASTFromCodeWithArgs(
    ArgumentsAdjuster Adjuster = getClangStripDependencyFileAdjuster(),
    const FileContentMappings &VirtualMappedFiles = FileContentMappings(),
    DiagnosticConsumer *DiagConsumer = nullptr,
+    CaptureDiagsKind CaptureKind = CaptureDiagsKind::None,


This is a public API, and likely one with a fair number of downstream users.

While clang doesn't make any stability guarantees about it's C++ API, I think it's still good practice to avoid API breaks that are unnecessary, since it takes work for downstreams to rebase across them.

So, in a case like this, where we're adding a new optional argument to an API function, I think we should add it to the end.

HighCommander4 · 2025-08-29T04:23:30Z

clang/lib/Tooling/Tooling.cpp

    if (!AST)
      return false;

    ASTs.push_back(std::move(AST));
    return true;
  }
+
+  CaptureDiagsKind CaptureKinds{CaptureDiagsKind::None};


I would make this a private field set via a (possibly optional) constructor parameter

HighCommander4 · 2025-08-29T04:24:22Z

clang/lib/Tooling/Tooling.cpp

  ASTBuilderAction Action(ASTs);
+  Action.CaptureKinds = CaptureDiagsKind::All;


I think you mean to be propagating the CaptureKind parameter, rather than harcoding CaptureDiagsKind::All here

HighCommander4 · 2025-08-29T04:26:09Z

clang/unittests/Sema/HeuristicResolverTest.cpp

 void expectResolution(llvm::StringRef Code, ResolveFnT<ParamT> ResolveFn,
                      const InputMatcher &IM, const OutputMatchers &...OMS) {
+  llvm::SmallSet<unsigned int, 16> IgnoredDiagnostics{};


I would hold off on adding something like IgnoreDiagnostics until we actually have a use case for it

HighCommander4 · 2025-08-29T04:27:34Z

clang/unittests/Sema/HeuristicResolverTest.cpp

  auto TU = tooling::buildASTFromCodeWithArgs(Code, {"-std=c++23"});
+
+  for (auto D = TU->stored_diag_begin(), DEnd = TU->stored_diag_end();


Only tangentially related, but since we're touching libTooling code anyways (and since it's not 1998 any more 😆), shall we take the opportunity to add a range wrapper similar to this to ASTUnit?

HighCommander4 · 2025-08-29T04:30:06Z

clang/unittests/Sema/HeuristicResolverTest.cpp

+
+  for (auto D = TU->stored_diag_begin(), DEnd = TU->stored_diag_end();
+       D != DEnd; ++D) {
+    EXPECT_TRUE(D->getLevel() < DiagnosticsEngine::Warning ||


Are you intentionally disallowing warnings by default?

(I don't particularly feel strongly either way, but in the clangd test infra the check is level < Error, so I was just curious if you're deliberately being stricter.)

llvmbot added the clang Clang issues not falling into any other category label Aug 27, 2025

MythreyaK force-pushed the mythreyak/heuristicresolver-error-check branch from 7304a9a to e2e72a4 Compare August 27, 2025 06:23

MythreyaK commented Aug 27, 2025

View reviewed changes

MythreyaK mentioned this pull request Aug 28, 2025

[HeuristicResolver] Tests should fail if parsed code has error diagnostics #155545

Open

HighCommander4 reviewed Aug 28, 2025

View reviewed changes

MythreyaK added 2 commits August 28, 2025 01:27

Add CaptureDiagsKind to buildASTFromCodeWithArgs

b0c182f

Add error check in HeuristicResolverTest.cpp

b1a30c8

MythreyaK force-pushed the mythreyak/heuristicresolver-error-check branch from 110b460 to b1a30c8 Compare August 28, 2025 09:42

MythreyaK commented Aug 28, 2025

View reviewed changes

HighCommander4 reviewed Aug 29, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Add error check for HeuristicResolver #155561

Add error check for HeuristicResolver #155561

MythreyaK commented Aug 27, 2025 •

edited

Loading

Uh oh!

llvmbot commented Aug 27, 2025

Uh oh!

MythreyaK Aug 27, 2025

Uh oh!

HighCommander4 left a comment

Uh oh!

MythreyaK commented Aug 28, 2025

Uh oh!

MythreyaK commented Aug 28, 2025 •

edited

Loading

Uh oh!

MythreyaK Aug 28, 2025

Uh oh!

HighCommander4 Aug 29, 2025

Uh oh!

HighCommander4 Aug 29, 2025

Uh oh!

HighCommander4 Aug 29, 2025

Uh oh!

HighCommander4 Aug 29, 2025

Uh oh!

HighCommander4 Aug 29, 2025

Uh oh!

HighCommander4 Aug 29, 2025

Uh oh!

HighCommander4 Aug 29, 2025

Uh oh!

Uh oh!

		ASTBuilderAction Action(ASTs);
		Action.CaptureKinds = CaptureDiagsKind::All;

		auto TU = tooling::buildASTFromCodeWithArgs(Code, {"-std=c++23"});

		for (auto D = TU->stored_diag_begin(), DEnd = TU->stored_diag_end();

Add error check for HeuristicResolver #155561

Are you sure you want to change the base?

Add error check for HeuristicResolver #155561

Conversation

MythreyaK commented Aug 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Aug 27, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HighCommander4 left a comment

Choose a reason for hiding this comment

Uh oh!

MythreyaK commented Aug 28, 2025

Uh oh!

MythreyaK commented Aug 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

MythreyaK commented Aug 27, 2025 •

edited

Loading

MythreyaK commented Aug 28, 2025 •

edited

Loading