[SemaCodeComplete] Allow passing out scope specifiers in qualified-id completions via completion context.

[ilya-biryukov]

include/clang/Sema/CodeCompleteConsumer.h

284 ↗

(On Diff #124584)

Maybe add a brief comment for consistency with other decls?

lib/Sema/SemaCodeComplete.cpp

4609 ↗

(On Diff #124584)

Why do we alter this code path?

4609 ↗

(On Diff #124584)

Maybe we should add a test or provide examples why the current code does not work for us?

4611 ↗

(On Diff #124584)

Do we really want to set invalid scope specifiers here?

[ioeric]

• Address review comments.

lib/Sema/SemaCodeComplete.cpp

4609 ↗

(On Diff #124584)

There is no existing unit test for CodeCompletion, and there is no much to test here either. I added a comment with example for this.

4611 ↗

(On Diff #124584)

Yes, see the comment I added.

[arphaman]

Could you please add a test?

[ioeric]

In D40563#939555, @arphaman wrote:

Could you please add a test?

Any tip on how this should be tested? I couldn't find any existing unit test for either SemaCodeComplete or code completion context (under clang/unittests). It might be possible to set up a unit test framework for code completion, but I'm not sure if it's worth to do so since this change doesn't change completion behavior.

[arphaman]

If nothing uses getCXXScopeSpecifier right now we can't really test it with a clang or c-index-test regression test. A completion unit test could work here. I don't think we actually have existing completion unit tests though, so you would have to create one from scratch. But if getCXXScopeSpecifier will be used in a follow up patch maybe it will be easier to commit this without a test together with the followup patch?

[ilya-biryukov]

lib/Sema/SemaCodeComplete.cpp

4609 ↗

(On Diff #124584)

Makes sense, thanks for the explanation.

[ioeric]

In D40563#939964, @arphaman wrote:

If nothing uses getCXXScopeSpecifier right now we can't really test it with a clang or c-index-test regression test. A completion unit test could work here. I don't think we actually have existing completion unit tests though, so you would have to create one from scratch. But if getCXXScopeSpecifier will be used in a follow up patch maybe it will be easier to commit this without a test together with the followup patch?

I have another clangd patch that uses this, but this would still need to be a separate patch since they are in different repos...

[arphaman]

In D40563#940536, @ioeric wrote:

In D40563#939964, @arphaman wrote:

If nothing uses getCXXScopeSpecifier right now we can't really test it with a clang or c-index-test regression test. A completion unit test could work here. I don't think we actually have existing completion unit tests though, so you would have to create one from scratch. But if getCXXScopeSpecifier will be used in a follow up patch maybe it will be easier to commit this without a test together with the followup patch?

I have another clangd patch that uses this, but this would still need to be a separate patch since they are in different repos...

You can commit the two in one either using the monrepo or sparse SVN checkout ;)
But I wouldn't object if you commit this separately right before the clangd patch, so LGTM

[ioeric]

Fyi, you could find use of the new API in D40548

I'd like to land this patch if there is no objection.

cfe/trunk/include/clang/Sema/CodeCompleteConsumer.h

 #ifndef LLVM_CLANG_SEMA_CODECOMPLETECONSUMER_H
 #define LLVM_CLANG_SEMA_CODECOMPLETECONSUMER_H
 
 #include "clang-c/Index.h"
 #include "clang/AST/CanonicalType.h"
 #include "clang/AST/DeclBase.h"
 #include "clang/AST/Type.h"
 #include "clang/Sema/CodeCompleteOptions.h"
+#include "clang/Sema/DeclSpec.h"
 #include "llvm/ADT/DenseMap.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/Allocator.h"
 #include <string>
 #include <utility>
 
 namespace clang {
private:
   QualType PreferredType;
 
   /// \brief The type of the base object in a member access expression.
   QualType BaseType;
 
   /// \brief The identifiers for Objective-C selector parts.
   ArrayRef<IdentifierInfo *> SelIdents;
 
+  /// \brief The scope specifier that comes before the completion token e.g.
+  /// "a::b::"
+  llvm::Optional<CXXScopeSpec> ScopeSpecifier;
+
 public:
   /// \brief Construct a new code-completion context of the given kind.
   CodeCompletionContext(enum Kind Kind) : Kind(Kind), SelIdents(None) { }
 
   /// \brief Construct a new code-completion context of the given kind.
   CodeCompletionContext(enum Kind Kind, QualType T,
                         ArrayRef<IdentifierInfo *> SelIdents = None)
                         : Kind(Kind),
public:
   QualType getBaseType() const { return BaseType; }
 
   /// \brief Retrieve the Objective-C selector identifiers.
   ArrayRef<IdentifierInfo *> getSelIdents() const { return SelIdents; }
 
   /// \brief Determines whether we want C++ constructors as results within this
   /// context.
   bool wantConstructorResults() const;
-};
 
+  /// \brief Sets the scope specifier that comes before the completion token.
+  /// This is expected to be set in code completions on qualfied specifiers
+  /// (e.g. "a::b::").
+  void setCXXScopeSpecifier(CXXScopeSpec SS) {
+    this->ScopeSpecifier = std::move(SS);
+  }
+
+  llvm::Optional<const CXXScopeSpec *> getCXXScopeSpecifier() {
+    if (ScopeSpecifier)
+      return ScopeSpecifier.getPointer();
+    return llvm::None;
+  }
+};
 
 /// \brief A "string" used to describe how code completion can
 /// be performed for an entity.
 ///
 /// A code completion string typically shows how a particular entity can be
 /// used. For example, the code completion string for a function would show
 /// the syntax to call it, including the parentheses, placeholders for the
 /// arguments, etc.

cfe/trunk/lib/Sema/SemaCodeComplete.cpp

void Sema::CodeCompleteAssignmentRHS(Scope *S, Expr *LHS) {
   if (LHS)
     CodeCompleteExpression(S, static_cast<Expr *>(LHS)->getType());
   else
     CodeCompleteOrdinaryName(S, PCC_Expression);
 }
 
 void Sema::CodeCompleteQualifiedId(Scope *S, CXXScopeSpec &SS,
                                    bool EnteringContext) {
-  if (!SS.getScopeRep() || !CodeCompleter)
+  if (SS.isEmpty() || !CodeCompleter)
     return;
 
+  // We want to keep the scope specifier even if it's invalid (e.g. the scope
+  // "a::b::" is not corresponding to any context/namespace in the AST), since
+  // it can be useful for global code completion which have information about
+  // contexts/symbols that are not in the AST.
+  if (SS.isInvalid()) {
+    CodeCompletionContext CC(CodeCompletionContext::CCC_Name);
+    CC.setCXXScopeSpecifier(SS);
+    HandleCodeCompleteResults(this, CodeCompleter, CC, nullptr, 0);
+    return;
+  }
   // Always pretend to enter a context to ensure that a dependent type
   // resolves to a dependent record.
   DeclContext *Ctx = computeDeclContext(SS, /*EnteringContext=*/true);
   if (!Ctx)
     return;
 
   // Try to instantiate any non-dependent declaration contexts before
   // we look in them.
   if (!isDependentScopeSpecifier(SS) && RequireCompleteDeclContext(SS, Ctx))
     return;
 
   ResultBuilder Results(*this, CodeCompleter->getAllocator(),
                         CodeCompleter->getCodeCompletionTUInfo(),
                         CodeCompletionContext::CCC_Name);
   Results.EnterNewScope();
-  
   // The "template" keyword can follow "::" in the grammar, but only
   // put it into the grammar if the nested-name-specifier is dependent.
   NestedNameSpecifier *NNS = SS.getScopeRep();
   if (!Results.empty() && NNS->isDependent())
     Results.AddResult("template");
 
   // Add calls to overridden virtual functions, if there are any.
   //
   // FIXME: This isn't wonderful, because we don't know whether we're actually
   // in a context that permits expressions. This is a general issue with
   // qualified-id completions.
   if (!EnteringContext)
     MaybeAddOverrideCalls(*this, Ctx, Results);
-  Results.ExitScope();  
-  
+  Results.ExitScope();
   CodeCompletionDeclConsumer Consumer(Results, CurContext);
   LookupVisibleDecls(Ctx, LookupOrdinaryName, Consumer,
                      /*IncludeGlobalScope=*/true,
                      /*IncludeDependentBases=*/true);
 
-  HandleCodeCompleteResults(this, CodeCompleter, 
-                            Results.getCompletionContext(),
-                            Results.data(),Results.size());
+  auto CC = Results.getCompletionContext();
+  CC.setCXXScopeSpecifier(SS);
+
+  HandleCodeCompleteResults(this, CodeCompleter, CC, Results.data(),
+                            Results.size());
 }
 
 void Sema::CodeCompleteUsing(Scope *S) {
   if (!CodeCompleter)
     return;
   
   ResultBuilder Results(*this, CodeCompleter->getAllocator(),
                         CodeCompleter->getCodeCompletionTUInfo(),