[clang] Do not crash on undefined template partial specialization

[Fznamznon]

Summary

Before checking that template partial specialization is "reachable",
ensure it exists.

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

[erichkeane]

clang/lib/Sema/SemaCXXScopeSpec.cpp

134–135

I would expect 'hasReachableDefinition' to check boht reachable AND definition :)

That said, I doubt the 'missing import' error is an appropriate one here.

clang/test/SemaCXX/undefined-partial-specialization.cpp

13

I don't think this is correct. The diagnostic is inaccurate, it DOES refer to a class template partial specialization (I can see it on line 9!), but the problem is that it is incomplete.

[Fznamznon]

clang/lib/Sema/SemaCXXScopeSpec.cpp

134–135

I would expect 'hasReachableDefinition' to check boht reachable AND definition :)

Well, it is implemented in a way that it actually expects definition to be present. In some other places where 'hasReachableDefinition' is called there is also a check that definition exists prior the call.

[Fznamznon]

clang/test/SemaCXX/undefined-partial-specialization.cpp

13

Huh, it seems it was my change that made this diagnostic inaccurate. I was under impression that it shouldn't have done this.
For c++17 it used to say "error: out-of-line definition of 'foo' from class 'boo<type-parameter-0-0, true>' without definition" without crash. Now it is inaccurate for both c++17 and c++20. I'll look into this more.

[erichkeane]

clang/lib/Sema/SemaCXXScopeSpec.cpp

134–135

Hrmph, ok then. Perhaps it is not the right place to change things anyway.

clang/test/SemaCXX/undefined-partial-specialization.cpp

13

Yes, I would definitely expect the 'out-of-line-definition' diagnostic, like we do for non-partial specs, and the C++17 behavior. Interesting that we've messed this up for C++20, should be fun to track down/figure out!

[Fznamznon]

Rebase, fix error message

[Fznamznon]

clang/test/SemaCXX/undefined-partial-specialization.cpp

13

Interesting that we've messed this up for C++20, should be fun to track down/figure out!

hasReachableDefinition calls hasAcceptableDefinition that early-exits if support for modules is not required.

[erichkeane]

clang/test/SemaCXX/undefined-partial-specialization.cpp

2

nit: can you add a c++17 run line here too? Only 20 crashed, but I want to make sure these have hte same behavior.

[Fznamznon]

Test c++17 too

clang/test/SemaCXX/undefined-partial-specialization.cpp

2

Sure.

[erichkeane]

Thanks! still LGTM.

[shafik]

clang/lib/Sema/SemaCXXScopeSpec.cpp

134

So I see in another location we are basically checking !isBeginDefined() and in another place hasCompleteDefinition(). It is not clear to me if these are all basically checking the same thing or if we should figure out what is the right thing to check and do that everywhere.

[Fznamznon]

clang/lib/Sema/SemaCXXScopeSpec.cpp

134

I suppose you meant isBeingDefined(). So, IMO isBeingDefined() is not exactly the same as having a definition, this is set to true when TagDecl::startDefinition I suppose this should be done when parser meets a definition so at this point it can't be or have a complete definition. But at the places where !isBeingDefined() is checked prior hasReachableDefinition I saw a pointer representing a definition checked for not being null. Interestingly, hasReachableDefinition() early exits if isBeingDefined() returns true, so probably this check additional doesn't make sense at all.
Maybe we should just move both checks on having a definition and not being in the process of definition under hasReachableDefinition after all. That will require changing unrelated places, so I would prefer doing this separately in any case.

I can't grep hasCompleteDefinition() either, so I suppose you meant isCompleteDefinition(), I think this is basically the same thing as having definition and additionally the declaration through which the method called is checked that IT IS the definition. I'm not sure we have to require it here.

[Fznamznon]

clang/lib/Sema/SemaCXXScopeSpec.cpp

134

@shafik , are you satisfied with the explanation?

[shafik]

LGTM

clang/lib/Sema/SemaCXXScopeSpec.cpp

134

I think we are good.

clang/docs/ReleaseNotes.rst

 - Fix crash when a doc comment contains a line splicing.
   (`#62054 <https://github.com/llvm/llvm-project/issues/62054>`_)
 - Work around with a clang coverage crash which happens when visiting 
   expressions/statements with invalid source locations in non-assert builds. 
   Assert builds may still see assertions triggered from this.
 - Fix a failed assertion due to an invalid source location when trying to form
   a coverage report for an unresolved constructor expression.
   (`#62105 <https://github.com/llvm/llvm-project/issues/62105>`_)
+- Fix crash when handling undefined template partial specialization
+  (`#61356 <https://github.com/llvm/llvm-project/issues/61356>`_)
 
 Bug Fixes to Compiler Builtins
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 
 Bug Fixes to Attribute Support
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 
 Bug Fixes to C++ Support

clang/lib/Sema/SemaCXXScopeSpec.cpp

if (EnteringContext) {
           }
 
           if (PartialSpec) {
             // A declaration of the partial specialization must be visible.
             // We can always recover here, because this only happens when we're
             // entering the context, and that can't happen in a SFINAE context.
             assert(!isSFINAEContext() && "partial specialization scope "
                                          "specifier in SFINAE context?");
-            if (!hasReachableDefinition(PartialSpec))
+            if (PartialSpec->hasDefinition() &&
+                !hasReachableDefinition(PartialSpec))
               diagnoseMissingImport(SS.getLastQualifierNameLoc(), PartialSpec,
                                     MissingImportKind::PartialSpecialization,
                                     true);
             return PartialSpec;
           }
 
           // If the type of the nested name specifier is the same as the
           // injected class name of the named class template, we're entering

clang/test/SemaCXX/undefined-partial-specialization.cpp

+// RUN: %clang_cc1 -std=c++17 -verify %s
+// RUN: %clang_cc1 -std=c++20 -verify %s
+
+namespace GH61356 {
+
+template <typename T, bool b>
+class boo {void foo();};
+
+template <typename T>
+class boo<T, true>;
+
+template<typename T>
+void boo<T, true>::foo(){} // expected-error{{out-of-line definition of 'foo' from class 'boo<type-parameter-0-0, true>' without definition}}
+
+}