[PATCH] D52273: [clangd] Initial implementation of expected types
This revision was automatically updated to reflect the committed changes.
Closed by commit rL347559: [clangd] Initial implementation of expected types
(authored by ibiryukov, committed by ).
Herald added a subscriber: llvm-commits.
Repository:
rL LLVM
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D52273/new/
https://reviews.llvm.org/D52273
Files:
clang-tools-extra/trunk/clangd/CMakeLists.txt
clang-tools-extra/trunk/clangd/ExpectedTypes.cpp
clang-tools-extra/trunk/clangd/ExpectedTypes.h
clang-tools-extra/trunk/unittests/clangd/CMakeLists.txt
clang-tools-extra/trunk/unittests/clangd/ExpectedTypeTest.cpp
Index: clang-tools-extra/trunk/unittests/clangd/ExpectedTypeTest.cpp
===
--- clang-tools-extra/trunk/unittests/clangd/ExpectedTypeTest.cpp
+++ clang-tools-extra/trunk/unittests/clangd/ExpectedTypeTest.cpp
@@ -0,0 +1,157 @@
+//===-- ExpectedTypeTest.cpp ---*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "ClangdUnit.h"
+#include "ExpectedTypes.h"
+#include "TestTU.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/AST/Decl.h"
+#include "llvm/ADT/StringRef.h"
+#include "gmock/gmock-matchers.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+using namespace llvm;
+
+namespace clang {
+namespace clangd {
+namespace {
+
+using ::testing::Field;
+using ::testing::Matcher;
+using ::testing::SizeIs;
+using ::testing::UnorderedElementsAreArray;
+
+class ExpectedTypeConversionTest : public ::testing::Test {
+protected:
+ void build(StringRef Code) {
+assert(!AST && "AST built twice");
+AST = TestTU::withCode(Code).build();
+ }
+
+ const ValueDecl *decl(StringRef Name) {
+return (findDecl(*AST, Name));
+ }
+
+ QualType typeOf(StringRef Name) {
+return decl(Name)->getType().getCanonicalType();
+ }
+
+ /// An overload for convenience.
+ Optional fromCompletionResult(const ValueDecl *D) {
+return OpaqueType::fromCompletionResult(
+ASTCtx(), CodeCompletionResult(D, CCP_Declaration));
+ }
+
+ /// A set of DeclNames whose type match each other computed by
+ /// OpaqueType::fromCompletionResult.
+ using EquivClass = std::set;
+
+ Matcher>
+ ClassesAre(ArrayRef Classes) {
+using MapEntry = std::map::value_type;
+
+std::vector> Elements;
+Elements.reserve(Classes.size());
+for (auto : Classes)
+ Elements.push_back(Field(::second, Cls));
+return UnorderedElementsAreArray(Elements);
+ }
+
+ // Groups \p Decls into equivalence classes based on the result of
+ // 'OpaqueType::fromCompletionResult'.
+ std::map
+ buildEquivClasses(ArrayRef DeclNames) {
+std::map Classes;
+for (StringRef Name : DeclNames) {
+ auto Type = OpaqueType::fromType(ASTCtx(), typeOf(Name));
+ Classes[Type->raw()].insert(Name);
+}
+return Classes;
+ }
+
+ ASTContext () { return AST->getASTContext(); }
+
+private:
+ // Set after calling build().
+ Optional AST;
+};
+
+TEST_F(ExpectedTypeConversionTest, BasicTypes) {
+ build(R"cpp(
+// ints.
+bool b;
+int i;
+unsigned int ui;
+long long ll;
+
+// floats.
+float f;
+double d;
+
+// pointers
+int* iptr;
+bool* bptr;
+
+// user-defined types.
+struct X {};
+X user_type;
+ )cpp");
+
+ EXPECT_THAT(buildEquivClasses({"b", "i", "ui", "ll", "f", "d", "iptr", "bptr",
+ "user_type"}),
+ ClassesAre({{"b"},
+ {"i", "ui", "ll"},
+ {"f", "d"},
+ {"iptr"},
+ {"bptr"},
+ {"user_type"}}));
+}
+
+TEST_F(ExpectedTypeConversionTest, ReferencesDontMatter) {
+ build(R"cpp(
+int noref;
+int & ref = noref;
+const int & const_ref = noref;
+int && rv_ref = 10;
+ )cpp");
+
+ EXPECT_THAT(buildEquivClasses({"noref", "ref", "const_ref", "rv_ref"}),
+ SizeIs(1));
+}
+
+TEST_F(ExpectedTypeConversionTest, ArraysDecay) {
+ build(R"cpp(
+ int arr[2];
+ int (_ref)[2] = arr;
+ int *ptr;
+ )cpp");
+
+ EXPECT_THAT(buildEquivClasses({"arr", "arr_ref", "ptr"}), SizeIs(1));
+}
+
+TEST_F(ExpectedTypeConversionTest, FunctionReturns) {
+ build(R"cpp(
+ int returns_int();
+ int* returns_ptr();
+
+ int int_;
+ int* int_ptr;
+ )cpp");
+
+ OpaqueType IntTy = *OpaqueType::fromType(ASTCtx(), typeOf("int_"));
+ EXPECT_EQ(fromCompletionResult(decl("returns_int")), IntTy);
+
+ OpaqueType IntPtrTy = *OpaqueType::fromType(ASTCtx(), typeOf("int_ptr"));
+ EXPECT_EQ(fromCompletionResult(decl("returns_ptr")), IntPtrTy);
+}
+
+} // namespace
+} // namespace clangd
+} // namespace clang
Index:
[PATCH] D52273: [clangd] Initial implementation of expected types
ilya-biryukov updated this revision to Diff 175098.
ilya-biryukov marked 2 inline comments as done.
ilya-biryukov added a comment.
- Add using namespace llvm, get rid of llvm::
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D52273
Files:
clangd/CMakeLists.txt
clangd/ExpectedTypes.cpp
clangd/ExpectedTypes.h
unittests/clangd/CMakeLists.txt
unittests/clangd/ExpectedTypeTest.cpp
Index: unittests/clangd/ExpectedTypeTest.cpp
===
--- /dev/null
+++ unittests/clangd/ExpectedTypeTest.cpp
@@ -0,0 +1,157 @@
+//===-- ExpectedTypeTest.cpp ---*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "ClangdUnit.h"
+#include "ExpectedTypes.h"
+#include "TestTU.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/AST/Decl.h"
+#include "llvm/ADT/StringRef.h"
+#include "gmock/gmock-matchers.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+using namespace llvm;
+
+namespace clang {
+namespace clangd {
+namespace {
+
+using ::testing::Field;
+using ::testing::Matcher;
+using ::testing::SizeIs;
+using ::testing::UnorderedElementsAreArray;
+
+class ExpectedTypeConversionTest : public ::testing::Test {
+protected:
+ void build(StringRef Code) {
+assert(!AST && "AST built twice");
+AST = TestTU::withCode(Code).build();
+ }
+
+ const ValueDecl *decl(StringRef Name) {
+return (findDecl(*AST, Name));
+ }
+
+ QualType typeOf(StringRef Name) {
+return decl(Name)->getType().getCanonicalType();
+ }
+
+ /// An overload for convenience.
+ Optional fromCompletionResult(const ValueDecl *D) {
+return OpaqueType::fromCompletionResult(
+ASTCtx(), CodeCompletionResult(D, CCP_Declaration));
+ }
+
+ /// A set of DeclNames whose type match each other computed by
+ /// OpaqueType::fromCompletionResult.
+ using EquivClass = std::set;
+
+ Matcher>
+ ClassesAre(ArrayRef Classes) {
+using MapEntry = std::map::value_type;
+
+std::vector> Elements;
+Elements.reserve(Classes.size());
+for (auto : Classes)
+ Elements.push_back(Field(::second, Cls));
+return UnorderedElementsAreArray(Elements);
+ }
+
+ // Groups \p Decls into equivalence classes based on the result of
+ // 'OpaqueType::fromCompletionResult'.
+ std::map
+ buildEquivClasses(ArrayRef DeclNames) {
+std::map Classes;
+for (StringRef Name : DeclNames) {
+ auto Type = OpaqueType::fromType(ASTCtx(), typeOf(Name));
+ Classes[Type->raw()].insert(Name);
+}
+return Classes;
+ }
+
+ ASTContext () { return AST->getASTContext(); }
+
+private:
+ // Set after calling build().
+ Optional AST;
+};
+
+TEST_F(ExpectedTypeConversionTest, BasicTypes) {
+ build(R"cpp(
+// ints.
+bool b;
+int i;
+unsigned int ui;
+long long ll;
+
+// floats.
+float f;
+double d;
+
+// pointers
+int* iptr;
+bool* bptr;
+
+// user-defined types.
+struct X {};
+X user_type;
+ )cpp");
+
+ EXPECT_THAT(buildEquivClasses({"b", "i", "ui", "ll", "f", "d", "iptr", "bptr",
+ "user_type"}),
+ ClassesAre({{"b"},
+ {"i", "ui", "ll"},
+ {"f", "d"},
+ {"iptr"},
+ {"bptr"},
+ {"user_type"}}));
+}
+
+TEST_F(ExpectedTypeConversionTest, ReferencesDontMatter) {
+ build(R"cpp(
+int noref;
+int & ref = noref;
+const int & const_ref = noref;
+int && rv_ref = 10;
+ )cpp");
+
+ EXPECT_THAT(buildEquivClasses({"noref", "ref", "const_ref", "rv_ref"}),
+ SizeIs(1));
+}
+
+TEST_F(ExpectedTypeConversionTest, ArraysDecay) {
+ build(R"cpp(
+ int arr[2];
+ int (_ref)[2] = arr;
+ int *ptr;
+ )cpp");
+
+ EXPECT_THAT(buildEquivClasses({"arr", "arr_ref", "ptr"}), SizeIs(1));
+}
+
+TEST_F(ExpectedTypeConversionTest, FunctionReturns) {
+ build(R"cpp(
+ int returns_int();
+ int* returns_ptr();
+
+ int int_;
+ int* int_ptr;
+ )cpp");
+
+ OpaqueType IntTy = *OpaqueType::fromType(ASTCtx(), typeOf("int_"));
+ EXPECT_EQ(fromCompletionResult(decl("returns_int")), IntTy);
+
+ OpaqueType IntPtrTy = *OpaqueType::fromType(ASTCtx(), typeOf("int_ptr"));
+ EXPECT_EQ(fromCompletionResult(decl("returns_ptr")), IntPtrTy);
+}
+
+} // namespace
+} // namespace clangd
+} // namespace clang
Index: unittests/clangd/CMakeLists.txt
===
--- unittests/clangd/CMakeLists.txt
+++ unittests/clangd/CMakeLists.txt
@@ -19,6 +19,7 @@
ContextTests.cpp
DexTests.cpp
DraftStoreTests.cpp
+ ExpectedTypeTest.cpp
FileDistanceTests.cpp
FileIndexTests.cpp
[PATCH] D52273: [clangd] Initial implementation of expected types
sammccall accepted this revision.
sammccall added inline comments.
This revision is now accepted and ready to land.
Comment at: clangd/ExpectedTypes.cpp:8
+
+namespace clang {
+namespace clangd {
nit: using namespace llvm (until/unless we switch other files)
Comment at: unittests/clangd/ExpectedTypeTest.cpp:33
+protected:
+ void build(llvm::StringRef Code) {
+assert(!AST && "AST built twice");
drop llvm:: here and below?
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D52273
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52273: [clangd] Initial implementation of expected types
ilya-biryukov 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 {
sammccall wrote:
> 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...)
As discussed offline, kept the class with an expectation that we'll use the
fixed-size representation at some point. Added a comment that it can be viewed
as a strong typedef to string for now.
Comment at: clangd/ExpectedTypes.h:42
+ /// completion context.
+ static llvm::Optional fromPreferredType(ASTContext ,
+ QualType Type);
sammccall wrote:
> 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.
Done. Using `fromType` now.
Comment at: clangd/ExpectedTypes.h:59
+private:
+ static llvm::Optional encode(ASTContext , QualType Type);
+ explicit OpaqueType(std::string Data);
sammccall wrote:
> any reason to put this in the header?
It uses a private constructor of the class, so it seems natural for it to be a
private static function.
Comment at: unittests/clangd/ExpectedTypeTest.cpp:51
+
+class ConvertibleToMatcher
+: public ::testing::MatcherInterface {
sammccall wrote:
> "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.
Done. It was needed only for one test, testing it diretly now.
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"},
sammccall wrote:
> 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).
Fair point, changed this. Bool requires a whole different handling anyway, e.g.
I definitely want my pointers to be boosted in if conditions.
Comment at: unittests/clangd/ExpectedTypeTest.cpp:173
+
+TEST_F(ExpectedTypeConversionTest, FunctionReturns) {
+ build(R"cpp(
sammccall wrote:
> 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)
Done. There is still a helper variable per case (I think it improves the
readability a little), but otherwise the test is more straightforward now.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D52273
___
cfe-commits mailing list
[PATCH] D52273: [clangd] Initial implementation of expected types
ilya-biryukov updated this revision to Diff 175050.
ilya-biryukov marked 11 inline comments as done.
ilya-biryukov added a comment.
- Address comments
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D52273
Files:
clangd/CMakeLists.txt
clangd/ExpectedTypes.cpp
clangd/ExpectedTypes.h
unittests/clangd/CMakeLists.txt
unittests/clangd/ExpectedTypeTest.cpp
Index: unittests/clangd/ExpectedTypeTest.cpp
===
--- /dev/null
+++ unittests/clangd/ExpectedTypeTest.cpp
@@ -0,0 +1,157 @@
+//===-- ExpectedTypeTest.cpp ---*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "ClangdUnit.h"
+#include "ExpectedTypes.h"
+#include "TestTU.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/AST/Decl.h"
+#include "llvm/ADT/StringRef.h"
+#include "gmock/gmock-matchers.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+using namespace llvm;
+
+namespace clang {
+namespace clangd {
+namespace {
+
+using ::testing::Field;
+using ::testing::Matcher;
+using ::testing::SizeIs;
+using ::testing::UnorderedElementsAreArray;
+
+class ExpectedTypeConversionTest : public ::testing::Test {
+protected:
+ void build(llvm::StringRef Code) {
+assert(!AST && "AST built twice");
+AST = TestTU::withCode(Code).build();
+ }
+
+ const ValueDecl *decl(llvm::StringRef Name) {
+return ::cast(findDecl(*AST, Name));
+ }
+
+ QualType typeOf(llvm::StringRef Name) {
+return decl(Name)->getType().getCanonicalType();
+ }
+
+ /// An overload for convenience.
+ Optional fromCompletionResult(const ValueDecl *D) {
+return OpaqueType::fromCompletionResult(
+ASTCtx(), CodeCompletionResult(D, CCP_Declaration));
+ }
+
+ /// A set of DeclNames whose type match each other computed by
+ /// OpaqueType::fromCompletionResult.
+ using EquivClass = std::set;
+
+ Matcher>
+ ClassesAre(llvm::ArrayRef Classes) {
+using MapEntry = std::map::value_type;
+
+std::vector> Elements;
+Elements.reserve(Classes.size());
+for (auto : Classes)
+ Elements.push_back(Field(::second, Cls));
+return UnorderedElementsAreArray(Elements);
+ }
+
+ // Groups \p Decls into equivalence classes based on the result of
+ // 'OpaqueType::fromCompletionResult'.
+ std::map
+ buildEquivClasses(llvm::ArrayRef DeclNames) {
+std::map Classes;
+for (llvm::StringRef Name : DeclNames) {
+ auto Type = OpaqueType::fromType(ASTCtx(), typeOf(Name));
+ Classes[Type->raw()].insert(Name);
+}
+return Classes;
+ }
+
+ ASTContext () { return AST->getASTContext(); }
+
+private:
+ // Set after calling build().
+ llvm::Optional AST;
+};
+
+TEST_F(ExpectedTypeConversionTest, BasicTypes) {
+ build(R"cpp(
+// ints.
+bool b;
+int i;
+unsigned int ui;
+long long ll;
+
+// floats.
+float f;
+double d;
+
+// pointers
+int* iptr;
+bool* bptr;
+
+// user-defined types.
+struct X {};
+X user_type;
+ )cpp");
+
+ EXPECT_THAT(buildEquivClasses({"b", "i", "ui", "ll", "f", "d", "iptr", "bptr",
+ "user_type"}),
+ ClassesAre({{"b"},
+ {"i", "ui", "ll"},
+ {"f", "d"},
+ {"iptr"},
+ {"bptr"},
+ {"user_type"}}));
+}
+
+TEST_F(ExpectedTypeConversionTest, ReferencesDontMatter) {
+ build(R"cpp(
+int noref;
+int & ref = noref;
+const int & const_ref = noref;
+int && rv_ref = 10;
+ )cpp");
+
+ EXPECT_THAT(buildEquivClasses({"noref", "ref", "const_ref", "rv_ref"}),
+ SizeIs(1));
+}
+
+TEST_F(ExpectedTypeConversionTest, ArraysDecay) {
+ build(R"cpp(
+ int arr[2];
+ int (_ref)[2] = arr;
+ int *ptr;
+ )cpp");
+
+ EXPECT_THAT(buildEquivClasses({"arr", "arr_ref", "ptr"}), SizeIs(1));
+}
+
+TEST_F(ExpectedTypeConversionTest, FunctionReturns) {
+ build(R"cpp(
+ int returns_int();
+ int* returns_ptr();
+
+ int int_;
+ int* int_ptr;
+ )cpp");
+
+ OpaqueType IntTy = *OpaqueType::fromType(ASTCtx(), typeOf("int_"));
+ EXPECT_EQ(fromCompletionResult(decl("returns_int")), IntTy);
+
+ OpaqueType IntPtrTy = *OpaqueType::fromType(ASTCtx(), typeOf("int_ptr"));
+ EXPECT_EQ(fromCompletionResult(decl("returns_ptr")), IntPtrTy);
+}
+
+} // namespace
+} // namespace clangd
+} // namespace clang
Index: unittests/clangd/CMakeLists.txt
===
--- unittests/clangd/CMakeLists.txt
+++ unittests/clangd/CMakeLists.txt
@@ -19,6 +19,7 @@
ContextTests.cpp
DexTests.cpp
DraftStoreTests.cpp
+ ExpectedTypeTest.cpp
FileDistanceTests.cpp
[PATCH] D52273: [clangd] Initial implementation of expected types
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 fromPreferredType(ASTContext ,
+ 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 encode(ASTContext , 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 {
"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
+ buildEquivClasses(llvm::ArrayRef Decls) {
+std::map 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),
[PATCH] D52273: [clangd] Initial implementation of expected types
ilya-biryukov marked 2 inline comments as done.
ilya-biryukov added inline comments.
Comment at: clangd/ExpectedTypes.cpp:12
+
+static llvm::Optional toEquivClass(ASTContext , QualType T) {
+ if (T.isNull() || T->isDependentType())
sammccall wrote:
> returning QualType vs Type*? It seems we strip all qualifiers, seems clearest
> for the return type to reflect that.
Done. That produces a bit more trouble at the callsites, so not sure if it's an
improvement overall.
Comment at: clangd/ExpectedTypes.cpp:15
+return llvm::None;
+ T = T.getCanonicalType().getNonReferenceType().getUnqualifiedType();
+ if (T->isIntegerType() && !T->isEnumeralType())
sammccall wrote:
> Maybe we want Ctx.getUnqualifiedArrayType here or (more likely?) do
> array-to-pointer decay?
Added array-to-pointer decays, they should improve ranking when assigning from
an array to a pointer, which is nice.
Also added a FIXME that we should drop qualifiers from inner types of the
pointers (since we do this for arrays). I think it's fine to leave it for the
later improvements.
Comment at: clangd/ExpectedTypes.cpp:16
+ T = T.getCanonicalType().getNonReferenceType().getUnqualifiedType();
+ if (T->isIntegerType() && !T->isEnumeralType())
+return QualType(Ctx.IntTy);
sammccall wrote:
> wow, "enumeral" might be my favorite c++-made-up word, displacing "emplace"...
¯\_(ツ)_/¯
Comment at: clangd/ExpectedTypes.cpp:30
+return llvm::None;
+ QualType T = VD->getType().getCanonicalType().getNonReferenceType();
+ if (!T->isFunctionType())
sammccall wrote:
> nit: is canonicalization necessary here? you do it in toEquivClass
> (I guess dropping references is, for the function type check)
It was not important, removed it.
Comment at: clangd/ExpectedTypes.cpp:46
+ llvm::SmallString<128> Out;
+ if (index::generateUSRForType(T, Ctx, Out))
+return llvm::None;
sammccall wrote:
> I think ultimately we may want to replace this with a custom walker:
> - we may want to ignore attributes (e.g. const) or bail out in some cases
> - generateUSRForType may not have the exact semantics we want for other
> random reasons
> - we can do tricks with hash_combine to avoid actually building huge strings
> we don't care about
>
> not something for this patch, but maybe a FIXME?
>
USRs actually seems like a pretty good fit here. I'm not sure dropping
attributes for internal types would make a big difference in the scoring and
not sure how big of a problem the strings are, would be nice to actually learn
it's a problem (in memory consumption, memory alloc rates, etc) before changing
this.
It's definitely possible to do that, of course, we have a room to change the
encoding whenever we want, but would avoid adding a FIXME and committing to
this approach in the initial patch.
Comment at: clangd/ExpectedTypes.h:10
+// A simplified model of C++ types that can be used to check whether they are
+// convertible between each other without looking at the ASTs.
+// Used for code completion ranking.
ilya-biryukov wrote:
> ioeric wrote:
> > We might want to formalize what "convertible" means here. E.g. does it
> > cover conversion between base and derived class? Does it cover double <->
> > int conversion?
> I want to leave it vague for now. Convertible means whatever we think is good
> for code completion ranking.
> Formalizing means we'll either dig into the C++ encoding or be imprecise.
>
> Happy to add the docs, but they'll probably get outdated on every change.
> Reading the code is actually simpler to get what's going on at this point.
Added a clarification that we want "convertible for the purpose of code
completion".
Comment at: clangd/ExpectedTypes.h:29
+namespace clangd {
+/// An opaque representation of a type that can be computed based on clang AST
+/// and compared for equality. The encoding is stable between different ASTs,
ioeric wrote:
> The name seems opaque ;) Why is it `opaque`?
Removed the "opaque" from the comment, hopefully this causes less confusion.
The idea is that users shouldn't rely on this representation in any other way
than comparing it for equality.
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 {
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
[PATCH] D52273: [clangd] Initial implementation of expected types
ilya-biryukov updated this revision to Diff 174217.
ilya-biryukov marked 10 inline comments as done.
ilya-biryukov added a comment.
- Address comments
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D52273
Files:
clangd/CMakeLists.txt
clangd/ExpectedTypes.cpp
clangd/ExpectedTypes.h
unittests/clangd/CMakeLists.txt
unittests/clangd/ExpectedTypeTest.cpp
Index: unittests/clangd/ExpectedTypeTest.cpp
===
--- /dev/null
+++ unittests/clangd/ExpectedTypeTest.cpp
@@ -0,0 +1,198 @@
+//===-- ExpectedTypeTest.cpp ---*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "ClangdUnit.h"
+#include "ExpectedTypes.h"
+#include "TestTU.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/AST/Decl.h"
+#include "llvm/ADT/StringRef.h"
+#include "gmock/gmock-matchers.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+namespace clang {
+namespace clangd {
+namespace {
+
+using ::testing::Field;
+using ::testing::Matcher;
+using ::testing::SizeIs;
+using ::testing::UnorderedElementsAreArray;
+
+class ASTTest : public ::testing::Test {
+protected:
+ void build(llvm::StringRef Code) {
+assert(!AST && "AST built twice");
+AST = TestTU::withCode(Code).build();
+ }
+
+ const ValueDecl *decl(llvm::StringRef Name) {
+return ::cast(findDecl(*AST, Name));
+ }
+
+ QualType typeOf(llvm::StringRef Name) {
+return decl(Name)->getType().getCanonicalType();
+ }
+
+ ASTContext () { return AST->getASTContext(); }
+
+private:
+ // Set after calling build().
+ llvm::Optional AST;
+};
+
+class ConvertibleToMatcher
+: public ::testing::MatcherInterface {
+ ASTContext
+ QualType To;
+ OpaqueType PreferredType;
+
+public:
+ ConvertibleToMatcher(ASTContext , QualType To)
+ : Ctx(Ctx), To(To.getCanonicalType()),
+PreferredType(*OpaqueType::fromPreferredType(Ctx, To)) {}
+
+ void DescribeTo(std::ostream *OS) const override {
+*OS << "Is convertible to type '" << To.getAsString() << "'";
+ }
+
+ bool MatchAndExplain(const ValueDecl *V,
+ ::testing::MatchResultListener *L) const override {
+assert(V);
+assert(>getASTContext() == && "different ASTs?");
+auto ConvertibleTo = OpaqueType::fromCompletionResult(
+Ctx, CodeCompletionResult(V, CCP_Declaration));
+
+bool Matched = PreferredType == ConvertibleTo;
+if (L->IsInterested())
+ *L << "Types of source and target "
+ << (Matched ? "matched" : "did not match")
+ << "\n\tTarget type: " << To.getAsString()
+ << "\n\tSource value type: " << V->getType().getAsString();
+return Matched;
+ }
+};
+
+class ExpectedTypeConversionTest : public ASTTest {
+protected:
+ Matcher isConvertibleTo(QualType To) {
+return ::testing::MakeMatcher(new ConvertibleToMatcher(ASTCtx(), To));
+ }
+
+ // A set of DeclNames whose type match each other computed by
+ // OpaqueType::fromCompletionResult.
+ using EquivClass = std::set;
+
+ Matcher>
+ ClassesAre(llvm::ArrayRef Classes) {
+using MapEntry = std::map::value_type;
+
+std::vector> Elements;
+Elements.reserve(Classes.size());
+for (auto : Classes)
+ Elements.push_back(Field(::second, Cls));
+return UnorderedElementsAreArray(Elements);
+ }
+
+ // Groups \p Decls into equivalence classes based on the result of
+ // 'OpaqueType::fromCompletionResult'.
+ std::map
+ buildEquivClasses(llvm::ArrayRef Decls) {
+std::map Classes;
+for (auto *D : Decls) {
+ auto Type = OpaqueType::fromCompletionResult(
+ ASTCtx(), CodeCompletionResult(D, CCP_Declaration));
+ Classes[Type->rawStr()].insert(D->getNameAsString());
+}
+return Classes;
+ }
+};
+
+TEST_F(ExpectedTypeConversionTest, BasicTypes) {
+ build(R"cpp(
+// ints.
+bool b;
+int i;
+unsigned int ui;
+long long ll;
+
+// floats.
+float f;
+double d;
+
+// pointers
+int* iptr;
+bool* bptr;
+
+// user-defined types.
+struct X {};
+X user_type;
+ )cpp");
+
+ const ValueDecl *Decls[] = {decl("b"),decl("i"),decl("ui"),
+ decl("ll"), decl("f"),decl("d"),
+ decl("iptr"), decl("bptr"), decl("user_type")};
+ EXPECT_THAT(buildEquivClasses(Decls), ClassesAre({{"b", "i", "ui", "ll"},
+{"f", "d"},
+{"iptr"},
+{"bptr"},
+{"user_type"}}));
+}
+
+TEST_F(ExpectedTypeConversionTest, ReferencesDontMatter) {
+ build(R"cpp(
+int noref;
+int
[PATCH] D52273: [clangd] Initial implementation of expected types
sammccall added inline comments.
Comment at: clangd/ExpectedTypes.cpp:12
+
+static llvm::Optional toEquivClass(ASTContext , QualType T) {
+ if (T.isNull() || T->isDependentType())
returning QualType vs Type*? It seems we strip all qualifiers, seems clearest
for the return type to reflect that.
Comment at: clangd/ExpectedTypes.cpp:15
+return llvm::None;
+ T = T.getCanonicalType().getNonReferenceType().getUnqualifiedType();
+ if (T->isIntegerType() && !T->isEnumeralType())
Maybe we want Ctx.getUnqualifiedArrayType here or (more likely?) do
array-to-pointer decay?
Comment at: clangd/ExpectedTypes.cpp:16
+ T = T.getCanonicalType().getNonReferenceType().getUnqualifiedType();
+ if (T->isIntegerType() && !T->isEnumeralType())
+return QualType(Ctx.IntTy);
wow, "enumeral" might be my favorite c++-made-up word, displacing "emplace"...
Comment at: clangd/ExpectedTypes.cpp:25
+typeOfCompletion(const CodeCompletionResult ) {
+ if (!R.Declaration)
+return llvm::None;
nit: dyn_cast_or_null below instead?
Comment at: clangd/ExpectedTypes.cpp:30
+return llvm::None;
+ QualType T = VD->getType().getCanonicalType().getNonReferenceType();
+ if (!T->isFunctionType())
nit: is canonicalization necessary here? you do it in toEquivClass
(I guess dropping references is, for the function type check)
Comment at: clangd/ExpectedTypes.cpp:33
+return T;
+ // Functions are a special case. They are completed as 'foo()' and we want to
+ // match their return type rather than the function type itself.
nit: I'd put the special case in the if() block, but up to you
Comment at: clangd/ExpectedTypes.cpp:37
+ // after the function name, e.g. `std::cout << std::endl`.
+ return T->getAs()->getReturnType().getNonReferenceType();
+}
dropping references seems redundant here, as you do it again later
Comment at: clangd/ExpectedTypes.cpp:46
+ llvm::SmallString<128> Out;
+ if (index::generateUSRForType(T, Ctx, Out))
+return llvm::None;
I think ultimately we may want to replace this with a custom walker:
- we may want to ignore attributes (e.g. const) or bail out in some cases
- generateUSRForType may not have the exact semantics we want for other random
reasons
- we can do tricks with hash_combine to avoid actually building huge strings
we don't care about
not something for this patch, but maybe a FIXME?
Comment at: clangd/ExpectedTypes.cpp:71
+return llvm::None;
+ T = toEquivClass(Ctx, *T);
+ if (!T)
can you reuse fromPreferredType for the rest?
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 {
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.
Comment at: unittests/clangd/ExpectedTypeTest.cpp:79
+ << (Matched ? "matched" : "did not match")
+ << "\n\tTarget type: " << To.getAsString()
+ << "\n\tSource value type: " << V->getType().getAsString();
note that if you think it's useful you can To.dump(*L->stream())
Maybe this is more interesting if/when we have a custom visitor.
Comment at: unittests/clangd/ExpectedTypeTest.cpp:92
+
+TEST_F(ExpectedTypeConversionTest, BasicTypes) {
+ build(R"cpp(
I really like the declarative equivalence-class setup of the tests.
A couple of suggestions:
- maybe store the equivalence classes as groups of strings rather than decls,
and lazily grab the decls. It's easier to tersely represent them...
- I think the "convertibleTo" DSL obscures/abstracts the actual APIs you're
testing - they build opaque types, and you're asserting equality.
- pairwise assertion messages may not give enough context: if you expect a ==
b == c, and a != b, then whether a == c and b == c are probably relevant
I'd consider actually building up the equivalence classes `map>` and writing a `MATCHER_P2(ClassesAre,
/*vector>*/Classes, /*ParsedAST*/AST, "classes are " +
testing::PrintToString(Classes))`
That way the actual and expected equivalence classes will be dumped on failure,
and you can still grab the decls/types from the AST to dump their details.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D52273
[PATCH] D52273: [clangd] Initial implementation of expected types
sammccall added a comment. Forgot to say - the scope here looks just right, thanks for slimming this down! Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D52273 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52273: [clangd] Initial implementation of expected types
ilya-biryukov added a comment.
@ioeric, thanks for the review round!
Answering the most important comments, will shortly send changes to actually
address the rest.
Comment at: clangd/ExpectedTypes.cpp:40
+
+llvm::Optional encodeType(ASTContext , QualType T) {
+ assert(!T.isNull());
ioeric wrote:
> IIUC, we also encode the qualifiers into the final representation? If so,
> have you considered the underlying type without qualifiers? It seems to me
> this might be too restrictive for type-based boosting. For code completion
> ranking, I think type qualifiers (`const` etc) can be separate signals.
This function's responsibility is to encode the type. There is code to strip
the qualifiers from the types in `toEquivClass`.
The initial patch does not take qualifiers into account as none of the
complicated conversion logic (qualifiers were taken into account there) the
original patch had made much difference in the ranking measurements I made.
That said, this change does not aim to finalize the type encoding. I'll be
looking into improving the type-based ranking after this lands, might re-add
qualifiers if they turn out to be an improvement. Want to prove this with
measurements, though.
Comment at: clangd/ExpectedTypes.h:10
+// A simplified model of C++ types that can be used to check whether they are
+// convertible between each other without looking at the ASTs.
+// Used for code completion ranking.
ioeric wrote:
> We might want to formalize what "convertible" means here. E.g. does it cover
> conversion between base and derived class? Does it cover double <-> int
> conversion?
I want to leave it vague for now. Convertible means whatever we think is good
for code completion ranking.
Formalizing means we'll either dig into the C++ encoding or be imprecise.
Happy to add the docs, but they'll probably get outdated on every change.
Reading the code is actually simpler to get what's going on at this point.
Comment at: clangd/ExpectedTypes.h:37
+ fromCompletionResult(ASTContext , const CodeCompletionResult );
+ static llvm::Optional fromPreferredType(ASTContext ,
+ QualType Type);
ioeric wrote:
> why "preferred type"? maybe add a comment?
That's the terminology that clang uses for completion's context type. Will add
a comment, thanks!
Comment at: clangd/ExpectedTypes.h:40
+
+ /// Get the raw byte representation of the type. Bitwise equality of the raw
+ /// data is equivalent to equality operators of SType itself. The raw
ioeric wrote:
> What is the raw representation? A hash or the type name or USR?
A string representation of the usr, but users shouldn't rely on it.
The contract is: you can use it to compare for equality and nothing else, so
the comment is actually accurate :-)
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D52273
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52273: [clangd] Initial implementation of expected types
ioeric added inline comments.
Comment at: clangd/ExpectedTypes.cpp:27
+return llvm::None;
+ auto *VD = llvm::dyn_cast(R.Declaration);
+ if (!VD)
maybe add a comment what `ValueDecl` covers roughly? E.g. functions, classes,
variables etc.
Comment at: clangd/ExpectedTypes.cpp:40
+
+llvm::Optional encodeType(ASTContext , QualType T) {
+ assert(!T.isNull());
IIUC, we also encode the qualifiers into the final representation? If so, have
you considered the underlying type without qualifiers? It seems to me this
might be too restrictive for type-based boosting. For code completion ranking,
I think type qualifiers (`const` etc) can be separate signals.
Comment at: clangd/ExpectedTypes.h:10
+// A simplified model of C++ types that can be used to check whether they are
+// convertible between each other without looking at the ASTs.
+// Used for code completion ranking.
We might want to formalize what "convertible" means here. E.g. does it cover
conversion between base and derived class? Does it cover double <-> int
conversion?
Comment at: clangd/ExpectedTypes.h:29
+namespace clangd {
+/// An opaque representation of a type that can be computed based on clang AST
+/// and compared for equality. The encoding is stable between different ASTs,
The name seems opaque ;) Why is it `opaque`?
Comment at: clangd/ExpectedTypes.h:37
+ fromCompletionResult(ASTContext , const CodeCompletionResult );
+ static llvm::Optional fromPreferredType(ASTContext ,
+ QualType Type);
why "preferred type"? maybe add a comment?
Comment at: clangd/ExpectedTypes.h:40
+
+ /// Get the raw byte representation of the type. Bitwise equality of the raw
+ /// data is equivalent to equality operators of SType itself. The raw
What is the raw representation? A hash or the type name or USR?
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D52273
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52273: [clangd] Initial implementation of expected types
ilya-biryukov added a comment. In https://reviews.llvm.org/D52273#1294767, @malaperle wrote: > What is the goal for doing this without the AST? Is the goal to not have to > keep the AST and save memory? We don't have AST for index completions. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D52273 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52273: [clangd] Initial implementation of expected types
malaperle added a comment. What is the goal for doing this without the AST? Is the goal to not have to keep the AST and save memory? Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D52273 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52273: [clangd] Initial implementation of expected types
ilya-biryukov added a comment. I've run the measurements on a highly simplified vs the original complicated model and got roughly the same results wrt to ranking improvements, so sending a new version of the patch with highly simplified mode for the type representation. I believe there are still gains to be had from a more thorough treatment of C++ conversions, but there is definitely much to do in other areas that should provide better ground for seeing the actual improvements with the more complicated model. In any case, starting with something simple is definitely a better ground. Thanks for initial review and suggestions! And please take a look at the new version, it is significantly simpler and should be pretty easy to review :-) Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D52273 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52273: [clangd] Initial implementation of expected types
ilya-biryukov updated this revision to Diff 173337.
ilya-biryukov added a comment.
- Simplify the initial implementation
- Rename SType to OpaqueType
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D52273
Files:
clangd/CMakeLists.txt
clangd/ExpectedTypes.cpp
clangd/ExpectedTypes.h
unittests/clangd/CMakeLists.txt
unittests/clangd/ExpectedTypeTest.cpp
Index: unittests/clangd/ExpectedTypeTest.cpp
===
--- /dev/null
+++ unittests/clangd/ExpectedTypeTest.cpp
@@ -0,0 +1,191 @@
+//===-- ExpectedTypeTest.cpp ---*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "ClangdUnit.h"
+#include "ExpectedTypes.h"
+#include "TestTU.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/AST/Decl.h"
+#include "clang/AST/Type.h"
+#include "llvm/ADT/StringRef.h"
+#include "gmock/gmock-matchers.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+namespace clang {
+namespace clangd {
+namespace {
+
+using ::testing::ElementsAre;
+using ::testing::Field;
+using ::testing::Matcher;
+using ::testing::UnorderedElementsAre;
+using ::testing::UnorderedElementsAreArray;
+
+class ASTTest : public ::testing::Test {
+protected:
+ void build(llvm::StringRef Code) {
+assert(!AST && "AST built twice");
+AST = TestTU::withCode(Code).build();
+ }
+
+ const ValueDecl *decl(llvm::StringRef Name) {
+return ::cast(findDecl(*AST, Name));
+ }
+
+ QualType typeOf(llvm::StringRef Name) {
+return decl(Name)->getType().getCanonicalType();
+ }
+
+ ASTContext () { return AST->getASTContext(); }
+
+private:
+ // Set after calling build().
+ llvm::Optional AST;
+};
+
+class ConvertibleToMatcher
+: public ::testing::MatcherInterface {
+ ASTContext
+ QualType To;
+ OpaqueType PreferredType;
+
+public:
+ ConvertibleToMatcher(ASTContext , QualType To)
+ : Ctx(Ctx), To(To.getCanonicalType()),
+PreferredType(*OpaqueType::fromPreferredType(Ctx, To)) {}
+
+ void DescribeTo(std::ostream *OS) const override {
+*OS << "Is convertible to type '" << To.getAsString() << "'";
+ }
+
+ bool MatchAndExplain(const ValueDecl *V,
+ ::testing::MatchResultListener *L) const override {
+assert(V);
+assert(>getASTContext() == && "different ASTs?");
+auto ConvertibleTo = OpaqueType::fromCompletionResult(
+Ctx, CodeCompletionResult(V, CCP_Declaration));
+
+bool Matched = PreferredType == ConvertibleTo;
+if (L->IsInterested())
+ *L << "Types of source and target "
+ << (Matched ? "matched" : "did not match")
+ << "\n\tTarget type: " << To.getAsString()
+ << "\n\tSource value type: " << V->getType().getAsString();
+return Matched;
+ }
+};
+
+class ExpectedTypeConversionTest : public ASTTest {
+protected:
+ Matcher isConvertibleTo(QualType To) {
+return ::testing::MakeMatcher(new ConvertibleToMatcher(ASTCtx(), To));
+ }
+};
+
+TEST_F(ExpectedTypeConversionTest, BasicTypes) {
+ build(R"cpp(
+// ints.
+bool b;
+int i;
+unsigned int ui;
+long long ll;
+
+// floats.
+float f;
+double d;
+
+// pointers
+int* iptr;
+bool* bptr;
+
+// user-defined types.
+struct X {};
+X user_type;
+ )cpp");
+
+ const ValueDecl *Ints[] = {decl("b"), decl("i"), decl("ui"), decl("ll")};
+ const ValueDecl *Floats[] = {decl("f"), decl("d")};
+ const ValueDecl *IntPtr = decl("iptr");
+ const ValueDecl *BoolPtr = decl("bptr");
+ const ValueDecl *UserType = decl("user_type");
+
+ // Split all decls into equivalence groups. Decls inside the same equivalence
+ // group contain items that are convertible between each other.
+ llvm::ArrayRef EquivGroups[] = {
+ Ints, Floats, llvm::makeArrayRef(IntPtr), llvm::makeArrayRef(BoolPtr),
+ llvm::makeArrayRef(UserType)};
+
+ // Checks decls inside the same equivalence class can convert to each other.
+ for (auto Group : EquivGroups) {
+for (auto Decl : Group) {
+ for (auto OtherDecl : Group)
+EXPECT_THAT(Decl, isConvertibleTo(OtherDecl->getType()));
+}
+ }
+
+ // Checks items from different equivalence classes are not convertible between
+ // each other.
+ for (auto Group : EquivGroups) {
+for (auto OtherGroup : EquivGroups) {
+ if (Group.data() == OtherGroup.data())
+continue;
+
+ for (auto Decl1 : Group) {
+for (auto Decl2 : OtherGroup) {
+ if (Decl1 == Decl2)
+continue;
+ EXPECT_THAT(Decl1, Not(isConvertibleTo(Decl2->getType(;
+}
+ }
+}
+ }
+}
+
+TEST_F(ExpectedTypeConversionTest, FunctionReturns) {
+ build(R"cpp(
+ int returns_int();
+ int* returns_ptr();
+
+ int int_;
+
[PATCH] D52273: [clangd] Initial implementation of expected types
sammccall added a comment. Happy to speculate about what might work here, but I strongly believe the path forward here is to build the simplest version of this feature, without conversions, and try to avoid complicated conversion logic if we can get most of the benefit in simpler ways. In https://reviews.llvm.org/D52273#1243652, @ilya-biryukov wrote: > > This seems very clever, but extremely complicated - you've implemented much > > of C++'s conversion logic, it's not clear to me which parts are actually > > necessary to completion quality. > > Clearly the model that supports C++ conversions is something that **will** > improve code completion quality. It's not clear that will be significant. This isn't hard to measure, so I'm not sure why we should guess. And I'm not sure why it all has to go in the first patch. > I do agree it's not trivial, but would argue we at least want: > > - qualification conversions (i.e. adding const) Another approach here is just always dropping const. (And refs, and so on). This will create some false positives, but maybe they don't hurt much. This handles some true cases too, like invoking copy constructors. > - user-defined conversions (e.g. operator bool is commonly useful think) My **guess** is you're not going to measure a difference here, bool has lots of false positives and others are rare. > - derived-to-base conversions (Derived* should convert to Base*) Yes, probably. If this ends up being the only "chain" we have to follow, we're probably in good shape complexity-wise. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D52273 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52273: [clangd] Initial implementation of expected types
ilya-biryukov added inline comments. Comment at: clangd/ExpectedTypes.h:68 + +/// Represents a type of partially applied conversion. Should be treated as an +/// opaque value and can only be used to check whether the types are converible sammccall wrote: > this represents a type (in the c++ sense), not a conversion, right? It's an "expression" with an extra data with some extra data (whether the user conversion was applied to get this expression) Comment at: clangd/ExpectedTypes.h:82 + static llvm::SmallVector + fromCompletionResult(ASTContext , const CodeCompletionResult ); + sammccall wrote: > coupling to CompletionResult seems premature here, can we stick to passing > getExpectedType() until we know that abstraction needs to be broken? There's some useful logic that is tied to completion results, e.g. to extract function return type `CompletionResult`. Happy to accept a decl, but would keep the name `fromCompletionResult`. Does that LG? Comment at: clangd/ExpectedTypes.h:213 + +void collectConvertibleFrom(ASTContext , MockExpr Source, +llvm::function_ref OutF); sammccall wrote: > sammccall wrote: > > why is implementing one of these directions not enough? > > > > It should probably be: > > As far as I can tell, derived-to-base is the tricky one here: it's an > > important conversion (albeit one we should leave out of the first patch), > > and you can't ask "what's convertible to base" since the answer is an open > > set you can't see. > > > > So it seems the minimal set you need for handling pointer to base is `Type > > getRepresentative(Type)` and `set > > getRepresentativesAfterConversion(Type)` or so... > names are unclear: is `collectConvertibleFrom(T)` the convertible-from types > for T (i.e the types T is convertible from), or the types that are > convertible from T? Derived-to-base and user conversions. We can't enumerate all derived classes for some type, so instead need to enumerate all bases when adding a symbol to the index. We can't enumerate all types that have user-defined conversions to some type T, so we need to enumerate all user-defined conversions when adding a symbol instead. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D52273 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52273: [clangd] Initial implementation of expected types
ilya-biryukov added a comment.
> This seems very clever, but extremely complicated - you've implemented much
> of C++'s conversion logic, it's not clear to me which parts are actually
> necessary to completion quality.
Clearly the model that supports C++ conversions is something that **will**
improve code completion quality.
I do agree it's not trivial, but would argue we at least want:
- qualification conversions (i.e. adding const)
- user-defined conversions (e.g. operator bool is commonly useful think)
- derived-to-base conversions (Derived* should convert to Base*)
Without those, we don't support a bunch of useful cases.
> As chatted offline, I think the return type can be split into multiple
> orthogonal signals. For example, const T & can be split into 3 independent
> signals {const, type T, reference}. I think this can make the reasoning of
> boosting/scoring easier for both index and code completion. Agree with Sam
> that we should start with something simple (e.g. type matching without
> conversing) and land basic components to make further evaluation possible.
Yeah, I do keep it in mind and I think it's a great idea. E.g., we can put all
numeric types into one equivalence class and get rid of all numeric conversions.
That adds some complexity to the interface, though, I wanted to measure how the
trivial solution (enumerate all types) works. To make sure we actually can't
get away without it.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D52273
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52273: [clangd] Initial implementation of expected types
ioeric added a comment.
In https://reviews.llvm.org/D52273#1241281, @sammccall wrote:
> This seems very clever, but extremely complicated - you've implemented much
> of C++'s conversion logic, it's not clear to me which parts are actually
> necessary to completion quality.
> (Honestly this applies to expected types overall - it seems intuitively
> likely that it's a good signal, it seems less obvious that it pulls its
> weight if it can't be made simple).
>
> From the outside it seems much of it is YAGNI, and if we do then we need to
> build it up slowly with an eye for maintainability.
> Can we start with expected type boosting (no conversions) as previously
> discussed, and later measure which other parts make a difference? (I think
> we'll need/want the simple model anyway, for this to work with Dex and other
> token-based indexes).
+1 to a simpler model.
As chatted offline, I think the return type can be split into multiple
orthogonal signals. For example, `const T &` can be split into 3 independent
signals {`const`, `type T`, `reference`}. I think this can make the reasoning
of boosting/scoring easier for both index and code completion. Agree with Sam
that we should start with something simple (e.g. type matching without
conversing) and land basic components to make further evaluation possible.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D52273
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52273: [clangd] Initial implementation of expected types
sammccall added a comment.
This seems very clever, but extremely complicated - you've implemented much of
C++'s conversion logic, it's not clear to me which parts are actually necessary
to completion quality.
(Honestly this applies to expected types overall - it seems intuitively likely
that it's a good signal, it seems less obvious that it pulls its weight if it
can't be made simple).
From the outside it seems much of it is YAGNI, and if we do then we need to
build it up slowly with an eye for maintainability.
Can we start with expected type boosting (no conversions) as previously
discussed, and later measure which other parts make a difference? (I think
we'll need/want the simple model anyway, for this to work with Dex and other
token-based indexes).
Comment at: clangd/ExpectedTypes.h:66
+using SHA1Array = std::array;
+SHA1Array computeSHA1(llvm::StringRef Input);
+
While a hash of a string might be a reasonable choice in the long term, I worry
about debuggability. (With SymbolID we can just look up the symbol).
You could make the hashing an implementation detail of the index, and have the
APIs speak in terms of opaque strings. But that forces the index to be able to
report the full opaque string of each returned symbol (for scoring), so the
index now has to have a lookup table... messy.
Another fun thing about this representation is that you're storing 20 bytes of
data (+ overhead) for common types like "void" where we could get away with one.
Comment at: clangd/ExpectedTypes.h:66
+using SHA1Array = std::array;
+SHA1Array computeSHA1(llvm::StringRef Input);
+
sammccall wrote:
> While a hash of a string might be a reasonable choice in the long term, I
> worry about debuggability. (With SymbolID we can just look up the symbol).
>
> You could make the hashing an implementation detail of the index, and have
> the APIs speak in terms of opaque strings. But that forces the index to be
> able to report the full opaque string of each returned symbol (for scoring),
> so the index now has to have a lookup table... messy.
>
> Another fun thing about this representation is that you're storing 20 bytes
> of data (+ overhead) for common types like "void" where we could get away
> with one.
in the *short run* I'd suggest just printing the type name and using that as
the representation.
I'm happy to (eventually) learn about the semantics of USRs in types, but not
today :-)
Comment at: clangd/ExpectedTypes.h:68
+
+/// Represents a type of partially applied conversion. Should be treated as an
+/// opaque value and can only be used to check whether the types are converible
this represents a type (in the c++ sense), not a conversion, right?
Comment at: clangd/ExpectedTypes.h:69
+/// Represents a type of partially applied conversion. Should be treated as an
+/// opaque value and can only be used to check whether the types are converible
+/// between each other (by using the equality operator).
"convertible (using equality)" is confusing.
It sounds like "this is actually an equivalence class of types" but I think
that's not true, because it's not symmetric.
Isn't the model here just "SType is a serializable token representing a type.
They can be compared for equality."
Comment at: clangd/ExpectedTypes.h:72
+/// Representation is fixed-size, small and cheap to copy.
+class SType {
+public:
Is this a placeholder name? It's not clear what it means.
Suggest OpaqueType or ExpectedType
Comment at: clangd/ExpectedTypes.h:81
+ /// implementation attempts to store as little types as possible.
+ static llvm::SmallVector
+ fromCompletionResult(ASTContext , const CodeCompletionResult );
can we separate "get the representative set of types for R" from "encode them
as SType"?
Seems like the APIs would be easier to test and understand.
(I think at least the former should be a non-member function BTW, to keep clear
that SType itself isn't aware of any clever folding or whatnot)
Comment at: clangd/ExpectedTypes.h:82
+ static llvm::SmallVector
+ fromCompletionResult(ASTContext , const CodeCompletionResult );
+
coupling to CompletionResult seems premature here, can we stick to passing
getExpectedType() until we know that abstraction needs to be broken?
Comment at: clangd/ExpectedTypes.h:91
+ ///
+ /// The result is a map from a type to a multiplier (>= 1) that denotes the
+ /// quality of conversion that had to be applied (better conversion receive
I don't understand the scale here. If better conversions get higher numbers,
what number does "no conversion" get?
The code looks like worse conversions get higher numbers.
I'd suggest using an additive penalty to avoid confusion with scores, but
really...
this all
[PATCH] D52273: [clangd] Initial implementation of expected types
ilya-biryukov added inline comments. Comment at: clangd/ExpectedTypes.h:119 + explicit SType(SHA1Array Data); + SHA1Array Data; +}; I assume this will be controversial. Happy to discuss/change. We are currently building this representation based on USRs for types, the alternative is to store the USRs directly. Would be a bit more debuggable/explainable in case of failures, but also not particularly readable. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D52273 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52273: [clangd] Initial implementation of expected types
ilya-biryukov added a comment. The implementation might look a bit scary, please feel free to ask for comments/clarifications! Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D52273 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52273: [clangd] Initial implementation of expected types
ilya-biryukov created this revision.
ilya-biryukov added reviewers: sammccall, ioeric.
Herald added subscribers: kadircet, arphaman, jkorous, MaskRay, mgorny.
Provides facilities to model the C++ conversion rules without the AST.
The introduced representation can be stored in the index and used to
implement type-based ranking improvements for index-based completions.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D52273
Files:
clangd/CMakeLists.txt
clangd/ExpectedTypes.cpp
clangd/ExpectedTypes.h
unittests/clangd/CMakeLists.txt
unittests/clangd/ExpectedTypeTest.cpp
Index: unittests/clangd/ExpectedTypeTest.cpp
===
--- /dev/null
+++ unittests/clangd/ExpectedTypeTest.cpp
@@ -0,0 +1,475 @@
+//===-- SimpleTypeTests.cpp *- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "ClangdUnit.h"
+#include "ExpectedTypes.h"
+#include "TestTU.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/AST/Decl.h"
+#include "clang/AST/Type.h"
+#include "llvm/ADT/SmallVector.h"
+#include "llvm/ADT/StringRef.h"
+#include "gmock/gmock-matchers.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+namespace clang {
+namespace clangd {
+namespace {
+
+using detail::MockExpr;
+using detail::PartialConv;
+using detail::ValueCategory;
+
+using ::testing::ElementsAre;
+using ::testing::Field;
+using ::testing::Matcher;
+using ::testing::UnorderedElementsAre;
+using ::testing::UnorderedElementsAreArray;
+
+class ASTTest : public ::testing::Test {
+protected:
+ void build(llvm::StringRef Code) {
+assert(!AST && "AST built twice");
+AST = TestTU::withCode(Code).build();
+ }
+
+ const ValueDecl *decl(llvm::StringRef Name) {
+return ::cast(findDecl(*AST, Name));
+ }
+
+ QualType typeOf(llvm::StringRef Name) {
+return decl(Name)->getType().getCanonicalType();
+ }
+
+ ASTContext () { return AST->getASTContext(); }
+
+private:
+ // Set after calling build().
+ llvm::Optional AST;
+};
+
+class ExpectedTypeCollectorTest : public ASTTest {
+protected:
+ std::vector convertibleTo(QualType To) {
+std::vector Result;
+detail::collectConvertibleTo(ASTCtx(), To,
+ [&](PartialConv C) { Result.push_back(C); });
+return Result;
+ }
+
+ std::vector convertibleFrom(const NamedDecl *D) {
+std::vector Result;
+detail::collectConvertibleFrom(
+ASTCtx(),
+*MockExpr::forCompletion(CodeCompletionResult(D, CCP_Declaration)),
+[&](PartialConv C) { Result.push_back(C); });
+return Result;
+ }
+};
+
+// Matchers for l-values and r-values, which don't come from user-defined
+// conversions.
+MATCHER_P(lv, TypeStr, "") {
+ return arg.Cat == ValueCategory::LVal && arg.Type.getAsString() == TypeStr;
+}
+MATCHER_P(rv, TypeStr, "") {
+ return arg.Cat == ValueCategory::RVal && arg.Type.getAsString() == TypeStr;
+}
+
+Matcher converted(Matcher M) {
+ return AllOf(M, Field(::AfterUserConv, true));
+}
+
+std::vector>
+alsoConverted(std::vector> Matchers) {
+ std::vector> Result;
+ Result.reserve(Matchers.size() * 2);
+ for (auto M : Matchers) {
+Result.push_back(M);
+Result.push_back(converted(M));
+ }
+ return Result;
+}
+
+template
+Matcher> stdConversions(StrT... TypeStrs) {
+ return UnorderedElementsAreArray(
+ alsoConverted({lv(TypeStrs)..., rv(TypeStrs)...}));
+}
+
+std::vector> concat(std::vector> L,
+ std::vector> R) {
+ L.reserve(L.size() + R.size());
+ L.insert(L.end(), R.begin(), R.end());
+ return L;
+}
+
+TEST_F(ExpectedTypeCollectorTest, NumericTypes) {
+ build(R"cpp(
+bool b;
+int i;
+unsigned int ui;
+long long ll;
+float f;
+double d;
+ )cpp");
+
+ EXPECT_THAT(convertibleTo(decl("i")->getType()),
+ stdConversions("int", "float", "const int", "const float"));
+ EXPECT_THAT(convertibleFrom(decl("i")), UnorderedElementsAre(lv("int")));
+
+ const ValueDecl *Ints[] = {decl("b"), decl("ui"), decl("ll")};
+ for (const auto *D : Ints) {
+std::string DType = D->getType().getAsString();
+EXPECT_THAT(
+convertibleTo(D->getType()),
+stdConversions(DType, "int", "float", "const int", "const float"));
+EXPECT_THAT(convertibleFrom(D), UnorderedElementsAre(lv(DType), rv("int")));
+ }
+
+ // Check float.
+ EXPECT_THAT(convertibleTo(decl("f")->getType()),
+ stdConversions("int", "float", "const int", "const float"));
+ EXPECT_THAT(convertibleFrom(decl("f")), UnorderedElementsAre(lv("float")));
+ // Check double.
+ EXPECT_THAT(
+ convertibleTo(decl("d")->getType()),
+ stdConversions("int", "double", "float", "const int", "const float"));
+
