[Clang] Fix compat diagnostic to detect a nontype template parameter has a placeholder type using getContainedAutoType()

[shafik]

Summary

Based on the changes introduced by 15361a21e01026e74cb17011b702c7d1c881ae94 it looks like C++17 compatibility diagnostic should have been checking getContainedAutoType().

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

[mizvekov]

clang/lib/Sema/SemaTemplate.cpp

1533–1538

I think checking that the deduced type is undeduced was really unnecessary though, as I don't think we update the type source infos after deduction in any case.

clang/test/SemaTemplate/temp_arg_nontype_diagnostic_cxx1z.cpp

7 ↗

(On Diff #456848)

I think this should have a new diagnostic per above, as this is not compatible with C++17.

[mizvekov]

clang/test/SemaTemplate/temp_arg_nontype_diagnostic_cxx1z.cpp

1 ↗

(On Diff #456848)

One thing we could do is to run this test also in std=c++17 mode, with a different expectancy, as a kind of sanity check that the warning is in sync.

[shafik]

clang/lib/Sema/SemaTemplate.cpp

1533–1538

I am not sure about the DeducedTemplateSpecializationType I was looking at the change that brought in that type and it is not clear to me what a good test would be. I believe outside of C++17 mode they would be hard errors. Can you provide an example that would work outside of C++17 mode?

1533–1538

The DeducedTemplateSpecializationType also feels a bit outside the bug I am fixing so I might just choose to address it in a follow-up.

[mizvekov]

clang/lib/Sema/SemaTemplate.cpp

1533–1538

Well the bit on your test below, which you marked with no diagnostic expected, is an example.

This is something that works on C++20 but not on C++17, which is what this warning is supposed to warn, right?

[shafik]

• Updated to check contained deduced type before checking if it is an AutoType
• Split out test into C++20 and C++17 parts

[shafik]

clang/test/SemaTemplate/temp_arg_nontype_diagnostic_cxx1z.cpp

7 ↗

(On Diff #456848)

I split out the tests to C++20 and C++17 parts.

[aaron.ballman]

Can you also add a release note for the changes?

clang/lib/Sema/SemaTemplate.cpp

1534–1539

Let's get fancy!

clang/test/SemaTemplate/temp_arg_nontype_diagnostic_cxx1z.cpp

1 ↗

(On Diff #457471)

Please rename the file to end in cxx17 instead of cxx1z :-)

[mizvekov]

clang/lib/Sema/SemaTemplate.cpp

1534–1539

You would use getContainedDeducedType if you expected to handle DeducedTypes in general, not just AutoTypes.

So if you only want to handle AutoTypes, there is no point in using getContainedDeducedType.

clang/test/SemaTemplate/temp_arg_nontype_diagnostic_cxx1z.cpp

3–8 ↗

(On Diff #457471)

I don't understand why these cases are grouped under GH57362 issue, they are cases that worked fine without this patch, we just didn't have tests for them.

[mizvekov]

clang/test/SemaTemplate/temp_arg_nontype_cxx20.cpp

310–316 ↗

(On Diff #457471)

I think the issue might not be tested in this file since we do not run it with the -Wpre-c++17-compat flag

[shafik]

• Updating testing to reflect comments.

clang/lib/Sema/SemaTemplate.cpp

1534–1539

I am going to keep the getContainedDeducedType(...) b/c I do plan on coming back to this and adding the other diagnostic but I won't get fancy at this point but will happily consider it on the follow-up.

clang/test/SemaTemplate/temp_arg_nontype_cxx20.cpp

310–316 ↗

(On Diff #457471)

Good catch, I will just create a standalone gh57362.cpp file since this does not fit neatly anywhere else.

clang/test/SemaTemplate/temp_arg_nontype_diagnostic_cxx1z.cpp

3–8 ↗

(On Diff #457471)

Good point.

[shafik]

ping,

[mizvekov]

Oh by the way, it's missing the Release notes, but otherwise LGTM.

[shafik]

• Add release notes.

[mizvekov]

clang/docs/ReleaseNotes.rst

149–151

'compatibility' mispelled.

Though since this is a user facing document, I would try to give a less mechanical / less local explanation of the problem.

[shafik]

• Update wording in release note.

clang/docs/ReleaseNotes.rst

- Clang will now no longer treat a C 'overloadable' function without a prototype as
   a variadic function with the attribute.  This should make further diagnostics more
   clear.
 - Fixes to builtin template emulation of regular templates.
   `Issue 42102 <https://github.com/llvm/llvm-project/issues/42102>`_
   `Issue 51928 <https://github.com/llvm/llvm-project/issues/51928>`_
 - A SubstTemplateTypeParmType can now represent the pack index for a
   substitution from an expanded pack.
   `Issue 56099 <https://github.com/llvm/llvm-project/issues/56099>`_
+- Fix `-Wpre-c++17-compat` crashing Clang when compiling C++20 code which
+  contains deduced template specializations. This Fixes
+  `Issue 57369 <https://github.com/llvm/llvm-project/issues/57369>`_
+  `Issue 57643 <https://github.com/llvm/llvm-project/issues/57643>`_
+  `Issue 57793 <https://github.com/llvm/llvm-project/issues/57793>`_
 
 
 Improvements to Clang's diagnostics
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 - Clang will now check compile-time determinable string literals as format strings.
   Fixes `Issue 55805: <https://github.com/llvm/llvm-project/issues/55805>`_.
 - ``-Wformat`` now recognizes ``%b`` for the ``printf``/``scanf`` family of
   functions and ``%B`` for the ``printf`` family of functions. Fixes

clang/lib/Sema/SemaTemplate.cpp

auto CheckValidDeclSpecifiers = [this, &D] {
     if (DS.hasExplicitSpecifier())
       EmitDiag(DS.getExplicitSpecLoc());
 
     if (DS.isNoreturnSpecified())
       EmitDiag(DS.getNoreturnSpecLoc());
   };
 
   CheckValidDeclSpecifiers();
 
-  if (TInfo->getType()->isUndeducedType()) {
+  if (const auto *T = TInfo->getType()->getContainedDeducedType())
+    if (isa<AutoType>(T))
       Diag(D.getIdentifierLoc(),
            diag::warn_cxx14_compat_template_nontype_parm_auto_type)
           << QualType(TInfo->getType()->getContainedAutoType(), 0);
-  }
 
   assert(S->isTemplateParamScope() &&
          "Non-type template parameter not in template parameter scope!");
   bool Invalid = false;
 
   QualType T = CheckNonTypeTemplateParameterType(TInfo, D.getIdentifierLoc());
   if (T.isNull()) {
     T = Context.IntTy; // Recover with an 'int' type.
     Invalid = true;

clang/test/SemaTemplate/gh57362.cpp

+// RUN: %clang_cc1 -fsyntax-only -verify -std=c++20 -Wpre-c++17-compat %s
+// expected-no-diagnostics
+
+namespace GH57362 {
+template <int num>
+class TemplateClass {};
+
+template <TemplateClass nttp> // ok, no diagnostic expected
+void func() {}
+}

clang/test/SemaTemplate/temp_arg_nontype_diagnostic_cxx17.cpp

+// RUN: %clang_cc1 -fsyntax-only -verify -std=c++17 -Wpre-c++17-compat %s
+
+template <decltype(auto) n> // expected-warning {{non-type template parameters declared with 'decltype(auto)' are incompatible with C++ standards before C++17}}
+struct B{};
+
+template <auto n> // expected-warning {{non-type template parameters declared with 'auto' are incompatible with C++ standards before C++17}}
+struct A{};