[PATCH] D52273: [clangd] Initial implementation of expected types

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.


================
Comment at: clangd/ExpectedTypes.h:32
+/// this allows the representation to be stored in the index and compared with
+/// types coming from a different AST later.
+class OpaqueType {
----------------
ilya-biryukov wrote:
> sammccall wrote:
> > Does this need to be a separate class rather than using `std::string`?
> > There are echoes of `SymbolID` here, but there were some factors that don't apply here:
> >  - it was fixed-width
> >  - memory layout was important as we stored lots of these in memory
> >  - we hashed them a lot and wanted a specific hash function
> > 
> > I suspect at least initially producing a somewhat readable std::string a la USRGeneration would be enough.
> Would still want to keep it as a marker type just for the sake of indicating what we return and documentation purposes.
> It also adds some type safety (granted, not much) for some use-cases.
> 
> There's still an option to go strings with `rawStr()` if needed.
For documentation purposes, `using OpaqueType = std::string` or so seems like a reasonable compromise?

This is very heavyweight for the amount of typesafety we get.
(Apart from the class itself, you've got `==` and `!=`, we should definitely have `<<` as well, `DenseMapInfo<>` and `<` may get added down the line...)


================
Comment at: clangd/ExpectedTypes.h:15
+//
+// When using clang APIs, we cannot determine if a type coming from an AST is
+// convertible to another type without looking at both types in the same AST.
----------------
I think this largely rehashes the second sentence of the above para. I'd suggest this one focus more closely on what our model *is*:

  We define an encoding of AST types as opaque strings, which can be stored in the index.
  Similar types (such as `string` and `const string&`) are folded together, forming equivalence classes with the same encoding.


================
Comment at: clangd/ExpectedTypes.h:18
+// Unfortunately, we do not have ASTs for index-based completion, so we have use
+// a stable encoding for the C++ types and map them into equivalence classes
+// based on convertibility.
----------------
("stable" might suggest across versions)


================
Comment at: clangd/ExpectedTypes.h:42
+  /// completion context.
+  static llvm::Optional<OpaqueType> fromPreferredType(ASTContext &Ctx,
+                                                      QualType Type);
----------------
I'd suggest just `fromType`, exposing this as the primary method, and then on `fromCompletionResult` document why it's different.

Having the names suggest the underlying structure (that `fromType` is "more fundamental")  aids understanding, and doesn't really feel like we're painting ourselves into a corner.

Alternately, `fromCompletionContext` and `fromCompletionResult` would be more clearly symmetrical.


================
Comment at: clangd/ExpectedTypes.h:49
+  /// rely on it.
+  llvm::StringRef rawStr() const { return Data; }
+
----------------
nit: if you keep this class, call this raw() for consistency with symbolid?(


================
Comment at: clangd/ExpectedTypes.h:59
+private:
+  static llvm::Optional<OpaqueType> encode(ASTContext &Ctx, QualType Type);
+  explicit OpaqueType(std::string Data);
----------------
any reason to put this in the header?


================
Comment at: unittests/clangd/ExpectedTypeTest.cpp:29
+
+class ASTTest : public ::testing::Test {
+protected:
----------------
This seems fine as a fixture, but I'd merge with the subclass - tests should be easy to read!


================
Comment at: unittests/clangd/ExpectedTypeTest.cpp:51
+
+class ConvertibleToMatcher
+    : public ::testing::MatcherInterface<const ValueDecl *> {
----------------
"convertible to" is a problematic description for a couple of reasons:
 - it's a relationship between types, but encapsulates unrelated semantics to do with completions
 - it's a higher level of abstraction than the code under test

As discussed offline/below, I think the best remedy here is just to drop this matcher - it's only used in one test that can now live with something much simpler.


================
Comment at: unittests/clangd/ExpectedTypeTest.cpp:107
+  std::map<std::string, EquivClass>
+  buildEquivClasses(llvm::ArrayRef<const ValueDecl *> Decls) {
+    std::map<std::string, EquivClass> Classes;
----------------
nit: any reason this takes Decl*s instead of strings? would be a bit terser not to wrap the args in decl()


================
Comment at: unittests/clangd/ExpectedTypeTest.cpp:110
+    for (auto *D : Decls) {
+      auto Type = OpaqueType::fromCompletionResult(
+          ASTCtx(), CodeCompletionResult(D, CCP_Declaration));
----------------
I think we could simplify by only testing the type encodings/equiv classes here, and relying on the function -> return type conversion happening elsewhere.


================
Comment at: unittests/clangd/ExpectedTypeTest.cpp:142
+                              decl("iptr"), decl("bptr"), decl("user_type")};
+  EXPECT_THAT(buildEquivClasses(Decls), ClassesAre({{"b", "i", "ui", "ll"},
+                                                    {"f", "d"},
----------------
Ooh, we should avoid folding bool with other integer types I think!

You hardly ever want to pass a bool where an int is expected. (The reverse int -> bool is somewhat common, but no more than pointer -> bool... type equivalence isn't the right hammer to solve that case).


================
Comment at: unittests/clangd/ExpectedTypeTest.cpp:173
+
+TEST_F(ExpectedTypeConversionTest, FunctionReturns) {
+  build(R"cpp(
----------------
I think this test is a bit too high-level - there are big abstractions between the test code and the code under test (which is pretty simple).

I'd suggest just
`EXPECT_EQ(
    OpaqueType::fromCompletionResult(ASTCtx(), decl("returns_int")),
    OpaqueType::fromExpectedType(ASTCtx(), decl("int_"));`

(If you think there's something worth testing for the pointer case, I'd do that instead rather than as well)


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D52273