[Clang] Add sanity check in Sema::getDestructorName to prevent nullptr dereference

[shafik]

Summary

Currently in Sema::getDestructorName we call SS.getScopeRep()->getPrefix() but SS.getScopeRep() can return nullptr because LookupInNestedNameSpec(...) called a little before can invalidate SS.

This fixes: https://github.com/llvm/llvm-project/issues/59446

[tahonermann]

clang/lib/Sema/SemaExprCXX.cpp

394

CXXScopeSpec::isSet() is apparently (intended to be) deprecated.

clang/include/clang/Sema/DeclSpec.h:
 209   /// Deprecated.  Some call sites intend isNotEmpty() while others intend
 210   /// isValid().
 211   bool isSet() const { return getScopeRep() != nullptr; }

It sounds like this should instead call .isValid() or .isNotEmpty(), but I'm not sure which.

[aaron.ballman]

clang/lib/Sema/SemaExprCXX.cpp

394

We want to use isValid() -- isNotEmpty() will return true when the scope spec is present but invalid, so the call to SS.getScopeRep()->getPrefix() would crash in that case.

[aaron.ballman]

Also, don't forget to add a release note for the fix. :-)

[shafik]

• Switched to isValid() over isSet()
• Added release note

[aaron.ballman]

LGTM!

clang/docs/ReleaseNotes.rst

 infrastructure are described first, followed by language-specific
 sections with improvements to Clang's support for those languages.
 
 Major New Features
 ------------------
 
 Bug Fixes
 ---------
+- Fix crash on invalid code when looking up a destructor in a templated class
+  inside a namespace. This fixes
+  `Issue 59446 <https://github.com/llvm/llvm-project/issues/59446>`_.
 
 Improvements to Clang's diagnostics
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 
 Non-comprehensive list of changes in this release
 -------------------------------------------------
 
 New Compiler Flags

clang/lib/Sema/SemaExprCXX.cpp

if (SS.isSet()) {
     }
 
     // For compatibility with other compilers and older versions of Clang,
     //
     //   nested-name-specifier type-name :: ~ type-name
     //
     // also looks for type-name in the scope. Unfortunately, we can't
     // reasonably apply this fallback for dependent nested-name-specifiers.
-    if (SS.getScopeRep()->getPrefix()) {
+    if (SS.isValid() && SS.getScopeRep()->getPrefix()) {
       if (ParsedType T = LookupInScope()) {
         Diag(SS.getEndLoc(), diag::ext_qualified_dtor_named_in_lexical_scope)
             << FixItHint::CreateRemoval(SS.getRange());
         Diag(FoundDecls.back()->getLocation(), diag::note_destructor_type_here)
             << GetTypeFromParser(T);
         return T;
       }
     }

clang/test/SemaCXX/GH59446.cpp

+// RUN: %clang_cc1 -fsyntax-only -verify -std=c++11 %s
+
+namespace GH59446 { // expected-note {{to match this '{'}}
+namespace N {
+    template <typename T> struct X ; // expected-note 2 {{template is declared here}}
+                                     // expected-note@-1 {{'N::X' declared here}}
+             // expected-note@-2 {{non-type declaration found by destructor name lookup}}
+  }
+  void f(X<int> *x) { // expected-error {{no template named 'X'; did you mean 'N::X'}}
+    x->N::X<int>::~X(); // expected-error 2 {{implicit instantiation of undefined template 'GH59446::N::X<int>'}}
+                        // expected-error@-1 {{identifier 'X' after '~' in destructor name does not name a type}}
+} // expected-error {{expected '}'}}