[clangd] Add missing (but documented!) JSONExpr typed accessors

[sammccall]

Summary

Noticed this when I tried to port the Protocol.h parsers.
And tests for the inspect API, which caught a small bug.

[sammccall]

While here, fix a duplicated test.

[ioeric]

clangd/JSONExpr.h

368

Why is this needed? v[x].null() seems to be more intuitive than v.null(x).

unittests/clangd/JSONExprTests.cpp

203

It's not very obvious that this accesses a KV and converts the value, by only reading this line. Would it make sense to make the APIs more explicit e.g. get_as_xxx(...)?

[sammccall]

clangd/JSONExpr.h

368

In the overwhelmingly common case when parsing, v is a pointer, because it resulted from a call to array().

So it's (*v)[x].null() vs v->null(x) - I think the latter is slightly more readable.

But more compelling to me is having consistency between object/array.

We *can* live without these if you like, though - most of the time we iterate over arrays rather than indexed access.

unittests/clangd/JSONExprTests.cpp

203

I think this is made worse because it's an artificial example.
I've sent D40406 which has "real life" code - what do you think about the getters there?

I think the best variant names would be asString() or getString().

I'm a bit reluctant to add a fixed prefix to these common accessors as they seem noisy. I also think we'd need to change all the names on Expr(), which seems like a shame.

So I'm not sure about changing this, but curious what you think of the Protocol conversion code.

[sammccall]

Renames to address review comments

[sammccall]

Thanks for the review! I think you're right about new names.

clangd/JSONExpr.h

368

I've kept these for now, for readability and symmetry.

unittests/clangd/JSONExprTests.cpp

203

After offline discussion, renamed to Expr::asString() and obj::getString().
These aren't much longer and are more accessible to casual inspection.

[sammccall]

rebase

[ioeric]

Lgtm

Thanks for the rename!

clangd/JSONExpr.h

 //   null    (nullptr_t)
 //   boolean (bool)
 //   number  (double)
 //   string  (StringRef)
 //   array   (json::ary)
 //   object  (json::obj)
 //
 // The kind can be queried directly, or implicitly via the typed accessors:
-//   if (Optional<StringRef> S = E.string())
+//   if (Optional<StringRef> S = E.asString()
 //     assert(E.kind() == Expr::String);
 //
 // Array and Object also have typed indexing accessors for easy traversal:
 //   Expected<Expr> E = parse(R"( {"options": {"font": "sans-serif"}} )");
-//   if (json::obj* O = E->object())
-//     if (json::obj* Opts = O->object("options"))
-//       if (Optional<StringRef> Font = Opts->string("font"))
+//   if (json::obj* O = E->asObject())
+//     if (json::obj* Opts = O->getObject("options"))
+//       if (Optional<StringRef> Font = Opts->getString("font"))
 //         assert(Opts->at("font").kind() == Expr::String);
 //
 // === Serialization ===
 //
 // Exprs can be serialized to JSON:
 //   1) raw_ostream << Expr                    // Basic formatting.
 //   2) raw_ostream << formatv("{0}", Expr)    // Basic formatting.
 //   3) raw_ostream << formatv("{0:2}", Expr)  // Pretty-print with indent 2.
case T_Object:
       return Object;
     case T_Array:
       return Array;
     }
     llvm_unreachable("Unknown kind");
   }
 
   // Typed accessors return None/nullptr if the Expr is not of this type.
-  llvm::Optional<std::nullptr_t> null() const {
+  llvm::Optional<std::nullptr_t> asNull() const {
     if (LLVM_LIKELY(Type == T_Null))
       return nullptr;
     return llvm::None;
   }
-  llvm::Optional<bool> boolean() const {
-    if (LLVM_LIKELY(Type == T_Null))
+  llvm::Optional<bool> asBoolean() const {
+    if (LLVM_LIKELY(Type == T_Boolean))
       return as<bool>();
     return llvm::None;
   }
-  llvm::Optional<double> number() const {
+  llvm::Optional<double> asNumber() const {
     if (LLVM_LIKELY(Type == T_Number))
       return as<double>();
     return llvm::None;
   }
-  llvm::Optional<llvm::StringRef> string() const {
+  llvm::Optional<llvm::StringRef> asString() const {
     if (Type == T_String)
       return llvm::StringRef(as<std::string>());
     if (LLVM_LIKELY(Type == T_StringRef))
       return as<llvm::StringRef>();
     return llvm::None;
   }
-  const ObjectExpr *object() const {
+  const ObjectExpr *asObject() const {
     return LLVM_LIKELY(Type == T_Object) ? &as<ObjectExpr>() : nullptr;
   }
-  ObjectExpr *object() {
+  ObjectExpr *asObject() {
     return LLVM_LIKELY(Type == T_Object) ? &as<ObjectExpr>() : nullptr;
   }
-  const ArrayExpr *array() const {
+  const ArrayExpr *asArray() const {
     return LLVM_LIKELY(Type == T_Array) ? &as<ArrayExpr>() : nullptr;
   }
-  ArrayExpr *array() {
+  ArrayExpr *asArray() {
     return LLVM_LIKELY(Type == T_Array) ? &as<ArrayExpr>() : nullptr;
   }
 
   friend llvm::raw_ostream &operator<<(llvm::raw_ostream &, const Expr &);
 
 private:
   void destroy();
   void copyFrom(const Expr &M);
public:
 
     // Allow [] as if Expr was default-constructible as null.
     Expr &operator[](const ObjectKey &K) {
       return emplace(K, Expr(nullptr)).first->second;
     }
     Expr &operator[](ObjectKey &&K) {
       return emplace(std::move(K), Expr(nullptr)).first->second;
     }
+
+    // Look up a property, returning nullptr if it doesn't exist.
+    json::Expr *get(const ObjectKey &K) {
+      auto I = find(K);
+      if (I == end())
+        return nullptr;
+      return &I->second;
+    }
+    const json::Expr *get(const ObjectKey &K) const {
+      auto I = find(K);
+      if (I == end())
+        return nullptr;
+      return &I->second;
+    }
+    // Typed accessors return None/nullptr if
+    //   - the property doesn't exist
+    //   - or it has the wrong type
+    llvm::Optional<std::nullptr_t> getNull(const ObjectKey &K) const {
+      if (auto *V = get(K))
+        return V->asNull();
+      return llvm::None;
+    }
+    llvm::Optional<bool> getBoolean(const ObjectKey &K) const {
+      if (auto *V = get(K))
+        return V->asBoolean();
+      return llvm::None;
+    }
+    llvm::Optional<double> getNumber(const ObjectKey &K) const {
+      if (auto *V = get(K))
+        return V->asNumber();
+      return llvm::None;
+    }
+    llvm::Optional<llvm::StringRef> getString(const ObjectKey &K) const {
+      if (auto *V = get(K))
+        return V->asString();
+      return llvm::None;
+    }
+    const ObjectExpr *getObject(const ObjectKey &K) const {
+      if (auto *V = get(K))
+        return V->asObject();
+      return nullptr;
+    }
+    ObjectExpr *getObject(const ObjectKey &K) {
+      if (auto *V = get(K))
+        return V->asObject();
+      return nullptr;
+    }
+    const ArrayExpr *getArray(const ObjectKey &K) const {
+      if (auto *V = get(K))
+        return V->asArray();
+      return nullptr;
+    }
+    ArrayExpr *getArray(const ObjectKey &K) {
+      if (auto *V = get(K))
+        return V->asArray();
+      return nullptr;
+    }
   };
 
   class ArrayExpr : public std::vector<Expr> {
   public:
     explicit ArrayExpr() {}
     explicit ArrayExpr(std::initializer_list<Expr> Elements) {
       reserve(Elements.size());
       for (const Expr &V : Elements)
         emplace_back(std::move(V));
     };
     template <typename Collection> explicit ArrayExpr(const Collection &C) {
       for (const auto &V : C)
         emplace_back(V);
     }
+
+    // Typed accessors return None/nullptr if the element has the wrong type.
+    llvm::Optional<std::nullptr_t> getNull(size_t I) const {
+      return (*this)[I].asNull();
+    }
+    llvm::Optional<bool> getBoolean(size_t I) const {
+      return (*this)[I].asBoolean();
+    }
+    llvm::Optional<double> getNumber(size_t I) const {
+      return (*this)[I].asNumber();
+    }
+    llvm::Optional<llvm::StringRef> getString(size_t I) const {
+      return (*this)[I].asString();
+    }
+    const ObjectExpr *getObject(size_t I) const {
+      return (*this)[I].asObject();
+    }
+    ObjectExpr *getObject(size_t I) { return (*this)[I].asObject(); }
+    const ArrayExpr *getArray(size_t I) const { return (*this)[I].asArray(); }
+    ArrayExpr *getArray(size_t I) { return (*this)[I].asArray(); }
   };
 
 private:
   mutable llvm::AlignedCharArrayUnion<bool, double, llvm::StringRef,
                                       std::string, ArrayExpr, ObjectExpr>
       Union;
 };
 

clangd/JSONExpr.cpp

case '"': {
     if (parseString(S)) {
       Out = std::move(S);
       return true;
     }
     return false;
   }
   case '[': {
     Out = json::ary{};
-    json::ary &A = *Out.array();
+    json::ary &A = *Out.asArray();
     eatWhitespace();
     if (peek() == ']') {
       ++P;
       return true;
     }
     for (;;) {
       A.emplace_back(nullptr);
       if (!parseExpr(A.back()))
         return false;
       eatWhitespace();
       switch (next()) {
       case ',':
         eatWhitespace();
         continue;
       case ']':
         return true;
       default:
         return parseError("Expected , or ] after array element");
       }
     }
   }
   case '{': {
     Out = json::obj{};
-    json::obj &O = *Out.object();
+    json::obj &O = *Out.asObject();
     eatWhitespace();
     if (peek() == '}') {
       ++P;
       return true;
     }
     for (;;) {
       if (next() != '"')
         return parseError("Expected object key");
llvm::raw_ostream &operator<<(raw_ostream &OS, const Expr &E) {
   return OS;
 }
 
 bool operator==(const Expr &L, const Expr &R) {
   if (L.kind() != R.kind())
     return false;
   switch (L.kind()) {
   case Expr::Null:
-    return L.null() == R.null();
+    return *L.asNull() == *R.asNull();
   case Expr::Boolean:
-    return L.boolean() == R.boolean();
+    return *L.asBoolean() == *R.asBoolean();
   case Expr::Number:
-    return L.boolean() == R.boolean();
+    return *L.asNumber() == *R.asNumber();
   case Expr::String:
-    return L.string() == R.string();
+    return *L.asString() == *R.asString();
   case Expr::Array:
-    return *L.array() == *R.array();
+    return *L.asArray() == *R.asArray();
   case Expr::Object:
-    return *L.object() == *R.object();
+    return *L.asObject() == *R.asObject();
   }
   llvm_unreachable("Unknown expression kind");
 }
 } // namespace json
 } // namespace clangd
 } // namespace clang
 
 void llvm::format_provider<clang::clangd::json::Expr>::format(

unittests/clangd/JSONExprTests.cpp

if (auto E = parse(S)) {
       handleAllErrors(E.takeError(), [S, Msg](const llvm::ErrorInfoBase &E) {
         EXPECT_THAT(E.message(), testing::HasSubstr(Msg)) << S;
       });
     }
   };
   ExpectErr("Unexpected EOF", "");
   ExpectErr("Unexpected EOF", "[");
   ExpectErr("Text after end of document", "[][]");
-  ExpectErr("Text after end of document", "[][]");
   ExpectErr("Invalid bareword", "fuzzy");
   ExpectErr("Expected , or ]", "[2?]");
   ExpectErr("Expected object key", "{a:2}");
   ExpectErr("Expected : after object key", R"({"a",2})");
   ExpectErr("Expected , or } after object property", R"({"a":2 "b":3})");
   ExpectErr("Expected JSON value", R"([&%!])");
   ExpectErr("Invalid number", "1e1.0");
   ExpectErr("Unterminated string", R"("abc\"def)");
   ExpectErr("Control character in string", "\"abc\ndef\"");
   ExpectErr("Invalid escape sequence", R"("\030")");
   ExpectErr("Invalid \\u escape sequence", R"("\usuck")");
   ExpectErr("[3:3, byte=19]", R"({
   "valid": 1,
   invalid: 2
 })");
 }
 
+TEST(JSONTest, Inspection) {
+  llvm::Expected<Expr> Doc = parse(R"(
+    {
+      "null": null,
+      "boolean": false,
+      "number": 2.78,
+      "string": "json",
+      "array": [null, true, 3.14, "hello", [1,2,3], {"time": "arrow"}],
+      "object": {"fruit": "banana"}
+    }
+  )");
+  EXPECT_TRUE(!!Doc);
+
+  obj *O = Doc->asObject();
+  ASSERT_TRUE(O);
+
+  EXPECT_FALSE(O->getNull("missing"));
+  EXPECT_FALSE(O->getNull("boolean"));
+  EXPECT_TRUE(O->getNull("null"));
+
+  EXPECT_EQ(O->getNumber("number"), llvm::Optional<double>(2.78));
+  EXPECT_EQ(O->getString("string"), llvm::Optional<llvm::StringRef>("json"));
+  ASSERT_FALSE(O->getObject("missing"));
+  ASSERT_FALSE(O->getObject("array"));
+  ASSERT_TRUE(O->getObject("object"));
+  EXPECT_EQ(*O->getObject("object"), (obj{{"fruit", "banana"}}));
+
+  ary *A = O->getArray("array");
+  ASSERT_TRUE(A);
+  EXPECT_EQ(A->getBoolean(1), llvm::Optional<bool>(true));
+  ASSERT_TRUE(A->getArray(4));
+  EXPECT_EQ(*A->getArray(4), (ary{1, 2, 3}));
+  int I = 0;
+  for (Expr &E : *A) {
+    if (I++ == 5) {
+      ASSERT_TRUE(E.asObject());
+      EXPECT_EQ(E.asObject()->getString("time"),
+                llvm::Optional<llvm::StringRef>("arrow"));
+    } else
+      EXPECT_FALSE(E.asObject());
+  }
+}
+
 } // namespace
 } // namespace json
 } // namespace clangd
 } // namespace clang