[Support] Allow complex names for annotation points and ranges via ${}

[tom-anders]

Summary

This is useful when tests encode information in these names. Currently,
this is pretty limited because names consist of only alphanumeric
characters and '_'.

Arguably, writing foo${name}^bar instead of foo$name^bar might also be a
bit more readable in general.

The new syntax should be fully backwards compatible (if I haven't missed
anything). I tested this against clangd unit tests and everything still passes.

[sammccall]

This seems pretty nice (I do find foo$long_word^bar a bit hard to read). I think I'm in favor, my concerns would be that for a small benefit we're:

• adding some extra complexity people have to understand
• providing more ways to write something, so more bikeshedding opportunities
• encouraging people to embed structured information in the annotations, in a way that won't be very readable *and* isn't well-supported by the API

Can you give examples of where you want to use this?
(I think we should *probably* accept this regardless, but tests want to iterate over payload-name pairs or things like that we should think a couple of steps ahead)

llvm/include/llvm/Testing/Support/Annotations.h

31

How do you feel about ${...} rather than $(...)?

$foo looks like shell to me, and so ${foo} seems like a quoting version while $(foo) reminds me more of some kind of embedded expression.

Small difference but maybe others would have similar associations.

[tom-anders]

In D137909#3924587, @sammccall wrote:

This seems pretty nice (I do find foo$long_word^bar a bit hard to read). I think I'm in favor, my concerns would be that for a small benefit we're:

• adding some extra complexity people have to understand
• providing more ways to write something, so more bikeshedding opportunities
• encouraging people to embed structured information in the annotations, in a way that won't be very readable *and* isn't well-supported by the API

Can you give examples of where you want to use this?
(I think we should *probably* accept this regardless, but tests want to iterate over payload-name pairs or things like that we should think a couple of steps ahead)

Yeah sorry, should've included this example right away:

This clangd test (and some others in this file) use the names of annotation for encoding the expected Attribute field of the Referencestruct.
For testing https://github.com/clangd/clangd/issues/177 (https://reviews.llvm.org/D137894) I wanted to extend those tests instead of writing new one, so the idea was to encode both the expected Attribute and the expected Context inside the annotation name (e.g. via comma-separated list).
Or maybe there's a better way for extending the test without abusing annotations? I couldn't really find a solution that wouldn't involve a major rewrite of the test logic here.

llvm/include/llvm/Testing/Support/Annotations.h

31

Ah yes I agree, this is definitely more intuitive, I'll change it

[tom-anders]

${} instead of $()

[tom-anders]

Fix one test I missed

[sammccall]

TL;DR: I think the ${} is a nice quality-of-life improvement as-is, but attaching complex metadata needs something more. And we should spend our syntax-complexity-budget on the latter. What about making payload a separate attribute of points, with syntax $name(payload)^, which collapses in simple cases to $name^ or $(payload)^ or just ^?

---

In D137909#3924611, @tom-anders wrote:

This clangd test (and some others in this file) use the names of annotation for encoding the expected Attribute field of the Referencestruct.
For testing https://github.com/clangd/clangd/issues/177 (https://reviews.llvm.org/D137894) I wanted to extend those tests instead of writing new one, so the idea was to encode both the expected Attribute and the expected Context inside the annotation name (e.g. via comma-separated list).

Right, I think this is a slightly bigger can of worms...

Annotations was designed to support lookup of points/ranges with known names and the API works pretty well for this purpose without being complicated.
The tests you link work OK because we're looking up a fixed set of names (unnamed, def, decl). Using names for metadata beyond basic categories doesn't work well because you need the name to look them up. (We sometimes hack around this, at the cost of test clarity/simplicity).

Metadata comes up enough that it might be worth accommodating. Simply providing iterators over (name,point) pairs would make this possible but I don't think overloading names in this way is a great design:

• makes it less obvious what a name is
• makes llvm::Annotations API harder to understand
• means names + metadata can't be used together

I think it would be better to make metadata orthogonal to names. Silly strawman syntax:

^  // just a point
@foo^ // unnamed (i.e. def+decl), container is foo
$def^ // definition, no container
$decl@foo^ // definition, contained by foo

then add functions to Annotations like ArrayRef<pair<unsigned, StringRef>> pointsWithPayload(StringRef Name = "")

Of course good syntax is both important and hard: @ isn't really available as a sigil (ObjC), and brackets are nicer in these complicated chains than prefix sigils too... and we need to be backwards compatible.

What do you think about $name(payload)^ for the general case, with $(payload)^, $name, and ^ as abbreviated forms? I don't think the lack of brackets on names hurts a lot if they stay simple, and if they're "just labels" that don't have to carry arbitrary payloads then they can be simple.

[sammccall]

D137894

this looks cool! I left a couple of comments, mentioning here because phab won't notify for comments on drafts.

[tom-anders]

Keep name syntax, add optional payload instead

[tom-anders]

In D137909#3930043, @sammccall wrote:

What do you think about $name(payload)^ for the general case, with $(payload)^, $name, and ^ as abbreviated forms? I don't think the lack of brackets on names hurts a lot if they stay simple, and if they're "just labels" that don't have to carry arbitrary payloads then they can be simple.

Thanks for the detailed feedback, I think this looks perfect! I adapted the code and docs accordingly (I added some comments to the parts I'm not sure about)

In D137909#3930047, @sammccall wrote:

D137894

this looks cool! I left a couple of comments, mentioning here because phab won't notify for comments on drafts.

Thanks! I replied to one of them :)

clang-tools-extra/clangd/unittests/Annotations.h

29

I thought about just using std::pair<clangd::Position, llvm::Optional<llvm::StringRef>> instead, but that is pretty verbose and unreadable here IMO. Another nice thing about subclassing here is that we can keep the API stable and don't have to introduce an additional pointWithPayload()` getter like in the base class.

llvm/include/llvm/Testing/Support/Annotations.h

61

One could argue that this should just be llvm::StringRef. Right now, $name^ results in llvm::None for the payload, while $name()^ will yield an empty string.

Do you think they should just both be empty strings instead?

[tom-anders]

Fix clangd::Annotations interface, now also has pointWithPayload etc.

[tom-anders]

clang-tools-extra/clangd/unittests/Annotations.h

29

Ok the part about not needing API changes was a total lie, I had a typo so that ninja would only build clang but not clangd tests - I now kept the struct but added the same withPayload methods like in llvm::Annotations

[sammccall]

Thanks! sorry about continuing to drift this patch, but this looks like a great improvement.

Critical comments below are:

• dropping Optional and just use StringRef
• using one consistent way to to include payloads across the API. I think pair<T, StringRef> is actually best overall

Others are less important/optional if you want to get this wrapped up sooner :-)

clang-tools-extra/clangd/unittests/tweaks/TweakTesting.h

119 ↗

(On Diff #475939)

not being able to ignore payloads (e.g. not assert the presence/absence) seems like a regression to me, this points further towards separating ranges() and rangesWithPayload

llvm/include/llvm/Testing/Support/Annotations.h

58

this patch takes too many different approaches to attaching the payloads to the points:

• adding it to an existing struct that is already returned
• defining a new wrapping struct for the andPayload version
• defining a new inheriting struct for the andPayload version

I think we should keep it simple, and use pair<T, StringRef> for all the andPayload versions. This isn't optimal in isolation, but it keeps things coherent, avoiding:

• confusion around clever type-system features (inheritance, implicit conversions, smart-pointer like types, templates)
• introducing new structs whose names we have to remember
• inconsistency between how payloads are accessed between ranges/points/clangd/llvm versions

In many cases structured bindings make using the pair versions less cumbersome than they would have been in the past.

61

Yes, I don't think that distinguishing between "no payload" and "empty payload" is useful, so would prefer the simpler API.

95

I hadn't seen these context on these all_ functions, I reached out to the authors to find out about them (they're used out-of-tree).

Basically the use case is to use llvm::Annotations to do the parsing, but dump out the data into some external test helper which lookups/assertions will run on.
So instead of adding extra variants, we just need to expose the parse results in some convenient way.

125

Internally I'd consider switching to a uniform representation of points and ranges. For two reasons:

• we're adding a new feature that's common between them
• all_points/all_ranges want to expose such a representation

Something like:

struct Annotation {
  size_t Start;
  size_t End; // or size_t(-1) for a point
  StringRef Name;
  StringRef Payload;
}
vector<Annotation> All;
StringMap<SmallVector<unsigned>> Points; // values are indices into All
StringMap<SmallVector<unsigned>> Ranges; // values are indices into All

ArrayRef<Annotation> all() { return All; } // replaces all_points and all_ranges

up to you though

llvm/lib/Testing/Support/Annotations.cpp

30

(If using the Annotation struct I mentioned before, an instance of it could cover Name+Payload and it could also be the elements of OpenRanges)

llvm/unittests/Support/AnnotationsTest.cpp

110

this is no payload specified, we should also have one test where it's specified but empty e.g. $()^ (even if we make this equivalent to unspecified)

[tom-anders]

Use std::pair for returning payloads along with points/ranges. Also, refactor internal logic.

[tom-anders]

In D137909#3933730, @sammccall wrote:

Thanks! sorry about continuing to drift this patch, but this looks like a great improvement.

No problem! I wasn't quite happy yet with the previous version of this patch either, really appreciate the continuous feedback!

llvm/include/llvm/Testing/Support/Annotations.h

58

Yeah structured bindings help a lot here. Also, gtest has a Pair matcher which is really nice to use for this stuff

95

Ok, got rid of all_points_and_payloads() and kept all_points() as-is for now

llvm/lib/Testing/Support/Annotations.cpp

30

Felt a bit less readable to me, so I kept the two optionals for name and payload

[sammccall]

Fantastic, thanks!

clang-tools-extra/clangd/unittests/Annotations.h

29–31

Nit: no need for clangd:: qualifiers here or in cpp file, apart from on Range (to distinguish from llvm:: Annotations::Range)

llvm/include/llvm/Testing/Support/Annotations.h

28

Nit: I'd probably rather show payload both with and without name here, and then not bother repeating different combinations for range:

$definition^ // points can be named: "definition"
$(foo)^ // or have a payload: "foo"
$definition(foo)^ // or both
$fail[[...]] // ranges can have names/payloads too

28

Nit: examples here are realistic, so avoid "foo". Maybe "complete"?

78

Nit: I think it's more valuable to keep the one-sentence summary to one line (as above) than to repeat (some of) the syntax options again here, since this is clearly a variant of the method above.
Also the llvm::Optional part is obsolete.

Maybe Return the position of the named marked point, and its payload if any.?

90–91

Can you add a FIXME here and on all_points() to remove these functions in favor of exposing All directly?

(It's great that this isn't done in this patch though: that API break will cause some out-of-tree headaches I'm happy to deal with myself)

118

Document the sentinel value, or maybe better

size_t End = -1;
bool isPoint() const { return End == size_t(-1); }

[sammccall]

(please update the commit message too since the design changed)

[tom-anders]

Thanks again! Landed this with your suggestions incorporated.

clang-tools-extra/clangd/unittests/Annotations.h

 /// Same as llvm::Annotations, but adjusts functions to LSP-specific types for
 /// positions and ranges.
 class Annotations : public llvm::Annotations {
   using Base = llvm::Annotations;
 
 public:
   using llvm::Annotations::Annotations;
 
   Position point(llvm::StringRef Name = "") const;
+  std::pair<Position, llvm::StringRef>
+  pointWithPayload(llvm::StringRef Name = "") const;
   std::vector<Position> points(llvm::StringRef Name = "") const;
+  std::vector<std::pair<Position, llvm::StringRef>>
+  pointsWithPayload(llvm::StringRef Name = "") const;
 
   clangd::Range range(llvm::StringRef Name = "") const;
+  std::pair<clangd::Range, llvm::StringRef>
+  rangeWithPayload(llvm::StringRef Name = "") const;
   std::vector<clangd::Range> ranges(llvm::StringRef Name = "") const;
+  std::vector<std::pair<clangd::Range, llvm::StringRef>>
+  rangesWithPayload(llvm::StringRef Name = "") const;
 };
 
 } // namespace clangd
 } // namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANGD_UNITTESTS_ANNOTATIONS_H

clang-tools-extra/clangd/unittests/Annotations.cpp

 //===--- Annotations.cpp - Annotated source code for unit tests --*- C++-*-===//
 //
 // 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 "Annotations.h"
 #include "SourceCode.h"
 
 namespace clang {
 namespace clangd {
 
 Position Annotations::point(llvm::StringRef Name) const {
-  return offsetToPosition(code(), Base::point(Name));
+  return pointWithPayload(Name).first;
+}
+
+std::pair<Position, llvm::StringRef>
+Annotations::pointWithPayload(llvm::StringRef Name) const {
+  auto [BasePoint, Payload] = Base::pointWithPayload(Name);
+  return {offsetToPosition(code(), BasePoint), Payload};
 }
 
 std::vector<Position> Annotations::points(llvm::StringRef Name) const {
-  auto Offsets = Base::points(Name);
+  auto BasePoints = Base::points(Name);
 
   std::vector<Position> Ps;
-  Ps.reserve(Offsets.size());
-  for (size_t O : Offsets)
-    Ps.push_back(offsetToPosition(code(), O));
+  Ps.reserve(BasePoints.size());
+  for (const auto Point : BasePoints)
+    Ps.push_back(offsetToPosition(code(), Point));
 
   return Ps;
 }
 
-static clangd::Range toLSPRange(llvm::StringRef Code, Annotations::Range R) {
+std::vector<std::pair<Position, llvm::StringRef>>
+Annotations::pointsWithPayload(llvm::StringRef Name) const {
+  auto BasePoints = Base::pointsWithPayload(Name);
+
+  std::vector<std::pair<Position, llvm::StringRef>> Ps;
+  Ps.reserve(BasePoints.size());
+  for (const auto &[Point, Payload] : BasePoints)
+    Ps.push_back({offsetToPosition(code(), Point), Payload});
+
+  return Ps;
+}
+
+static clangd::Range toLSPRange(llvm::StringRef Code,
+                                llvm::Annotations::Range R) {
   clangd::Range LSPRange;
   LSPRange.start = offsetToPosition(Code, R.Begin);
   LSPRange.end = offsetToPosition(Code, R.End);
   return LSPRange;
 }
 
-clangd::Range Annotations::range(llvm::StringRef Name) const {
-  return toLSPRange(code(), Base::range(Name));
+Range Annotations::range(llvm::StringRef Name) const {
+  return rangeWithPayload(Name).first;
+}
+
+std::pair<clangd::Range, llvm::StringRef>
+Annotations::rangeWithPayload(llvm::StringRef Name) const {
+  auto [BaseRange, Payload] = Base::rangeWithPayload(Name);
+  return {toLSPRange(code(), BaseRange), Payload};
 }
 
-std::vector<clangd::Range> Annotations::ranges(llvm::StringRef Name) const {
+std::vector<Range> Annotations::ranges(llvm::StringRef Name) const {
   auto OffsetRanges = Base::ranges(Name);
 
   std::vector<clangd::Range> Rs;
   Rs.reserve(OffsetRanges.size());
-  for (Annotations::Range R : OffsetRanges)
+  for (const auto &R : OffsetRanges)
     Rs.push_back(toLSPRange(code(), R));
 
   return Rs;
 }
 
+std::vector<std::pair<clangd::Range, llvm::StringRef>>
+Annotations::rangesWithPayload(llvm::StringRef Name) const {
+  auto OffsetRanges = Base::rangesWithPayload(Name);
+
+  std::vector<std::pair<clangd::Range, llvm::StringRef>> Rs;
+  Rs.reserve(OffsetRanges.size());
+  for (const auto &[R, Payload] : OffsetRanges)
+    Rs.push_back({toLSPRange(code(), R), Payload});
+
+  return Rs;
+}
+
 } // namespace clangd
 } // namespace clang

llvm/include/llvm/Testing/Support/Annotations.h

 //===--- Annotations.h - Annotated source code for tests ---------*- C++-*-===//
 //
 // 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
 //
 //===----------------------------------------------------------------------===//
 #ifndef LLVM_TESTING_SUPPORT_ANNOTATIONS_H
 #define LLVM_TESTING_SUPPORT_ANNOTATIONS_H
 
+#include "llvm/ADT/Optional.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/StringMap.h"
 #include "llvm/ADT/StringRef.h"
 #include <tuple>
 #include <vector>
 
 namespace llvm {
 
 class raw_ostream;
 
 /// Annotations lets you mark points and ranges inside source code, for tests:
 ///
 ///    Annotations Example(R"cpp(
 ///       int complete() { x.pri^ }         // ^ indicates a point
 ///       void err() { [["hello" == 42]]; } // [[this is a range]]
 ///       $definition^class Foo{};          // points can be named: "definition"
-///       $fail[[static_assert(false, "")]] // ranges can be named too: "fail"
+///       $(foo)^class Foo{};               // ...or have a payload: "foo"
+///       $definition(foo)^class Foo{};     // ...or both
+///       $fail(runtime)[[assert(false)]]   // ranges can have names/payloads too
 ///    )cpp");
 ///
 ///    StringRef Code = Example.code();             // annotations stripped.
 ///    std::vector<size_t> PP = Example.points();   // all unnamed points
 ///    size_t P = Example.point();                  // there must be exactly one
 ///    llvm::Range R = Example.range("fail");       // find named ranges
 ///
 /// Points/ranges are coordinated into `code()` which is stripped of
 /// annotations.
 ///
+/// Names consist of only alphanumeric characters or '_'.
+/// Payloads can contain any character expect '(' and ')'.
+///
 /// Ranges may be nested (and points can be inside ranges), but there's no way
 /// to define general overlapping ranges.
 ///
 /// FIXME: the choice of the marking syntax makes it impossible to represent
 ///        some of the C++ and Objective C constructs (including common ones
 ///        like C++ attributes). We can fix this by:
 ///          1. introducing an escaping mechanism for the special characters,
 ///          2. making characters for marking points and ranges configurable,
 ///          3. changing the syntax to something less commonly used,
 ///          4. ...
 class Annotations {
 public:
   /// Two offsets pointing to a continuous substring. End is not included, i.e.
   /// represents a half-open range.
   struct Range {
     size_t Begin = 0;
     size_t End = 0;
 
     friend bool operator==(const Range &L, const Range &R) {
       return std::tie(L.Begin, L.End) == std::tie(R.Begin, R.End);
     }
     friend bool operator!=(const Range &L, const Range &R) { return !(L == R); }
   };
 
   /// Parses the annotations from Text. Crashes if it's malformed.
   Annotations(llvm::StringRef Text);
 
   /// The input text with all annotations stripped.
   /// All points and ranges are relative to this stripped text.
   llvm::StringRef code() const { return Code; }
 
   /// Returns the position of the point marked by ^ (or $name^) in the text.
   /// Crashes if there isn't exactly one.
   size_t point(llvm::StringRef Name = "") const;
+  /// Returns the position of the point with \p Name and its payload (if any).
+  std::pair<size_t, llvm::StringRef>
+  pointWithPayload(llvm::StringRef Name = "") const;
   /// Returns the position of all points marked by ^ (or $name^) in the text.
   /// Order matches the order within the text.
   std::vector<size_t> points(llvm::StringRef Name = "") const;
+  /// Returns the positions and payloads (if any) of all points named \p Name
+  std::vector<std::pair<size_t, llvm::StringRef>>
+  pointsWithPayload(llvm::StringRef Name = "") const;
   /// Returns the mapping of all names of points marked in the text to their
   /// position. Unnamed points are mapped to the empty string. The positions are
   /// sorted.
-  const llvm::StringMap<llvm::SmallVector<size_t, 1>> &all_points() const;
+  /// FIXME Remove this and expose `All` directly (currently used out-of-tree)
+  llvm::StringMap<llvm::SmallVector<size_t, 1>> all_points() const;
 
   /// Returns the location of the range marked by [[ ]] (or $name[[ ]]).
   /// Crashes if there isn't exactly one.
   Range range(llvm::StringRef Name = "") const;
+  /// Returns the location and payload of the range marked by [[ ]]
+  /// (or $name(payload)[[ ]]). Crashes if there isn't exactly one.
+  std::pair<Range, llvm::StringRef>
+  rangeWithPayload(llvm::StringRef Name = "") const;
   /// Returns the location of all ranges marked by [[ ]] (or $name[[ ]]).
   /// They are ordered by start position within the text.
   std::vector<Range> ranges(llvm::StringRef Name = "") const;
+  /// Returns the location of all ranges marked by [[ ]]
+  /// (or $name(payload)[[ ]]).
+  /// They are ordered by start position within the text.
+  std::vector<std::pair<Range, llvm::StringRef>>
+  rangesWithPayload(llvm::StringRef Name = "") const;
   /// Returns the mapping of all names of ranges marked in the text to their
   /// location. Unnamed ranges are mapped to the empty string. The ranges are
   /// sorted by their start position.
-  const llvm::StringMap<llvm::SmallVector<Range, 1>> &all_ranges() const;
+  llvm::StringMap<llvm::SmallVector<Range, 1>> all_ranges() const;
 
 private:
   std::string Code;
+  /// Either a Point (Only Start) or a Range (Start and End)
+  struct Annotation {
+    size_t Begin;
+    size_t End = -1;
+    bool isPoint() const { return End == size_t(-1); }
+    llvm::StringRef Name;
+    llvm::StringRef Payload;
+  };
+  std::vector<Annotation> All;
+  // Values are the indices into All
   llvm::StringMap<llvm::SmallVector<size_t, 1>> Points;
-  llvm::StringMap<llvm::SmallVector<Range, 1>> Ranges;
+  llvm::StringMap<llvm::SmallVector<size_t, 1>> Ranges;
 };
 
 llvm::raw_ostream &operator<<(llvm::raw_ostream &O,
                               const llvm::Annotations::Range &R);
 
 } // namespace llvm
 
 #endif

llvm/lib/Testing/Support/Annotations.cpp

if (!Assertion) {
     llvm_unreachable("Annotated testcase assertion failed!");
   }
 }
 
 Annotations::Annotations(llvm::StringRef Text) {
   auto Require = [Text](bool Assertion, const char *Msg) {
     require(Assertion, Msg, Text);
   };
   llvm::Optional<llvm::StringRef> Name;
-  llvm::SmallVector<std::pair<llvm::StringRef, size_t>, 8> OpenRanges;
+  llvm::Optional<llvm::StringRef> Payload;
+  llvm::SmallVector<Annotation, 8> OpenRanges;
 
   Code.reserve(Text.size());
   while (!Text.empty()) {
     if (Text.consume_front("^")) {
-      Points[Name.value_or("")].push_back(Code.size());
+      All.push_back(
+          {Code.size(), size_t(-1), Name.value_or(""), Payload.value_or("")});
+      Points[Name.value_or("")].push_back(All.size() - 1);
       Name = llvm::None;
+      Payload = llvm::None;
       continue;
     }
     if (Text.consume_front("[[")) {
-      OpenRanges.emplace_back(Name.value_or(""), Code.size());
+      OpenRanges.push_back(
+          {Code.size(), size_t(-1), Name.value_or(""), Payload.value_or("")});
       Name = llvm::None;
+      Payload = llvm::None;
       continue;
     }
     Require(!Name, "$name should be followed by ^ or [[");
     if (Text.consume_front("]]")) {
       Require(!OpenRanges.empty(), "unmatched ]]");
-      Range R;
-      R.Begin = OpenRanges.back().second;
-      R.End = Code.size();
-      Ranges[OpenRanges.back().first].push_back(R);
+
+      const Annotation &NewRange = OpenRanges.back();
+      All.push_back(
+          {NewRange.Begin, Code.size(), NewRange.Name, NewRange.Payload});
+      Ranges[NewRange.Name].push_back(All.size() - 1);
+
       OpenRanges.pop_back();
       continue;
     }
     if (Text.consume_front("$")) {
       Name =
           Text.take_while([](char C) { return llvm::isAlnum(C) || C == '_'; });
       Text = Text.drop_front(Name->size());
+
+      if (Text.consume_front("(")) {
+        Payload = Text.take_while([](char C) { return C != ')'; });
+        Require(Text.size() > Payload->size(), "unterminated payload");
+        Text = Text.drop_front(Payload->size() + 1);
+      }
+
       continue;
     }
     Code.push_back(Text.front());
     Text = Text.drop_front();
   }
   Require(!Name, "unterminated $name");
   Require(OpenRanges.empty(), "unmatched [[");
 }
 
 size_t Annotations::point(llvm::StringRef Name) const {
+  return pointWithPayload(Name).first;
+}
+
+std::pair<size_t, llvm::StringRef>
+Annotations::pointWithPayload(llvm::StringRef Name) const {
   auto I = Points.find(Name);
   require(I != Points.end() && I->getValue().size() == 1,
           "expected exactly one point", Code);
-  return I->getValue()[0];
+  const Annotation &P = All[I->getValue()[0]];
+  return {P.Begin, P.Payload};
 }
 
 std::vector<size_t> Annotations::points(llvm::StringRef Name) const {
-  auto I = Points.find(Name);
-  if (I == Points.end())
+  auto Pts = pointsWithPayload(Name);
+  std::vector<size_t> Positions;
+  Positions.reserve(Pts.size());
+  for (const auto &[Point, Payload] : Pts)
+    Positions.push_back(Point);
+  return Positions;
+}
+
+std::vector<std::pair<size_t, llvm::StringRef>>
+Annotations::pointsWithPayload(llvm::StringRef Name) const {
+  auto Iter = Points.find(Name);
+  if (Iter == Points.end())
     return {};
-  return {I->getValue().begin(), I->getValue().end()};
+
+  std::vector<std::pair<size_t, llvm::StringRef>> Res;
+  Res.reserve(Iter->getValue().size());
+  for (size_t I : Iter->getValue())
+    Res.push_back({All[I].Begin, All[I].Payload});
+
+  return Res;
 }
 
-const llvm::StringMap<llvm::SmallVector<size_t, 1>> &
-Annotations::all_points() const {
-  return Points;
+llvm::StringMap<llvm::SmallVector<size_t, 1>> Annotations::all_points() const {
+  llvm::StringMap<llvm::SmallVector<size_t, 1>> Result;
+  for (const auto &Name : Points.keys()) {
+    auto Pts = points(Name);
+    Result[Name] = {Pts.begin(), Pts.end()};
+  }
+  return Result;
 }
 
 Annotations::Range Annotations::range(llvm::StringRef Name) const {
+  return rangeWithPayload(Name).first;
+}
+
+std::pair<Annotations::Range, llvm::StringRef>
+Annotations::rangeWithPayload(llvm::StringRef Name) const {
   auto I = Ranges.find(Name);
   require(I != Ranges.end() && I->getValue().size() == 1,
           "expected exactly one range", Code);
-  return I->getValue()[0];
+  const Annotation &R = All[I->getValue()[0]];
+  return {{R.Begin, R.End}, R.Payload};
 }
 
 std::vector<Annotations::Range>
 Annotations::ranges(llvm::StringRef Name) const {
-  auto I = Ranges.find(Name);
-  if (I == Ranges.end())
+  auto WithPayload = rangesWithPayload(Name);
+  std::vector<Annotations::Range> Res;
+  Res.reserve(WithPayload.size());
+  for (const auto &[Range, Payload] : WithPayload)
+    Res.push_back(Range);
+  return Res;
+}
+std::vector<std::pair<Annotations::Range, llvm::StringRef>>
+Annotations::rangesWithPayload(llvm::StringRef Name) const {
+  auto Iter = Ranges.find(Name);
+  if (Iter == Ranges.end())
     return {};
-  return {I->getValue().begin(), I->getValue().end()};
+
+  std::vector<std::pair<Annotations::Range, llvm::StringRef>> Res;
+  Res.reserve(Iter->getValue().size());
+  for (size_t I : Iter->getValue())
+    Res.emplace_back(Annotations::Range{All[I].Begin, All[I].End},
+                     All[I].Payload);
+
+  return Res;
 }
 
-const llvm::StringMap<llvm::SmallVector<Annotations::Range, 1>> &
+llvm::StringMap<llvm::SmallVector<Annotations::Range, 1>>
 Annotations::all_ranges() const {
-  return Ranges;
+  llvm::StringMap<llvm::SmallVector<Annotations::Range, 1>> Res;
+  for (const llvm::StringRef &Name : Ranges.keys()) {
+    auto R = ranges(Name);
+    Res[Name] = {R.begin(), R.end()};
+  }
+  return Res;
 }
 
 llvm::raw_ostream &llvm::operator<<(llvm::raw_ostream &O,
                                     const llvm::Annotations::Range &R) {
   return O << llvm::formatv("[{0}, {1})", R.Begin, R.End);
 }

llvm/unittests/Support/AnnotationsTest.cpp

 //===----- unittests/AnnotationsTest.cpp ----------------------------------===//
 //
 // 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 "llvm/Testing/Support/Annotations.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 
 using ::testing::AllOf;
 using ::testing::ElementsAre;
 using ::testing::IsEmpty;
+using ::testing::Pair;
 using ::testing::ResultOf;
 using ::testing::UnorderedElementsAre;
 
 namespace {
 MATCHER_P2(pair, first_matcher, second_matcher, "") {
   return testing::ExplainMatchResult(
       AllOf(ResultOf([](const auto &entry) { return entry.getKey(); },
                      first_matcher),

 
 TEST(AnnotationsTest, Nested) {
   llvm::Annotations Annotated("a[[f^oo^bar[[b[[a]]z]]]]bcdef");
   EXPECT_THAT(Annotated.points(), ElementsAre(2u, 4u));
   EXPECT_THAT(Annotated.ranges(),
               ElementsAre(range(8, 9), range(7, 10), range(1, 10)));
 }
 
+TEST(AnnotationsTest, Payload) {
+  // // A single unnamed point or range with unspecified payload
+  EXPECT_THAT(llvm::Annotations("a$^b").pointWithPayload(), Pair(1u, ""));
+  EXPECT_THAT(llvm::Annotations("a$[[b]]cdef").rangeWithPayload(),
+              Pair(range(1, 2), ""));
+
+  // A single unnamed point or range with empty payload
+  EXPECT_THAT(llvm::Annotations("a$()^b").pointWithPayload(), Pair(1u, ""));
+  EXPECT_THAT(llvm::Annotations("a$()[[b]]cdef").rangeWithPayload(),
+              Pair(range(1, 2), ""));
+
+  // A single unnamed point or range with payload.
+  EXPECT_THAT(llvm::Annotations("a$(foo)^b").pointWithPayload(),
+              Pair(1u, "foo"));
+  EXPECT_THAT(llvm::Annotations("a$(foo)[[b]]cdef").rangeWithPayload(),
+              Pair(range(1, 2), "foo"));
+
+  // A single named point or range with payload
+  EXPECT_THAT(llvm::Annotations("a$name(foo)^b").pointWithPayload("name"),
+              Pair(1u, "foo"));
+  EXPECT_THAT(
+      llvm::Annotations("a$name(foo)[[b]]cdef").rangeWithPayload("name"),
+      Pair(range(1, 2), "foo"));
+
+  // Multiple named points with payload.
+  llvm::Annotations Annotated("a$p1(p1)^bcd$p2(p2)^123$p1^345");
+  EXPECT_THAT(Annotated.points(), IsEmpty());
+  EXPECT_THAT(Annotated.pointsWithPayload("p1"),
+              ElementsAre(Pair(1u, "p1"), Pair(7u, "")));
+  EXPECT_THAT(Annotated.pointWithPayload("p2"), Pair(4u, "p2"));
+}
+
 TEST(AnnotationsTest, Named) {
   // A single named point or range.
   EXPECT_EQ(llvm::Annotations("a$foo^b").point("foo"), 1u);
   EXPECT_EQ(llvm::Annotations("a$foo[[b]]cdef").range("foo"), range(1, 2));
 
   // Empty names should also work.
   EXPECT_EQ(llvm::Annotations("a$^b").point(""), 1u);
   EXPECT_EQ(llvm::Annotations("a$[[b]]cdef").range(""), range(1, 2));
EXPECT_DEATH(llvm::Annotations("$foo^a$foo^a").point("foo"),
                "expected exactly one point");
   EXPECT_DEATH(llvm::Annotations("$foo[[a]]bc$foo[[a]]").range("foo"),
                "expected exactly one range");
 
   // Parsing failures.
   EXPECT_DEATH(llvm::Annotations("ff[[fdfd"), "unmatched \\[\\[");
   EXPECT_DEATH(llvm::Annotations("ff[[fdjsfjd]]xxx]]"), "unmatched \\]\\]");
   EXPECT_DEATH(llvm::Annotations("ff$fdsfd"), "unterminated \\$name");
+  EXPECT_DEATH(llvm::Annotations("ff$("), "unterminated payload");
+  EXPECT_DEATH(llvm::Annotations("ff$name("), "unterminated payload");
 #endif
 }
 } // namespace