Avoid printing some redundant name qualifiers in completion

[ilya-biryukov]

Summary

Adjusted PrintingPolicy inside code completion to avoid printing some
redundant name qualifiers.

Before this change, typedefs that were written unqualified in source
code were printed with qualifiers in completion. For example, in the
following code

struct foo {
    typedef int type;
    type method();
};

completion item for method had return type of foo::type, even
though the original code used type without qualifiers.
After this change, the completion item has return type type, as
originally written in the source code.

Note that this change does not suppress qualifiers written by the
user. For example, in the following code

typedef int type;
struct foo {
    typedef int type;
    ::type method(foo::type);
};

completion item for method has return type of ::type and
parameter type of foo::type, as originally written in the source
code.

[ilya-biryukov]

test/CodeCompletion/enum-switch-case-qualified.cpp

25 ↗

(On Diff #117662)

This may be a somewhat unwanted part of this change.
Enum type is now written without qualifier here. I would argue that's ok, since the actual enum values are always properly qualified (they have to be, as they are actually inserted by completion) and those qualifiers provide all the necessary context for the user.

[arphaman]

test/CodeCompletion/call.cpp

22 ↗

(On Diff #117662)

Could we also test a similar class to Y that's not typedefed, so you'd see f(N::Y)?

test/CodeCompletion/enum-switch-case-qualified.cpp

25 ↗

(On Diff #117662)

I'm not 100% comfortable with making this kind of change right now. I'll try to investigate what's best for our users.

[ilya-biryukov]

• Restore qualifiers for types of EnumConstantDecl.

[ilya-biryukov]

test/CodeCompletion/enum-switch-case-qualified.cpp

25 ↗

(On Diff #117662)

I changed code to keep qualifiers for enums as is.
(This required moving some code from libTooling to AST in order to reuse it, I'll create a separate review for that if you're ok with the overall change).

[ilya-biryukov]

• Included more cases into a qualifiers-as-written.cpp test.

[ilya-biryukov]

test/CodeCompletion/call.cpp

22 ↗

(On Diff #117662)

Not sure if you expect this to behave differently, but I've updated qualifiers-as-written.cpp to include a similar case. You could take a look and validate whether it matches your expected behavior.

The general idea is to not add extra qualification in completion items, where possible, and prefer to show what was written in the source code.
This is what currently happens for all names that had name-qualifiers. However, before this patch, unqualified names would printed with an added name qualifiers.

This sometimes led to really bad results. One example is vector's push_back that roughly gets printed as (libstdc++ version) push_back(std::vector<int, std::allocator<int>>::value_type _Value). Whereas in the source code it's just push_back(value_type _Value).

[arphaman]

test/CodeCompletion/qualifiers-as-written.cpp

29 ↗

(On Diff #118205)

Sorry, I see the issue now. However, I don't think that we'd like to change the signature for a function like this, as we'd still prefer to show func (foo::type, ns::bar, ns::baz); on this line.

In Xcode we actually avoid the problem with std::vector<>s that you've pointed out entirely by using value_type. I'll check what our solution does.

Btw, maybe using things like value_type is completely wrong (with or without the qualifier)? If we have std::vector<int> shouldn't we rather show push_back(int _Value), rather than the value_type? Perhaps this kind of change should be discussed with a wider community somehow to find out what's best for all users.

[ilya-biryukov]

test/CodeCompletion/qualifiers-as-written.cpp

29 ↗

(On Diff #118205)

Using int _Value may be good in case of vector<int>, but it would probably loose typedefs, i.e. vector<int32> would still have int, which may be undesirable.
Also, for vector<vector<int>>, the type inside push_back would probably end up being vector<int, std::allocator<int>>.

As for the current vs new behavior, I can totally see why you want to see more context in completion items.

I would argue that the current code does not do a great job there, as it only adds qualifiers to unqualified references. But the context may be missing for qualified references as well.
For example, in the following code:

template <class T, class Alloc = std::allocator<T>>
struct vector {
  typedef T value_type;

  typename value_type::some_type foo();
  value_type bar()
};

Completion item for vector<X>::bar will have return type vector<X, std::allocator<X>>::value_type. However, completion item for vector<X, std::allocator<X>>::foo will have return type value_type::some_type (note that no vector<X, std::allocator<X>> qualifier was added).

I would also question the intent of the current implementation. The following line suggest that adding those qualifers was not intended in the first place:

Policy.SuppressUnwrittenScope = true;

But that's just a wild guess, I may be completely wrong.

That being said, I don't think there is one right preference, different people may prefer different options. We can surely discuss that in a wider community, though.

Would you be open to adding an option for that in completion and keeping the current behavior as a default?

[ilya-biryukov]

Friendly ping.
Would it be okay to make the new behaviour optional? (Leaving old behaviour to be the default).

[arphaman]

test/CodeCompletion/qualifiers-as-written.cpp

29 ↗

(On Diff #118205)

Sorry about the delay.

I clarified Xcode's behavior - we actually didn't show the qualifiers for typedefs and typealiases because of an older Clang, with ToT the behavior is as you describe.

So this patch will remove qualifiers for structs and classes, like in the func example here. I'm willing to accept it right now. We might have to revisit this change at some point though. Let's also keep one behavior right now, I don't think we need to diverge yet.

[ilya-biryukov]

• Extracted the move of QualTypeNames to AST into a separate revision.

cfe/trunk/lib/Sema/SemaCodeComplete.cpp

 //===---------------- SemaCodeComplete.cpp - Code Completion ----*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 //
 //  This file defines the code-completion semantic actions.
 //
 //===----------------------------------------------------------------------===//
 #include "clang/AST/DeclCXX.h"
 #include "clang/AST/DeclObjC.h"
 #include "clang/AST/ExprCXX.h"
 #include "clang/AST/ExprObjC.h"
+#include "clang/AST/QualTypeNames.h"
 #include "clang/Basic/CharInfo.h"
 #include "clang/Lex/HeaderSearch.h"
 #include "clang/Lex/MacroInfo.h"
 #include "clang/Lex/Preprocessor.h"
 #include "clang/Sema/CodeCompleteConsumer.h"
 #include "clang/Sema/Lookup.h"
 #include "clang/Sema/Overload.h"
 #include "clang/Sema/Scope.h"

 }
 
 static PrintingPolicy getCompletionPrintingPolicy(const ASTContext &Context,
                                                   const Preprocessor &PP) {
   PrintingPolicy Policy = Sema::getPrintingPolicy(Context, PP);
   Policy.AnonymousTagLocations = false;
   Policy.SuppressStrongLifetime = true;
   Policy.SuppressUnwrittenScope = true;
+  Policy.SuppressScope = true;
   return Policy;
 }
 
 /// \brief Retrieve a printing policy suitable for code completion.
 static PrintingPolicy getCompletionPrintingPolicy(Sema &S) {
   return getCompletionPrintingPolicy(S.Context, S.PP);
 }
 
static void AddResultTypeChunk(ASTContext &Context,
   QualType T;
   if (const FunctionDecl *Function = ND->getAsFunction())
     T = Function->getReturnType();
   else if (const ObjCMethodDecl *Method = dyn_cast<ObjCMethodDecl>(ND)) {
     if (!BaseType.isNull())
       T = Method->getSendResultType(BaseType);
     else
       T = Method->getReturnType();
-  } else if (const EnumConstantDecl *Enumerator = dyn_cast<EnumConstantDecl>(ND))
+  } else if (const EnumConstantDecl *Enumerator = dyn_cast<EnumConstantDecl>(ND)) {
     T = Context.getTypeDeclType(cast<TypeDecl>(Enumerator->getDeclContext()));
-  else if (isa<UnresolvedUsingValueDecl>(ND)) {
+    T = clang::TypeName::getFullyQualifiedType(T, Context);
+  } else if (isa<UnresolvedUsingValueDecl>(ND)) {
     /* Do nothing: ignore unresolved using declarations*/
   } else if (const ObjCIvarDecl *Ivar = dyn_cast<ObjCIvarDecl>(ND)) {
     if (!BaseType.isNull())
       T = Ivar->getUsageType(BaseType);
     else
       T = Ivar->getType();
   } else if (const ValueDecl *Value = dyn_cast<ValueDecl>(ND)) {
     T = Value->getType();

cfe/trunk/test/CodeCompletion/call.cpp

 }
 typedef N::Y Y;
 void f();
 
 void test() {
   f(Y(), 0, 0);
   // RUN: %clang_cc1 -fsyntax-only -code-completion-patterns -code-completion-at=%s:19:9 %s -o - | FileCheck -check-prefix=CHECK-CC1 %s
   // CHECK-CC1: COMPLETION: Pattern : dynamic_cast<<#type#>>(<#expression#>)
-  // CHECK-CC1: f(N::Y y, <#int ZZ#>)
+  // CHECK-CC1: f(Y y, <#int ZZ#>)
   // CHECK-CC1-NEXT: f(int i, <#int j#>, int k)
   // CHECK-CC1-NEXT: f(float x, <#float y#>)
   // RUN: %clang_cc1 -fsyntax-only -code-completion-at=%s:19:13 %s -o - | FileCheck -check-prefix=CHECK-CC2 %s
-  // CHECK-CC2-NOT: f(N::Y y, int ZZ)
+  // CHECK-CC2-NOT: f(Y y, int ZZ)
   // CHECK-CC2: f(int i, int j, <#int k#>)
 }

cfe/trunk/test/CodeCompletion/qualifiers-as-written.cpp

+struct foo {
+  typedef int type;
+
+  type method(type, foo::type, ::foo::type, ::foo::foo::type);
+};
+
+namespace ns {
+  struct bar {
+  };
+
+  struct baz {
+  };
+
+  int func(foo::type a, bar b, baz c);
+}
+
+typedef ns::bar bar;
+
+int func(foo a, bar b, ns::bar c, ns::baz d);
+using ns::func;
+
+void test() {
+  foo().method(0, 0, 0, 0);
+  // RUN: %clang_cc1 -fsyntax-only -code-completion-at=%s:23:9 %s -o - | FileCheck %s --check-prefix=CHECK-1
+  // CHECK-1: COMPLETION: method : [#type#]method(<#type#>, <#foo::type#>, <#::foo::type#>, <#::foo::foo::type#>)
+  f
+  // RUN: %clang_cc1 -fsyntax-only -code-completion-at=%s:26:3 %s -o - | FileCheck %s --check-prefix=CHECK-2
+  // CHECK-2: COMPLETION: func : [#int#]func(<#foo a#>, <#bar b#>, <#ns::bar c#>, <#ns::baz d#>
+  // CHECK-2: COMPLETION: func : [#int#]func(<#foo::type a#>, <#bar b#>, <#baz c#>
+}

cfe/trunk/test/CodeCompletion/uninstantiated_params.cpp

 template <class T>
 struct unique_ptr {
   typedef T* pointer;
 
   void reset(pointer ptr = pointer());
 };
 
 void test() {
   unique_ptr<int> x;
   x.
   // RUN: %clang_cc1 -fsyntax-only -code-completion-at=%s:10:5 %s -o - | FileCheck -check-prefix=CHECK-CC1 %s
-  // CHECK-CC1: [#void#]reset({#<#unique_ptr<int>::pointer ptr = pointer()#>#})
+  // CHECK-CC1: [#void#]reset({#<#pointer ptr = pointer()#>#})
 }

cfe/trunk/test/Index/complete-cxx-inline-methods.cpp

 private:
   int value;
   MyCls *object;
 };
 }
 
 // RUN: c-index-test -code-completion-at=%s:4:9 -std=c++98 %s | FileCheck %s
 // RUN: c-index-test -code-completion-at=%s:13:7 -std=c++98 %s | FileCheck %s
-// CHECK:      CXXMethod:{ResultType MyCls::Vec &}{TypedText operator=}{LeftParen (}{Placeholder const MyCls::Vec &}{RightParen )} (79)
+// CHECK:      CXXMethod:{ResultType Vec &}{TypedText operator=}{LeftParen (}{Placeholder const Vec &}{RightParen )} (79)
 // CHECK-NEXT: StructDecl:{TypedText Vec}{Text ::} (75)
 // CHECK-NEXT: FieldDecl:{ResultType int}{TypedText x} (35)
 // CHECK-NEXT: FieldDecl:{ResultType int}{TypedText y} (35)
 // CHECK-NEXT: CXXDestructor:{ResultType void}{TypedText ~Vec}{LeftParen (}{RightParen )} (79)
 // CHECK-NEXT: Completion contexts:
 // CHECK-NEXT: Dot member access
 // CHECK-NEXT: Container Kind: StructDecl
 
 // RUN: c-index-test -code-completion-at=%s:18:41 %s | FileCheck -check-prefix=CHECK-CTOR-INIT %s
 // CHECK-CTOR-INIT: NotImplemented:{TypedText MyCls}{LeftParen (}{Placeholder args}{RightParen )} (7)
 // CHECK-CTOR-INIT: MemberRef:{TypedText object}{LeftParen (}{Placeholder args}{RightParen )} (35)
 // CHECK-CTOR-INIT: MemberRef:{TypedText value}{LeftParen (}{Placeholder args}{RightParen )} (35)
 // RUN: c-index-test -code-completion-at=%s:18:55 %s | FileCheck -check-prefix=CHECK-CTOR-INIT-2 %s
 // CHECK-CTOR-INIT-2-NOT: NotImplemented:{TypedText MyCls}{LeftParen (}{Placeholder args}{RightParen )}
 // CHECK-CTOR-INIT-2: MemberRef:{TypedText object}{LeftParen (}{Placeholder args}{RightParen )} (35)
 // CHECK-CTOR-INIT-2: MemberRef:{TypedText value}{LeftParen (}{Placeholder args}{RightParen )} (7)