[clang][transformer] Finish plumbing `Note` all the way to the output.

[courbet]

Summary

Right now we can only add a single warning, notes are not possible.

Apparently some provisions were made to allow notes (ASTEdit::Note), but they were never
propagated all the way to the diagnostics.

[courbet]

Format patch

[ymandel]

Overall, looks good. But my main question is whether this functionality should be supported in the Edit's Metadata field instead.

Eric -- what do you think? You've thought a lot more about metadata recently.

clang/lib/Tooling/Transformer/RewriteRule.cpp

56–73

Can this ever be null? I assume the intent was "no" (in which case I'd recommend an assert instead), but I'm open to the idea that it's valid, just want to understand better.

[courbet]

clang/lib/Tooling/Transformer/RewriteRule.cpp

56–73

Given that we provide an EditGenerator edit(ASTEdit), we can't ever be sure that the user won't give us an empty replacement.

I've split this off to a separate patch here with a test to show the issue: https://reviews.llvm.org/D128887

[li.zhe.hua]

In D128807#3620573, @ymandel wrote:

Eric -- what do you think? You've thought a lot more about metadata recently.

(Apologies for the delay. Still struggling to figure out a good workflow for reviewing patches...)

Can you say more about what your intended use of the note is? Perhaps that can motivate some of the discussion.

From what I can see, here's my thoughts:

• A note being a part of an edit seems weird at best. An ASTEdit and Edit are fragments of a greater, logical change. That a note should semantically be associated with the insertion of an open paren "(" but not the close paren ")" seems odd to me.
• The note takes the location of the edit it is attached to. Perhaps that could be of some convenience when those coincide, but I don't believe that should necessarily be the case. I'm imagining notes could be used to point out other parts of the source that are interesting.

I don't have a lot of experience with how notes appear when surfaced, but I suspect that this interface might encourage callers to tack notes on without putting a lot of thought to where the note shows up. As is, I'm inclined to think that extending the metadata from std::string to something richer would be a better design.

Let me know if I'm misunderstanding the code or the intent of the patch.

clang/include/clang/Tooling/Transformer/RewriteRule.h

254–255

Are we sure that this is what someone would want? Perhaps I am not creative enough, but this seems like it could explode a large number of notes that coincide to edits, which seems like an odd thing to want.

[ymandel]

In D128807#3622727, @li.zhe.hua wrote:

• A note being a part of an edit seems weird at best. An ASTEdit and Edit are fragments of a greater, logical change. That a note should semantically be associated with the insertion of an open paren "(" but not the close paren ")" seems odd to me.
• The note takes the location of the edit it is attached to. Perhaps that could be of some convenience when those coincide, but I don't believe that should necessarily be the case. I'm imagining notes could be used to point out other parts of the source that are interesting.

Eric, these are great points. Originally, the idea for note (IIRC) came from the observation that sometimes a single match will generate edits in multiple locations, each which deserve a different diagnostic note (perhaps the same text, but appearing at the respective location). The intent was *not* to allow noting the insertion of paren, for example. So, I think it was a mistake. Yes, sometimes an ASTEdit is a coherent change, but sometimes (as the paren example demonstrates) its just a fragment.

So, from my original intent, I think that we'd just want to support multiple notes per rule, with each keyed on a source range. That said, we could decide to use ASTEdit for that purpose, assuming we're ok with ASTEdit with a null Replacement field. But, we'd have to think about th implications before we go down that route.

Clement -- what was your intended application? That may help clarify.

[courbet]

In D128807#3622779, @ymandel wrote:

In D128807#3622727, @li.zhe.hua wrote:

• A note being a part of an edit seems weird at best. An ASTEdit and Edit are fragments of a greater, logical change. That a note should semantically be associated with the insertion of an open paren "(" but not the close paren ")" seems odd to me.
• The note takes the location of the edit it is attached to. Perhaps that could be of some convenience when those coincide, but I don't believe that should necessarily be the case. I'm imagining notes could be used to point out other parts of the source that are interesting.

Eric, these are great points. Originally, the idea for note (IIRC) came from the observation that sometimes a single match will generate edits in multiple locations, each which deserve a different diagnostic note (perhaps the same text, but appearing at the respective location). The intent was *not* to allow noting the insertion of paren, for example. So, I think it was a mistake. Yes, sometimes an ASTEdit is a coherent change, but sometimes (as the paren example demonstrates) its just a fragment.

So, from my original intent, I think that we'd just want to support multiple notes per rule, with each keyed on a source range.

Clement -- what was your intended application? That may help clarify.

Thank for the comments.

For my particular case, I just need multiple notes per rule. I don't need them to be associated to a particular edit (and in that very particular case, I don't even need a source location).
Right now I have a check that is implemented as a transformer check. I want to emit an extra note. However, because transformer do not support notes I will have to rewrite the check to a traditional non-transformer one, which is a large change and could be a one-liner if transformer supported notes.

In terms of the overall design requirements, non-transformer checks can have multiple notes per rule, associated to a source location (and multiple checks are using the note location to point to somewhere else in the code, so that is a required feature if we want to be as powerful as the original clang-tidies, which I think we do). Notes cannot be associated to a particular FixitHint, and I'm not sure whether that's useful.

I went with the design in this patch (notes associated to edits) because:

• it looked like associating a note with an ASTEdit was the original intent, given that ASTEdit already had a Note field.
• we can plumb notes inside Metadata, but Metadata is already used for the warning, so that looks a bit more involved.

No strong opinion on that front though :)

[ymandel]

For my particular case, I just need multiple notes per rule. I don't need them to be associated to a particular edit (and in that very particular case, I don't even need a source location).

I'm not sure I understand: you need this additional note, but it doesn't need to be tied to a source location, so, why can't you concatenate it to the main note?

In terms of the overall design requirements, non-transformer checks can have multiple notes per rule, associated to a source location (and multiple checks are using the note location to point to somewhere else in the code, so that is a required feature if we want to be as powerful as the original clang-tidies, which I think we do).

Yes, that's the goal, essentially. And the Note field came from exactly that observation.

Notes cannot be associated to a particular FixitHint, and I'm not sure whether that's useful.

Right. I'm not sure about that. It seems more intuitive for the note to be bundled with the associated FixitHint in the same DiagnosticBuilder. That is, if we want to (mostly) go with this design, I would think we'd create a separate diagnostic per note or somesuch.

Overall, I'm fine with adding support for additional notes, but I want to get these details sorted out first, since they subtly change the API.

I went with the design in this patch (notes associated to edits) because:

• it looked like associating a note with an ASTEdit was the original intent, given that ASTEdit already had a Note field.
• we can plumb notes inside Metadata, but Metadata is already used for the warning, so that looks a bit more involved.

No strong opinion on that front though :)

[courbet]

In D128807#3625676, @ymandel wrote:

For my particular case, I just need multiple notes per rule. I don't need them to be associated to a particular edit (and in that very particular case, I don't even need a source location).

I'm not sure I understand: you need this additional note, but it doesn't need to be tied to a source location, so, why can't you concatenate it to the main note?

For my specific check the notes carry structure beyond what's typically in a warning, so I need the information to be separate.

[ymandel]

In D128807#3627625, @courbet wrote:

In D128807#3625676, @ymandel wrote:

For my particular case, I just need multiple notes per rule. I don't need them to be associated to a particular edit (and in that very particular case, I don't even need a source location).

I'm not sure I understand: you need this additional note, but it doesn't need to be tied to a source location, so, why can't you concatenate it to the main note?

For my specific check the notes carry structure beyond what's typically in a warning, so I need the information to be separate.

Based on our conversation offline, I'm inclined towards the change with the note below about API. Thanks!

clang/include/clang/Tooling/Transformer/RewriteRule.h

254–255

Agreed. I'm more inclined to only include the ASTEdit version. I see the value of the other one, but seems easy to get wrong. Alternatively, implement it to either only add to the first edit or append/prepend a separate edit with the note.

Either way, I think the primary use should be with a standalone note generator like ASTEdit note(TextGenerator Note). The idea is that the new combinator allows adding notes to a rule and we happen to store notes in ASTEdits. Then, withNote is really the uncommon case where you want to supplement an existing edit or set of edits with a note. WDYT?

259–260

clang/lib/Tooling/Transformer/RewriteRule.cpp

56–73

Agreed. Thanks!

[courbet]

Replace both versions of withNote() with a single ASTEdit note() generator.

[courbet]

Thanks for the comments.

clang/include/clang/Tooling/Transformer/RewriteRule.h

254–255

I'm more inclined to only include the ASTEdit version. I see the value of the other one, but seems easy to get wrong. Alternatively, implement it to either only add to the first edit or append/prepend a separate edit with the note.

The EditGenerator version was so that we can always add a note. Some generators return an EditGenerator instead of an ASTEdit. For example, noopEdit needs access to the match result to generate an empty range . The second version is required to work with these:

withNote(changeTo(anchor1, ...));   // OK, changeTo returns an `ASTEdit`.
withNote(noopEdit(...));   // not OK, noopEdit returns an `EditGenerator`, we need the second version.

Either way, I think the primary use should be with a standalone note generator like ASTEdit note(TextGenerator Note). The idea is that the new combinator allows adding notes to a rule and we happen to store notes in ASTEdits. Then, withNote is really the uncommon case where you want to supplement an existing edit or set of edits with a note. WDYT?

I think you're right, we don't need the notes to be attached to a particular edit. I expect most notes to be standalone: that's actually what I need in my case, and it's also actually what the diag API presents to the user.

diag(loc1, "some warning") << FixItHint::CreateReplacement(...);
diag(loc2, "some note", clang::DiagnosticIDs::Note)

becomes, in transformerland:

makeRule(matcher, flatten(changeTo(...), note(anchor2, cat("some note"))), cat("some warning"))

So I think we only really need an ASTEdit note() generator. I went with that solution.

[ymandel]

Thanks!

clang-tools-extra/clang-tidy/utils/TransformerClangTidyCheck.cpp

 //===---------- TransformerClangTidyCheck.cpp - clang-tidy ----------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "TransformerClangTidyCheck.h"
+#include "clang/Basic/DiagnosticIDs.h"
 #include "clang/Lex/Preprocessor.h"
 #include "llvm/ADT/STLExtras.h"
 
 namespace clang {
 namespace tidy {
 namespace utils {
 using transformer::RewriteRuleWith;
 
void TransformerClangTidyCheck::check(
   Expected<std::string> Explanation = Rule.Metadata[I]->eval(Result);
   if (!Explanation) {
     llvm::errs() << "Error in explanation: "
                  << llvm::toString(Explanation.takeError()) << "\n";
     return;
   }
 
   // Associate the diagnostic with the location of the first change.
+  {
     DiagnosticBuilder Diag =
         diag((*Edits)[0].Range.getBegin(), escapeForDiagnostic(*Explanation));
-  for (const auto &T : *Edits)
+    for (const auto &T : *Edits) {
       switch (T.Kind) {
       case transformer::EditKind::Range:
         Diag << FixItHint::CreateReplacement(T.Range, T.Replacement);
         break;
       case transformer::EditKind::AddInclude:
         Diag << Inserter.createIncludeInsertion(
             Result.SourceManager->getFileID(T.Range.getBegin()), T.Replacement);
         break;
       }
     }
+  }
+  // Emit potential notes.
+  for (const auto &T : *Edits) {
+    if (!T.Note.empty()) {
+      diag(T.Range.getBegin(), escapeForDiagnostic(T.Note),
+           DiagnosticIDs::Note);
+    }
+  }
+}
 
 void TransformerClangTidyCheck::storeOptions(
     ClangTidyOptions::OptionMap &Opts) {
   Options.store(Opts, "IncludeStyle", Inserter.getStyle());
 }
 
 } // namespace utils
 } // namespace tidy
 } // namespace clang

clang-tools-extra/unittests/clang-tidy/TransformerClangTidyCheckTest.cpp

 using namespace ::clang::ast_matchers;
 
 using transformer::cat;
 using transformer::change;
 using transformer::IncludeFormat;
 using transformer::makeRule;
 using transformer::node;
 using transformer::noopEdit;
+using transformer::note;
 using transformer::RewriteRuleWith;
 using transformer::RootID;
 using transformer::statement;
 
 // Invert the code of an if-statement, while maintaining its semantics.
 RewriteRuleWith<std::string> invertIf() {
   StringRef C = "C", T = "T", E = "E";
   RewriteRuleWith<std::string> Rule = makeRule(
DiagOnlyCheck(StringRef Name, ClangTidyContext *Context)
               Name, Context) {}
   };
   std::string Input = "int h() { return 5; }";
   std::vector<ClangTidyError> Errors;
   EXPECT_EQ(Input, test::runCheckOnCode<DiagOnlyCheck>(Input, &Errors));
   EXPECT_EQ(Errors.size(), 1U);
   EXPECT_EQ(Errors[0].Message.Message, "message");
   EXPECT_THAT(Errors[0].Message.Ranges, testing::IsEmpty());
+  EXPECT_THAT(Errors[0].Notes, testing::IsEmpty());
 
   // The diagnostic is anchored to the match, "return 5".
   EXPECT_EQ(Errors[0].Message.FileOffset, 10U);
 }
 
 transformer::ASTEdit noReplacementEdit(transformer::RangeSelector Target) {
   transformer::ASTEdit E;
   E.TargetRange = std::move(Target);
TEST(TransformerClangTidyCheckTest, EmptyReplacement) {
   EXPECT_EQ(Errors.size(), 1U);
   EXPECT_EQ(Errors[0].Message.Message, "message");
   EXPECT_THAT(Errors[0].Message.Ranges, testing::IsEmpty());
 
   // The diagnostic is anchored to the match, "return 5".
   EXPECT_EQ(Errors[0].Message.FileOffset, 10U);
 }
 
+TEST(TransformerClangTidyCheckTest, NotesCorrectlyGenerated) {
+  class DiagAndNoteCheck : public TransformerClangTidyCheck {
+  public:
+    DiagAndNoteCheck(StringRef Name, ClangTidyContext *Context)
+        : TransformerClangTidyCheck(
+              makeRule(returnStmt(),
+                       note(node(RootID), cat("some note")),
+                       cat("message")),
+              Name, Context) {}
+  };
+  std::string Input = "int h() { return 5; }";
+  std::vector<ClangTidyError> Errors;
+  EXPECT_EQ(Input, test::runCheckOnCode<DiagAndNoteCheck>(Input, &Errors));
+  EXPECT_EQ(Errors.size(), 1U);
+  EXPECT_EQ(Errors[0].Notes.size(), 1U);
+  EXPECT_EQ(Errors[0].Notes[0].Message, "some note");
+
+  // The note is anchored to the match, "return 5".
+  EXPECT_EQ(Errors[0].Notes[0].FileOffset, 10U);
+}
+
 TEST(TransformerClangTidyCheckTest, DiagnosticMessageEscaped) {
   class GiveDiagWithPercentSymbol : public TransformerClangTidyCheck {
   public:
     GiveDiagWithPercentSymbol(StringRef Name, ClangTidyContext *Context)
         : TransformerClangTidyCheck(makeRule(returnStmt(),
                                              noopEdit(node(RootID)),
                                              cat("bad code: x % y % z")),
                                     Name, Context) {}

clang/include/clang/Tooling/Transformer/RewriteRule.h

 };
 
 /// A concrete description of a source edit, represented by a character range in
 /// the source to be replaced and a corresponding replacement string.
 struct Edit {
   EditKind Kind = EditKind::Range;
   CharSourceRange Range;
   std::string Replacement;
+  std::string Note;
   llvm::Any Metadata;
 };
 
 /// Format of the path in an include directive -- angle brackets or quotes.
 enum class IncludeFormat {
   Quoted,
   Angled,
 };

 /// Generates no edits.
 inline EditGenerator noEdits() { return editList({}); }
 
 /// Generates a single, no-op edit anchored at the start location of the
 /// specified range. A `noopEdit` may be preferred over `noEdits` to associate a
 /// diagnostic `Explanation` with the rule.
 EditGenerator noopEdit(RangeSelector Anchor);
 
+/// Generates a single, no-op edit with the associated note anchored at the
+/// start location of the specified range.
+ASTEdit note(RangeSelector Anchor, TextGenerator Note);
+
 /// Version of `ifBound` specialized to `ASTEdit`.
 inline EditGenerator ifBound(std::string ID, ASTEdit TrueEdit,
                              ASTEdit FalseEdit) {
   return ifBound(std::move(ID), edit(std::move(TrueEdit)),
                  edit(std::move(FalseEdit)));
 }
 
 /// Version of `ifBound` that has no "False" branch. If the node is not bound,
Edit.Metadata =
       [Gen = std::move(Metadata)](
           const ast_matchers::MatchFinder::MatchResult &R) -> llvm::Any {
     return Gen(R);
   };
 
   return Edit;
 }
 
 /// Assuming that the inner range is enclosed by the outer range, creates
 /// precision edits to remove the parts of the outer range that are not included
 /// in the inner range.
 inline EditGenerator shrinkTo(RangeSelector outer, RangeSelector inner) {
   return editList({remove(enclose(before(outer), before(inner))),
                    remove(enclose(after(inner), after(outer)))});
 }
 
 /// Description of a source-code transformation.
 //
 // A *rewrite rule* describes a transformation of source code. A simple rule
 // contains each of the following components:
 //
 // * Matcher: the pattern term, expressed as clang matchers (with Transformer
 //   extensions).

clang/lib/Tooling/Transformer/RewriteRule.cpp

for (const auto &E : ASTEdits) {
     // empty list, does not abort. As a result, `editList({A,B})` is not
     // equivalent to `flatten(edit(A), edit(B))`. The former will abort if `A`
     // produces a bad range, whereas the latter will simply ignore A.
     if (!EditRange)
       return SmallVector<Edit, 0>();
     transformer::Edit T;
     T.Kind = E.Kind;
     T.Range = *EditRange;
     if (E.Replacement) {
       auto Replacement = E.Replacement->eval(Result);
       if (!Replacement)
         return Replacement.takeError();
       T.Replacement = std::move(*Replacement);
     }
+    if (E.Note) {
+      auto Note = E.Note->eval(Result);
+      if (!Note)
+        return Note.takeError();
+      T.Note = std::move(*Note);
+    }
     if (E.Metadata) {
       auto Metadata = E.Metadata(Result);
       if (!Metadata)
         return Metadata.takeError();
       T.Metadata = std::move(*Metadata);
     }
     Edits.push_back(std::move(T));
   }
   return Edits;
 }
 
 EditGenerator transformer::editList(SmallVector<ASTEdit, 1> Edits) {
   return [Edits = std::move(Edits)](const MatchResult &Result) {
     return translateEdits(Result, Edits);

 
 ASTEdit transformer::changeTo(RangeSelector Target, TextGenerator Replacement) {
   ASTEdit E;
   E.TargetRange = std::move(Target);
   E.Replacement = std::move(Replacement);
   return E;
 }
 
+ASTEdit transformer::note(RangeSelector Anchor, TextGenerator Note) {
+  ASTEdit E;
+  E.TargetRange = transformer::before(Anchor);
+  E.Note = std::move(Note);
+  return E;
+}
+
 namespace {
 /// A \c TextGenerator that always returns a fixed string.
 class SimpleTextGenerator : public MatchComputation<std::string> {
   std::string S;
 
 public:
   SimpleTextGenerator(std::string S) : S(std::move(S)) {}
   llvm::Error eval(const ast_matchers::MatchFinder::MatchResult &,