Add error check for HeuristicResolver #155561
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@llvm/pr-subscribers-clang Author: Mythreya Kuricheti (MythreyaK) ChangesFixes #155545. Full diff: https://github.com/llvm/llvm-project/pull/155561.diff 1 Files Affected:
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>
|
7304a9a to
e2e72a4
Compare
MythreyaK
commented
Aug 27, 2025
|
|
||
| auto Diags = | ||
| CompilerInstance::createDiagnostics(*VFS, DiagOpts, &DiagConsumer, false); | ||
| EXPECT_TRUE(DiagConsumer.GeneratedDiagnostics.size() == 0); |
There was a problem hiding this comment.
Could probably use a better check, perhaps one that writes out the errors?
There was a problem hiding this comment.
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?
|
Oh sounds good, I'll give that a shot! |
110b460 to
b1a30c8
Compare
|
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
commented
Aug 28, 2025
Comment on lines
62
to
64
| void expectResolution(llvm::StringRef Code, ResolveFnT<ParamT> ResolveFn, | ||
| const InputMatcher &IM, const OutputMatchers &...OMS) { | ||
| llvm::SmallSet<unsigned int, 16> IgnoredDiagnostics{}; |
There was a problem hiding this comment.
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) { |
There was a problem hiding this comment.
I would hold off on adding something like IgnoreDiagnostics until we actually have a use case for it
| @@ -238,6 +239,7 @@ std::unique_ptr<ASTUnit> buildASTFromCodeWithArgs( | |||
| ArgumentsAdjuster Adjuster = getClangStripDependencyFileAdjuster(), | |||
| const FileContentMappings &VirtualMappedFiles = FileContentMappings(), | |||
| DiagnosticConsumer *DiagConsumer = nullptr, | |||
| CaptureDiagsKind CaptureKind = CaptureDiagsKind::None, | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Makes sense, I'll do that
clang/lib/Tooling/Tooling.cpp
Outdated
| if (!AST) | ||
| return false; | ||
|
|
||
| ASTs.push_back(std::move(AST)); | ||
| return true; | ||
| } | ||
|
|
||
| CaptureDiagsKind CaptureKinds{CaptureDiagsKind::None}; |
There was a problem hiding this comment.
I would make this a private field set via a (possibly optional) constructor parameter
clang/lib/Tooling/Tooling.cpp
Outdated
| ASTBuilderAction Action(ASTs); | ||
| Action.CaptureKinds = CaptureDiagsKind::All; |
There was a problem hiding this comment.
I think you mean to be propagating the CaptureKind parameter, rather than harcoding CaptureDiagsKind::All here
There was a problem hiding this comment.
whoops, my bad, thanks!
Comment on lines
62
to
64
| void expectResolution(llvm::StringRef Code, ResolveFnT<ParamT> ResolveFn, | ||
| const InputMatcher &IM, const OutputMatchers &...OMS) { | ||
| llvm::SmallSet<unsigned int, 16> IgnoredDiagnostics{}; |
There was a problem hiding this comment.
I would hold off on adding something like IgnoreDiagnostics until we actually have a use case for it
| auto TU = tooling::buildASTFromCodeWithArgs(Code, {"-std=c++23"}); | ||
|
|
||
| for (auto D = TU->stored_diag_begin(), DEnd = TU->stored_diag_end(); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
I was wondering if that was okay lol, I'll go ahead and do that as well.
|
|
||
| for (auto D = TU->stored_diag_begin(), DEnd = TU->stored_diag_end(); | ||
| D != DEnd; ++D) { | ||
| EXPECT_TRUE(D->getLevel() < DiagnosticsEngine::Warning || |
There was a problem hiding this comment.
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.)
There was a problem hiding this comment.
Are you intentionally disallowing warnings by default?
Yeah, I thought this was helpful when language conveniences are enabled without explicit opt-in. I wasn't sure what the norm was; I'll update it to < Error, which is the norm (thanks for the heads-up!)?
HighCommander4
requested changes
Aug 30, 2025
clang/lib/Tooling/Tooling.cpp
Outdated
| @@ -644,9 +644,13 @@ namespace { | |||
|
|
|||
| class ASTBuilderAction : public ToolAction { | |||
| std::vector<std::unique_ptr<ASTUnit>> &ASTs; | |||
| CaptureDiagsKind CaptureKinds; | |||
There was a problem hiding this comment.
nit: CaptureKind or CaptureDiags (Kinds, as in "more than one kind", does not make sense here)
| @@ -42,6 +62,11 @@ template <typename InputNode, typename ParamT, typename InputMatcher, | |||
| void expectResolution(llvm::StringRef Code, ResolveFnT<ParamT> ResolveFn, | |||
| const InputMatcher &IM, const OutputMatchers &...OMS) { | |||
| auto TU = tooling::buildASTFromCodeWithArgs(Code, {"-std=c++23"}); | |||
There was a problem hiding this comment.
We need to be actually passing CaptureDiagsKind::All here
(A good way to validate that the mechanism we're adding is actually working is to change the -std=c++23 to -std=c++20 here. That should cause MemberExpr_ExplicitObjectParameter to fail, and it currently doesn't.)
There was a problem hiding this comment.
omg totally forgot about this, sorry!
| @@ -31,6 +33,24 @@ template <typename InputNode> | |||
| using ResolveFnT = std::function<std::vector<const NamedDecl *>( | |||
| const HeuristicResolver *, InputNode)>; | |||
|
|
|||
| std::string format_error(const clang::StoredDiagnostic &D) { | |||
| std::ostringstream Msg{}; | |||
| if (D.getLevel() == DiagnosticsEngine::Level::Ignored) | |||
There was a problem hiding this comment.
We don't actually need the first four cases, and I'm not sure the distinction between the remaining two (Error vs. Fatal) is that important; we could just skip printing the level?
On the other hand, we could print a short message prefix that provides some context, such as "Unexpected error diagnostic while building AST for test code:".
HighCommander4
approved these changes
Aug 30, 2025
|
Thanks @MythreyaK for the fix! |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/55/builds/16532 Here is the relevant piece of the build log for the reference |
|
Glad to help, Thank you for the review! |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/94/builds/10425 Here is the relevant piece of the build log for the reference |
lakshayk-nv
pushed a commit
to lakshayk-nv/llvm-project
that referenced
this pull request
Sep 3, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #155545.
Is probably a bit rough and could use improvements, happy to address any feedback!