[clangd] Overload bundles are only deprecated if each overloads is.

[sammccall]

Summary

Fixes https://github.com/clangd/clangd/issues/705

[sammccall]

fix bug link

[sammccall]

simplify

[kbobyrev]

So, if my understanding is correct, this will make the whole bundle non-deprecated if at least one overload is not deprecated? This is probably an improvement over the existing behaviour. However, do we maybe want to split the bundles into deprecated and non-deprecated groups?

clang-tools-extra/clangd/CodeComplete.cpp

411

The comment you added says "cleared" which means this should probably be |= rather than =, right?

Also, I this looks longer but probably more readable at least for me.

Also, the sources might be SemaResult, IndexResult or IdentifierResult, right? :( Otherwise could've been Completion.Deprecated |= C.SemaResult ? C.SemaResult->Availability == CXAvailability_Deprecated : C.IndexResult->Flags & Symbol::Deprecated;

[sammccall]

Whoops, lost this patch...

In D97803#2602787, @kbobyrev wrote:

So, if my understanding is correct, this will make the whole bundle non-deprecated if at least one overload is not deprecated? This is probably an improvement over the existing behaviour.

Yes, exactly.

However, do we maybe want to split the bundles into deprecated and non-deprecated groups?

I don't think splitting bundles over this is worthwhile.

• it's a distraction. The point of bundling is to hide some differences within an overload set, to let you see all the methods more clearly.
• the "deprecated" signal means "you don't want this". This is well-defined, and false, for a mixed bundle.

clang-tools-extra/clangd/CodeComplete.cpp

411

The comment you added says "cleared" which means this should probably be |= rather than =, right?

No, Deprecated *starts out true* and gets set to false (cleared) if we see any non-deprecated entry. (computes AND)

Your version assumes it starts out false and sets it if we see any deprecated entry. (computes OR).

I agree the OR version reads better - it's wrong though :-)

[kbobyrev]

I see, thank you very much for the explanation!

clang-tools-extra/clangd/CodeComplete.cpp

411

Ahh, okay, makes sense, thank you!

Nit: I think the version I suggested (with fixes |= vs =) is somewhat easier to read and doesn't take much more space.

[sammccall]

clang-tools-extra/clangd/CodeComplete.cpp

411

Done, though with &= instead of = otherwise the logic is subtly different (the second clobbers the first).
And because of &= we need an explicit cast to bool.

clang-tools-extra/clangd/CodeComplete.cpp

CodeCompletionBuilder(ASTContext *ASTCtx, const CompletionCandidate &C,
                         const IncludeInserter &Includes,
                         llvm::StringRef FileName,
                         CodeCompletionContext::Kind ContextKind,
                         const CodeCompleteOptions &Opts,
                         bool IsUsingDeclaration, tok::TokenKind NextTokenKind)
       : ASTCtx(ASTCtx),
         EnableFunctionArgSnippets(Opts.EnableFunctionArgSnippets),
         IsUsingDeclaration(IsUsingDeclaration), NextTokenKind(NextTokenKind) {
+    Completion.Deprecated = true; // cleared by any non-deprecated overload.
     add(C, SemaCCS);
     if (C.SemaResult) {
       assert(ASTCtx);
       Completion.Origin |= SymbolOrigin::AST;
       Completion.Name = std::string(llvm::StringRef(SemaCCS->getTypedText()));
       if (Completion.Scope.empty()) {
         if ((C.SemaResult->Kind == CodeCompletionResult::RK_Declaration) ||
             (C.SemaResult->Kind == CodeCompletionResult::RK_Pattern))
if (C.SemaResult) {
       for (const auto &FixIt : C.SemaResult->FixIts) {
         Completion.FixIts.push_back(toTextEdit(
             FixIt, ASTCtx->getSourceManager(), ASTCtx->getLangOpts()));
       }
       llvm::sort(Completion.FixIts, [](const TextEdit &X, const TextEdit &Y) {
         return std::tie(X.range.start.line, X.range.start.character) <
                std::tie(Y.range.start.line, Y.range.start.character);
       });
-      Completion.Deprecated |=
-          (C.SemaResult->Availability == CXAvailability_Deprecated);
     }
     if (C.IndexResult) {
       Completion.Origin |= C.IndexResult->Origin;
       if (Completion.Scope.empty())
         Completion.Scope = std::string(C.IndexResult->Scope);
       if (Completion.Kind == CompletionItemKind::Missing)
         Completion.Kind = toCompletionItemKind(C.IndexResult->SymInfo.Kind);
       if (Completion.Name.empty())
         Completion.Name = std::string(C.IndexResult->Name);
       // If the completion was visible to Sema, no qualifier is needed. This
       // avoids unneeded qualifiers in cases like with `using ns::X`.
       if (Completion.RequiredQualifier.empty() && !C.SemaResult) {
         llvm::StringRef ShortestQualifier = C.IndexResult->Scope;
         for (llvm::StringRef Scope : QueryScopes) {
           llvm::StringRef Qualifier = C.IndexResult->Scope;
           if (Qualifier.consume_front(Scope) &&
               Qualifier.size() < ShortestQualifier.size())
             ShortestQualifier = Qualifier;
         }
         Completion.RequiredQualifier = std::string(ShortestQualifier);
       }
-      Completion.Deprecated |= (C.IndexResult->Flags & Symbol::Deprecated);
     }
     if (C.IdentifierResult) {
       Completion.Origin |= SymbolOrigin::Identifier;
       Completion.Kind = CompletionItemKind::Text;
       Completion.Name = std::string(C.IdentifierResult->Name);
     }
 
     // Turn absolute path into a literal string that can be #included.
if (!Completion.Documentation) {
       };
       if (C.IndexResult) {
         SetDoc(C.IndexResult->Documentation);
       } else if (C.SemaResult) {
         SetDoc(getDocComment(*ASTCtx, *C.SemaResult,
                              /*CommentsFromHeader=*/false));
       }
     }
+    if (Completion.Deprecated) {
+      if (C.SemaResult)
+        Completion.Deprecated &=
+            C.SemaResult->Availability == CXAvailability_Deprecated;
+      if (C.IndexResult)
+        Completion.Deprecated &=
+            bool(C.IndexResult->Flags & Symbol::Deprecated);
+    }
   }
 
   CodeCompletion build() {
     Completion.ReturnType = summarizeReturnType();
     Completion.Signature = summarizeSignature();
     Completion.SnippetSuffix = summarizeSnippet();
     Completion.BundleSize = Bundled.size();
     return std::move(Completion);

clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp

 
 TEST(CompletionTest, OverloadBundling) {
   clangd::CodeCompleteOptions Opts;
   Opts.BundleOverloads = true;
 
   std::string Context = R"cpp(
     struct X {
       // Overload with int
-      int a(int);
+      int a(int) __attribute__((deprecated("", "")));
       // Overload with bool
       int a(bool);
       int b(float);
     };
     int GFuncC(int);
     int GFuncD(int);
   )cpp";
 
auto A =
       completions(Context + "int y = X().a^", {}, Opts).Completions.front();
   EXPECT_EQ(A.Name, "a");
   EXPECT_EQ(A.Signature, "(…)");
   EXPECT_EQ(A.BundleSize, 2u);
   EXPECT_EQ(A.Kind, CompletionItemKind::Method);
   EXPECT_EQ(A.ReturnType, "int"); // All overloads return int.
   // For now we just return one of the doc strings arbitrarily.
   ASSERT_TRUE(A.Documentation);
+  ASSERT_FALSE(A.Deprecated); // Not all overloads deprecated.
   EXPECT_THAT(
       A.Documentation->asPlainText(),
       AnyOf(HasSubstr("Overload with int"), HasSubstr("Overload with bool")));
   EXPECT_EQ(A.SnippetSuffix, "($0)");
 }
 
 TEST(CompletionTest, OverloadBundlingSameFileDifferentURI) {
   clangd::CodeCompleteOptions Opts;