[clang][#47272] Avoid suggesting deprecated version of a declaration over another in typo correction

[zero9178]

Summary

Prior to this patch, clang might suggest a deprecated name of a declaration over another name as the only mechanism for resolving two typo corrections referring to the same underlying declaration has previously been an alphabetical sort.

This patch adjusts this resolve by also taking into account whether one of two declarations are deprecated. If the new one is deprecated it may not replace a previous correction with a non-deprecated correction and a previous deprecated correction always gets replaced by a non-deprecated new correction.

Fixes https://github.com/llvm/llvm-project/issues/47272

[• Quuxplusone]

clang/lib/Sema/SemaLookup.cpp

4339

It seems like this code is overdue for a "compare these two corrections" predicate. The simple version would be

auto IsBetter = [&](const TypoCorrection& TC1, const TypoCorrection& TC2) {
    std::pair<bool, std::string> Key1 = { IsDeprecated(TC1.getFoundDecl()), TC1.getAsString(SemaRef.getLangOpts()) };
    std::pair<bool, std::string> Key2 = { IsDeprecated(TC2.getFoundDecl()), TC2.getAsString(SemaRef.getLangOpts()) };
    return Key1 < Key2;  // prefer non-deprecated, alphabetically earlier declarations
};
for (TypoCorrection& TC : CList) {
    if (IsBetter(Correction, TC))
        TC = Correction;
}

Actually, you could even replicate the hoisting of the loop-invariant stuff, the way the old code did:

std::pair<bool, std::string> CorrectionKey = { IsDeprecated(Correction.getFoundDecl()), Correction.getAsString(SemaRef.getLangOpts()) };
auto CorrectionIsBetterThan = [&](const TypoCorrection& TC) {
    std::pair<bool, std::string> Key = { IsDeprecated(TC.getFoundDecl()), TC.getAsString(SemaRef.getLangOpts()) };
    return CorrectionKey < Key;  // prefer non-deprecated, alphabetically earlier declarations
};
for (TypoCorrection& TC : CList) {
    if (CorrectionIsBetterThan(TC))
        TC = Correction;
}

clang/test/SemaCXX/typo-correction.cpp

764–769

I suggest three namespaces, named [[deprecated]] A, B, and [[deprecated]] C, and declared in that lexical order too, so that no matter how the preference predicate is expressed, we're still testing that the non-deprecated (middle) declaration gets chosen.

[zero9178]

Utilize operator< of std::pair to simplify the comparison A LOT, simplify using llvm::find_if and add additional test case

[zero9178]

clang/lib/Sema/SemaLookup.cpp

4339

Thanks I really liked the idea of using std::pairs comparison for that.
I chose to separate the concerns, by first finding the declaration in the list and only then using the comparison of the found decl and the new correction.

[aaron.ballman]

LGTM aside from some nits, though please wait for @Quuxplusone to respond before landing.

clang/lib/Sema/SemaLookup.cpp

4310–4311

4317

Feel free to pick a better name than D (other than Decl, please!), just cleaning up the coding style nit.

4327

4328–4332

[• Quuxplusone]

LGTM % comments, but I'll take one more look.

clang/test/SemaCXX/typo-correction.cpp

772–782

I'd like B to be declared second.

namespace [[deprecated]] A { int pr47272; }
namespace B { using A::pr47272; } // expected-note{{'B::pr47272' declared here}}
namespace [[deprecated]] C { using A::pr47272; }

~~~
  int y = ::pr47272; // expected-error{{no member named 'pr47272' in the global namespace; did you mean 'B::pr47272'?}}

I also changed it from a function to a variable, just to get a little more coverage (since Take() was already covering functions).

[zero9178]

Addressed reviewer nits:

• Used proper naming conventions
• Adjusted tests

[• Quuxplusone]

LGTM!

clang/lib/Sema/SemaLookup.cpp

void TypoCorrectionConsumer::addCorrection(TypoCorrection Correction) {
   }
 
   TypoResultList &CList =
       CorrectionResults[Correction.getEditDistance(false)][Name];
 
   if (!CList.empty() && !CList.back().isResolved())
     CList.pop_back();
   if (NamedDecl *NewND = Correction.getCorrectionDecl()) {
-    std::string CorrectionStr = Correction.getAsString(SemaRef.getLangOpts());
-    for (TypoResultList::iterator RI = CList.begin(), RIEnd = CList.end();
-         RI != RIEnd; ++RI) {
-      // If the Correction refers to a decl already in the result list,
-      // replace the existing result if the string representation of Correction
-      // comes before the current result alphabetically, then stop as there is
-      // nothing more to be done to add Correction to the candidate set.
-      if (RI->getCorrectionDecl() == NewND) {
-        if (CorrectionStr < RI->getAsString(SemaRef.getLangOpts()))
-          *RI = Correction;
-        return;
-      }
-    }
-  }
+    auto RI = llvm::find_if(CList, [NewND](const TypoCorrection &typoCorr) {
+      return typoCorr.getCorrectionDecl() == NewND;
+    });
+    if (RI != CList.end()) {
+      // The Correction refers to a decl already in the list. No insertion is
+      // necessary and all further cases will return.
+
+      auto IsDeprecated = [](Decl *decl) {
+        while (decl) {
+          if (decl->isDeprecated())
+            return true;
+          decl = llvm::dyn_cast_or_null<NamespaceDecl>(decl->getDeclContext());
+        }
+        return false;
+      };
+
+      // Prefer non deprecated Corrections over deprecated and only then
+      // sort using an alphabetical order
+      std::pair<bool, std::string> newKey = {
+          IsDeprecated(Correction.getFoundDecl()),
+          Correction.getAsString(SemaRef.getLangOpts())};
+
+      std::pair<bool, std::string> prevKey = {
+          IsDeprecated(RI->getFoundDecl()),
+          RI->getAsString(SemaRef.getLangOpts())};
+
+      if (newKey < prevKey)
   if (CList.empty() || Correction.isResolved())
     CList.push_back(Correction);
 
   while (CorrectionResults.size() > MaxTypoDistanceResultSets)
     CorrectionResults.erase(std::prev(CorrectionResults.end()));
 }
 
 void TypoCorrectionConsumer::addNamespaces(

clang/test/SemaCXX/typo-correction.cpp

namespace PR46487 {
   // FIXME: We shouldn't suggest a typo-correction to 'g_var_bool' here,
   // because it doesn't make the expression valid.
   // expected-error@+2 {{did you mean 'g_var_bool'}}
   // expected-error@+1 {{assigning to 'bool' from incompatible type 'void'}}
   enum : decltype((g_var_long = throw))::a {
     b = g_volatile_uchar // expected-error {{did you mean 'g_volatile_char'}}
   };
 }
+
+namespace PR47272
+{
+
+namespace Views {
+int Take(); // expected-note{{'Views::Take' declared here}}
+}
+
+namespace [[deprecated("use Views instead")]] View {
+using Views::Take;
+}
+
+namespace B {
+int pr47272();  // expected-note{{'B::pr47272' declared here}}
+}
+
+namespace [[deprecated]] A {
+using B::pr47272;
+}
+
+namespace [[deprecated]] C {
+using B::pr47272;
+}
+
+void function() {
+  int x = ::Take(); // expected-error{{no member named 'Take' in the global namespace; did you mean 'Views::Take'?}}
+  int y = ::pr47272(); // expected-error{{no member named 'pr47272' in the global namespace; did you mean 'B::pr47272'?}}
+}
+}
+
+