[libTooling] Improve error message from failure in selection Stencil

[ymandel]

Summary

This patch improves the error message provided by the stencil that handles
source from a range selector.

[gribozavr2]

Any chance for a test?

clang/lib/Tooling/Transformer/Stencil.cpp

244

must be carry => must carry

[ymandel]

In D82654#2116906, @gribozavr2 wrote:

Any chance for a test?

StencilTest.CatOfInvalidRangeFails essentially covers this (indeed, earlier versions of the code failed this test).
https://github.com/llvm/llvm-project/blob/master/clang/unittests/Tooling/StencilTest.cpp#L419
I can make it more specific, but since this only affects the error message and not the control flow, I didn't update the test. Let me know if you disagree -- I generally avoid testing error message content in tests, but I know there's a variety of opinions on this subject...

[gribozavr2]

I generally avoid testing error message content in tests, but I know there's a variety of opinions on this subject...

If you thought that the quality of the error message matters enough to improve it, then it is worth testing the message, I think.

clang/lib/Tooling/Transformer/Stencil.cpp

 #include "clang/ASTMatchers/ASTMatchers.h"
 #include "clang/Basic/SourceLocation.h"
 #include "clang/Lex/Lexer.h"
 #include "clang/Tooling/Transformer/SourceCode.h"
 #include "clang/Tooling/Transformer/SourceCodeBuilders.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/Twine.h"
 #include "llvm/Support/Errc.h"
+#include "llvm/Support/Error.h"
 #include <atomic>
 #include <memory>
 #include <string>
 
 using namespace clang;
 using namespace transformer;
 
 using ast_matchers::MatchFinder;

 
 Error evalData(const SelectorData &Data, const MatchFinder::MatchResult &Match,
                std::string *Result) {
   auto RawRange = Data.Selector(Match);
   if (!RawRange)
     return RawRange.takeError();
   CharSourceRange Range = Lexer::makeFileCharRange(
       *RawRange, *Match.SourceManager, Match.Context->getLangOpts());
+  if (Range.isInvalid()) {
+    // Validate the original range to attempt to get a meaningful error message.
+    // If it's valid, then something else is the cause and we just return the
+    // generic failure message.
+    if (auto Err = tooling::validateEditRange(*RawRange, *Match.SourceManager))
+      return handleErrors(std::move(Err), [](std::unique_ptr<StringError> E) {
+        assert(E->convertToErrorCode() ==
+                   llvm::make_error_code(errc::invalid_argument) &&
+               "Validation errors must carry the invalid_argument code");
+        return llvm::createStringError(
+            errc::invalid_argument,
+            "selected range could not be resolved to a valid source range; " +
+                E->getMessage());
+      });
+    return llvm::createStringError(
+        errc::invalid_argument,
+        "selected range could not be resolved to a valid source range");
+  }
+  // Validate `Range`, because `makeFileCharRange` accepts some ranges that
+  // `validateEditRange` rejects.
   if (auto Err = tooling::validateEditRange(Range, *Match.SourceManager))
-    return Err;
+    return joinErrors(
+        llvm::createStringError(errc::invalid_argument,
+                                "selected range is not valid for editing"),
+        std::move(Err));
   *Result += tooling::getText(Range, *Match.Context);
   return Error::success();
 }
 
 Error evalData(const AccessData &Data, const MatchFinder::MatchResult &Match,
                std::string *Result) {
   const auto *E = Match.Nodes.getNodeAs<Expr>(Data.BaseId);
   if (E == nullptr)

clang/unittests/Tooling/StencilTest.cpp

#define MACRO (3.77)
   foo(MACRO);)cpp";
 
   auto StmtMatch =
       matchStmt(Snippet, callExpr(callee(functionDecl(hasName("foo"))),
                                   argumentCountIs(1),
                                   hasArgument(0, expr().bind("arg"))));
   ASSERT_TRUE(StmtMatch);
   Stencil S = cat(node("arg"));
-  EXPECT_THAT_EXPECTED(S->eval(StmtMatch->Result), Failed<StringError>());
+  Expected<std::string> Result = S->eval(StmtMatch->Result);
+  ASSERT_THAT_EXPECTED(Result, Failed<StringError>());
+  llvm::handleAllErrors(Result.takeError(), [](const llvm::StringError &E) {
+    EXPECT_THAT(E.getMessage(), AllOf(HasSubstr("selected range"),
+                                      HasSubstr("macro expansion")));
+  });
 }
 
 TEST(StencilToStringTest, RawTextOp) {
   auto S = cat("foo bar baz");
   StringRef Expected = R"("foo bar baz")";
   EXPECT_EQ(S->toString(), Expected);
 }