Skip to content

Add error check for HeuristicResolver #155561

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Sep 2, 2025
Merged

Add error check for HeuristicResolver #155561

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
b0c182f
Add `CaptureDiagsKind` to `buildASTFromCodeWithArgs`
MythreyaK Aug 28, 2025
b1a30c8
Add error check in `HeuristicResolverTest.cpp`
MythreyaK Aug 28, 2025
d4ceb23
code review
MythreyaK Aug 30, 2025
1a97921
code review
MythreyaK Aug 30, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions clang/include/clang/Tooling/Tooling.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@
#include "clang/AST/ASTConsumer.h"
#include "clang/Basic/FileManager.h"
#include "clang/Basic/LLVM.h"
#include "clang/Frontend/ASTUnit.h"
#include "clang/Frontend/FrontendAction.h"
#include "clang/Frontend/PCHContainerOperations.h"
#include "clang/Tooling/ArgumentsAdjusters.h"
Expand Down Expand Up @@ -238,6 +239,7 @@ std::unique_ptr<ASTUnit> buildASTFromCodeWithArgs(
ArgumentsAdjuster Adjuster = getClangStripDependencyFileAdjuster(),
const FileContentMappings &VirtualMappedFiles = FileContentMappings(),
DiagnosticConsumer *DiagConsumer = nullptr,
CaptureDiagsKind CaptureKind = CaptureDiagsKind::None,
Copy link
Collaborator

@HighCommander4 HighCommander4 Aug 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@MythreyaK MythreyaK Aug 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense, I'll do that

IntrusiveRefCntPtr<llvm::vfs::FileSystem> BaseFS =
llvm::vfs::getRealFileSystem());

Expand Down
9 changes: 7 additions & 2 deletions clang/lib/Tooling/Tooling.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -658,13 +658,15 @@ class ASTBuilderAction : public ToolAction {
Invocation->getDiagnosticOpts(),
DiagConsumer,
/*ShouldOwnClient=*/false),
Files);
Files, false, CaptureKinds);
if (!AST)
return false;

ASTs.push_back(std::move(AST));
return true;
}

CaptureDiagsKind CaptureKinds{CaptureDiagsKind::None};
Copy link
Collaborator

@HighCommander4 HighCommander4 Aug 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

};

} // namespace
Expand Down Expand Up @@ -692,10 +694,13 @@ std::unique_ptr<ASTUnit> buildASTFromCodeWithArgs(
StringRef Code, const std::vector<std::string> &Args, StringRef FileName,
StringRef ToolName, std::shared_ptr<PCHContainerOperations> PCHContainerOps,
ArgumentsAdjuster Adjuster, const FileContentMappings &VirtualMappedFiles,
DiagnosticConsumer *DiagConsumer,
DiagnosticConsumer *DiagConsumer, CaptureDiagsKind CaptureKind,
IntrusiveRefCntPtr<llvm::vfs::FileSystem> BaseFS) {
std::vector<std::unique_ptr<ASTUnit>> ASTs;

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

@HighCommander4 HighCommander4 Aug 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@MythreyaK MythreyaK Aug 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

whoops, my bad, thanks!


auto OverlayFileSystem =
llvm::makeIntrusiveRefCnt<llvm::vfs::OverlayFileSystem>(
std::move(BaseFS));
Expand Down
29 changes: 29 additions & 0 deletions clang/unittests/Sema/HeuristicResolverTest.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,9 @@
#include "clang/Sema/HeuristicResolver.h"
#include "clang/ASTMatchers/ASTMatchFinder.h"
#include "clang/ASTMatchers/ASTMatchers.h"
#include "clang/Basic/Diagnostic.h"
#include "clang/Tooling/Tooling.h"
#include "llvm/ADT/SmallSet.h"
#include "gmock/gmock-matchers.h"
#include "gtest/gtest.h"

Expand All @@ -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)
Msg << "Ignored: ";
if (D->getLevel() == DiagnosticsEngine::Level::Note)
Msg << "Note: ";
if (D->getLevel() == DiagnosticsEngine::Level::Remark)
Msg << "Remark: ";
if (D->getLevel() == DiagnosticsEngine::Level::Warning)
Msg << "Warning: ";
if (D->getLevel() == DiagnosticsEngine::Level::Error)
Msg << "Error: ";
if (D->getLevel() == DiagnosticsEngine::Level::Fatal)
Msg << "Fatal: ";
Msg << D->getID() << ": " << D->getMessage().str();
return Msg.str();
}

// 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 *>`.
Expand All @@ -41,7 +61,16 @@ template <typename InputNode, typename ParamT, typename InputMatcher,
typename... OutputMatchers>
void expectResolution(llvm::StringRef Code, ResolveFnT<ParamT> ResolveFn,
const InputMatcher &IM, const OutputMatchers &...OMS) {
llvm::SmallSet<unsigned int, 16> IgnoredDiagnostics{};
Copy link
Contributor Author

@MythreyaK MythreyaK Aug 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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) {

Copy link
Collaborator

@HighCommander4 HighCommander4 Aug 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"});
Copy link
Collaborator

@HighCommander4 HighCommander4 Aug 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@MythreyaK MythreyaK Aug 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

omg totally forgot about this, sorry!


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

@HighCommander4 HighCommander4 Aug 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@MythreyaK MythreyaK Aug 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was wondering if that was okay lol, I'll go ahead and do that as well.

D != DEnd; ++D) {
EXPECT_TRUE(D->getLevel() < DiagnosticsEngine::Warning ||
Copy link
Collaborator

@HighCommander4 HighCommander4 Aug 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@MythreyaK MythreyaK Aug 30, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!)?

IgnoredDiagnostics.contains(D->getID()))
<< format_error(D);
}

auto &Ctx = TU->getASTContext();
auto InputMatches = match(IM, Ctx);
ASSERT_EQ(1u, InputMatches.size());
Expand Down
2 changes: 1 addition & 1 deletion clang/unittests/Tooling/ToolingTest.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,7 +161,7 @@ TEST(buildASTFromCode, FileSystem) {
R"(#include "included_file.h")", {}, "input.cc", "clang-tool",
std::make_shared<PCHContainerOperations>(),
getClangStripDependencyFileAdjuster(), FileContentMappings(), nullptr,
InMemoryFileSystem);
CaptureDiagsKind::None, InMemoryFileSystem);
ASSERT_TRUE(AST.get());
EXPECT_TRUE(FindClassDeclX(AST.get()));
}
Expand Down
Loading