Reapply "Fix crash on switch conditions of non-integer types in templates"

[eandrews]

Summary

This patch reapplies D61027. When D61027 was previously committed (76945821b9cad3), buildbots failed due to clang-tidy test fails. The test fails are because some errors in templates are now diagnosed earlier (does not wait till instantiation). I have modified the tests to add checks for these diagnostics/prevent these diagnostics.

Since I have not worked on clang-tidy in the past, I am hoping someone with more familiarity in this area can take a look and review my changes. There are no code changes in this second attempt (compared to D61027). I have only modified failing tests.

Summary of code changes (pasted from D61027) is below -

Clang currently crashes for switch statements inside a template when the condition is a non-integer field member because contextual implicit conversion is skipped when parsing the condition. This conversion is however later checked in an assert when the case statement is handled. The conversion is skipped when parsing the condition because the field member is set as type-dependent based on its containing class. This patch sets the type dependency based on the field's type instead. This patch fixes Bug 40982.

[rnk]

I don't have experience with clang-tidy tests, but I think these two small, test-only changes are small enough that you can go ahead and commit them without review from a clang-tidy owner. lgtm

[gribozavr2]

LGTM for ClangTidy, assuming you ran ninja check-all.

[eandrews]

Thank you for taking a look Reid and Dmitri. There were no fails with check-all.

My commit rights have not been transferred from SVN to Github yet, so Melanie (@mibintc) has kindly agreed to commit this patch for me.

[eandrews]

@rnk @gribozavr2 clang-tidy test "bugprone-string-integer-assignment.cpp" caused a bot fail on windows. From the logs it looks like the 'CHECK-FIXES' I added in that test is failing because the fix isn't applied. I am not sure why. I don't have a windows build set up and I assume a new build is going to take some time. I am not sure what the protocol for this is. Should I go ahead and revert my patch while I investigate the issue on Windows or should I just push a new patch deleting the 'CHECK-FIXES' while I look into the issue?

[gribozavr2]

Should I go ahead and revert my patch while I investigate the issue on Windows or should I just push a new patch deleting the 'CHECK-FIXES' while I look into the issue?

Yes. As a general rule, if you landed a patch and it broke a bot that was previously green, please revert ASAP and investigate later.

[eandrews]

Thank you for letting me know. The patch has been reverted.

The fail is a result of different diagnostics thrown on Windows and Linux. It looks like clang passes -fdelayed-template-parsing by default on Windows, which I assume is the reason why the diagnostic is not generated on Windows. I'm not entirely sure how different diagnostics in various targets are handled by clang-tidy. I'll push a new patch once I take a closer look.

[gribozavr2]

A number of ClangTidy tests have -fno-delayed-template-parsing in their RUN lines. While not a great solution (it only fixes the test, but does not make the warnings show for users on Windows), there's only so much a checker can do to counteract that. Before adding this flag to the affected test, it might be interesting to understand though why this patch had such a consequence though.

[eandrews]

The patch changes type dependency of field 'std::string s' and sets it based on field type (i.e. not type-dependent).

I do not know how clang-tidy works internally, but I assume checker used to skip the field earlier since it was set as dependent (based on containing class 'S'), hence not throwing the warning on any platform? A diagnostic is thrown with this patch possibly because of change in field s' type-dependency.

[gribozavr2]

hence not throwing the warning on any platform?

I'm not sure I understand? The way I read the buildbot breakage, an existing ClangTidy test passed before and after this change, but broke on Windows. The breakage was that the warnings stopped being produced. Usually that is related to delayed parsing of templates on Windows.

[eandrews]

In D69950#1751859, @gribozavr2 wrote:

hence not throwing the warning on any platform?

The way I read the buildbot breakage, an existing ClangTidy test passed before and after this change, but broke on Windows. The breakage was that the warnings stopped being produced.

The existing test does not have this warning. I modified the test to add the check for this warning since it is generated on Linux after my patch. It is not generated on Windows because of delayed template parsing.

[gribozavr2]

In D69950#1751894, @eandrews wrote:

In D69950#1751859, @gribozavr2 wrote:

hence not throwing the warning on any platform?

The way I read the buildbot breakage, an existing ClangTidy test passed before and after this change, but broke on Windows. The breakage was that the warnings stopped being produced.

The existing test does not have this warning. I modified the test to add the check for this warning since it is generated on Linux after my patch. It is not generated on Windows because of delayed template parsing.

Is it just that warning that is not generated, or all warnings in that test file?

Anyhow, adding -fno-delayed-template-parsing to a ClangTidy test is not unreasonable, and is indeed existing practice.

[eandrews]

In D69950#1752044, @gribozavr2 wrote:

In D69950#1751894, @eandrews wrote:

In D69950#1751859, @gribozavr2 wrote:

hence not throwing the warning on any platform?

The way I read the buildbot breakage, an existing ClangTidy test passed before and after this change, but broke on Windows. The breakage was that the warnings stopped being produced.

The existing test does not have this warning. I modified the test to add the check for this warning since it is generated on Linux after my patch. It is not generated on Windows because of delayed template parsing.

Is it just that warning that is not generated, or all warnings in that test file?

I am pretty sure it is just this warning but I will verify this again and push a new patch with "-fno-delayed-template-parsing" once I confirm . Thanks for all your help!

[mstorsjo]

This (when reapplied in https://reviews.llvm.org/rG878a24ee244a24c39d1c57e9af2) broke compilation of code that earlier built fine. A reduced example:

namespace glslang {
class TPoolAllocator {
  void operator=(TPoolAllocator);
};
template <class> class a {
  TPoolAllocator *b;
  void c() { allocator = *b; }
  TPoolAllocator allocator;
};
} // namespace glslang

[eandrews]

In D69950#1770138, @mstorsjo wrote:

This (when reapplied in https://reviews.llvm.org/rG878a24ee244a24c39d1c57e9af2) broke compilation of code that earlier built fine. A reduced example:

namespace glslang {
class TPoolAllocator {
  void operator=(TPoolAllocator);
};
template <class> class a {
  TPoolAllocator *b;
  void c() { allocator = *b; }
  TPoolAllocator allocator;
};
} // namespace glslang

With this patch, some errors in templates are diagnosed earlier (i.e. does not wait till instantiation). Since 'allocator' and 'b' aren't dependent, I think this is a valid diagnosis. GCC throws an error on this code upon instantiation. https://godbolt.org/z/X9Y-Vy

[mstorsjo]

In D69950#1771133, @eandrews wrote:

In D69950#1770138, @mstorsjo wrote:

This (when reapplied in https://reviews.llvm.org/rG878a24ee244a24c39d1c57e9af2) broke compilation of code that earlier built fine. A reduced example:

namespace glslang {
class TPoolAllocator {
  void operator=(TPoolAllocator);
};
template <class> class a {
  TPoolAllocator *b;
  void c() { allocator = *b; }
  TPoolAllocator allocator;
};
} // namespace glslang

With this patch, some errors in templates are diagnosed earlier (i.e. does not wait till instantiation). Since 'allocator' and 'b' aren't dependent, I think this is a valid diagnosis. GCC throws an error on this code upon instantiation. https://godbolt.org/z/X9Y-Vy

The original non-reduced case does build fine with GCC though (I'm fairly sure). I can try to reduce one later that still builds with GCC but fails with current clang.

[eandrews]

In D69950#1771340, @mstorsjo wrote:

In D69950#1771133, @eandrews wrote:

In D69950#1770138, @mstorsjo wrote:

This (when reapplied in https://reviews.llvm.org/rG878a24ee244a24c39d1c57e9af2) broke compilation of code that earlier built fine. A reduced example:

namespace glslang {
class TPoolAllocator {
  void operator=(TPoolAllocator);
};
template <class> class a {
  TPoolAllocator *b;
  void c() { allocator = *b; }
  TPoolAllocator allocator;
};
} // namespace glslang

With this patch, some errors in templates are diagnosed earlier (i.e. does not wait till instantiation). Since 'allocator' and 'b' aren't dependent, I think this is a valid diagnosis. GCC throws an error on this code upon instantiation. https://godbolt.org/z/X9Y-Vy

The original non-reduced case does build fine with GCC though (I'm fairly sure). I can try to reduce one later that still builds with GCC but fails with current clang.

Yes. The reduced example here builds fine with GCC as well, unless template is instantiated. However I don't think Clang always matches GCC behavior when diagnosing errors in templates (See example from cpplearner here - https://reviews.llvm.org/D61027). Clang diagnoses several errors even if template is not instantiated (unlike GCC). I am not sure what the 'correct' behavior is / whether we should match GCC here. @erichkeane @rnk could you please take a look?

[erichkeane]

In D69950#1771462, @eandrews wrote:

In D69950#1771340, @mstorsjo wrote:

In D69950#1771133, @eandrews wrote:

In D69950#1770138, @mstorsjo wrote:

This (when reapplied in https://reviews.llvm.org/rG878a24ee244a24c39d1c57e9af2) broke compilation of code that earlier built fine. A reduced example:

namespace glslang {
class TPoolAllocator {
  void operator=(TPoolAllocator);
};
template <class> class a {
  TPoolAllocator *b;
  void c() { allocator = *b; }
  TPoolAllocator allocator;
};
} // namespace glslang

With this patch, some errors in templates are diagnosed earlier (i.e. does not wait till instantiation). Since 'allocator' and 'b' aren't dependent, I think this is a valid diagnosis. GCC throws an error on this code upon instantiation. https://godbolt.org/z/X9Y-Vy

The original non-reduced case does build fine with GCC though (I'm fairly sure). I can try to reduce one later that still builds with GCC but fails with current clang.

Yes. The reduced example here builds fine with GCC as well, unless template is instantiated. However I don't think Clang always matches GCC behavior when diagnosing errors in templates (See example from cpplearner here - https://reviews.llvm.org/D61027). Clang diagnoses several errors even if template is not instantiated (unlike GCC). I am not sure what the 'correct' behavior is / whether we should match GCC here. @erichkeane @rnk could you please take a look?

I'll have to see the reproducer, however Clang DOES diagnose some template errors earlier than GCC in many situations. A template definition that isn't instantiatible is ill-formed (I think NDR), so if the original reproducer doesn't instantiate the template, then it is likely incorrect code.

[rnk]

In D69950#1770138, @mstorsjo wrote:

This (when reapplied in https://reviews.llvm.org/rG878a24ee244a24c39d1c57e9af2) broke compilation of code that earlier built fine. A reduced example:

namespace glslang {
class TPoolAllocator {
  void operator=(TPoolAllocator);
};
template <class> class a {
  TPoolAllocator *b;
  void c() { allocator = *b; }
  TPoolAllocator allocator;
};
} // namespace glslang

I fixed this particular code upstream:
https://github.com/KhronosGroup/glslang/pull/2010
I am not enough an expert to be sure, but I suspect this is in the area of "invalid, no diagnostic required", where this code is invalid, but a conforming C++ implementation could either reject or accept it. Now we reject it, and that seems better in the long term, even though it creates a fire drill in the short term. =(

[mstorsjo]

In D69950#1771515, @rnk wrote:

I fixed this particular code upstream:
https://github.com/KhronosGroup/glslang/pull/2010
I am not enough an expert to be sure, but I suspect this is in the area of "invalid, no diagnostic required", where this code is invalid, but a conforming C++ implementation could either reject or accept it. Now we reject it, and that seems better in the long term, even though it creates a fire drill in the short term. =(

Ok, thanks for the input! I'll go cherrypick that fix then.

[smeenai]

Similar to the case @mstorsjo mentioned, this also causes the following code to fail now:

struct S {
    unsigned size;
};

template <class T>
struct U {
    S s;
    unsigned size() { return s.size(); }
};

This is obviously invalid, and I personally prefer getting the error early instead of having to wait for the instantiation, but it is different from gcc and MSVC: https://godbolt.org/z/yoDDjS. I think the earlier diagnosis should be okay from a conformance point of view though (but I'm far from being well-versed with that).

clang-tools-extra/test/clang-tidy/checkers/bugprone-string-integer-assignment.cpp

 }
 
 namespace instantiation_dependent_exprs {
 template<typename T>
 struct S {
   static constexpr T t = 0x8000;
   std::string s;
   void f(char c) { s += c | static_cast<int>(t); }
+  // CHECK-MESSAGES: :[[@LINE-1]]:25: warning: an integer is interpreted as a chara
+  // CHECK-FIXES: {{^}}  void f(char c) { s += std::to_string(c | static_cast<int>(t)); } 
 };
 
 template S<int>;
 }

clang-tools-extra/test/clang-tidy/checkers/misc-unused-parameters.cpp

 struct a {
   void b(int c) {;}
 // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: parameter 'c' is unused
 // CHECK-FIXES: {{^}}  void b() {;}{{$}}
 };
 template <class>
 class d {
   a e;
-  void f() { e.b(); }
+  void f() { e.b(0); }
 };
 }  // namespace
 }  // namespace PR38055
 
 namespace strict_mode_off {
 // Do not warn on empty function bodies.
 void f1(int foo1) {}
 void f2(int foo2) {

clang/lib/AST/Expr.cpp

std::size_t Size =
                        TemplateArgumentLoc>(
           HasQualOrFound ? 1 : 0, HasTemplateKWAndArgsInfo ? 1 : 0,
           TemplateArgs ? TemplateArgs->size() : 0);
 
   void *Mem = C.Allocate(Size, alignof(MemberExpr));
   MemberExpr *E = new (Mem) MemberExpr(Base, IsArrow, OperatorLoc, MemberDecl,
                                        NameInfo, T, VK, OK, NOUR);
 
+  if (FieldDecl *Field = dyn_cast<FieldDecl>(MemberDecl)) {
+    DeclContext *DC = MemberDecl->getDeclContext();
+    // dyn_cast_or_null is used to handle objC variables which do not
+    // have a declaration context.
+    CXXRecordDecl *RD = dyn_cast_or_null<CXXRecordDecl>(DC);
+    if (RD && RD->isDependentContext() && RD->isCurrentInstantiation(DC))
+      E->setTypeDependent(T->isDependentType());
+  }
+
   if (HasQualOrFound) {
     // FIXME: Wrong. We should be looking at the member declaration we found.
     if (QualifierLoc && QualifierLoc.getNestedNameSpecifier()->isDependent()) {
       E->setValueDependent(true);
       E->setTypeDependent(true);
       E->setInstantiationDependent(true);
     }
     else if (QualifierLoc &&

clang/lib/Sema/SemaChecking.cpp

void Sema::RefersToMemberWithReducedAlignment(
 
   // For a chain of MemberExpr like "a.b.c.d" this list
   // will keep FieldDecl's like [d, c, b].
   SmallVector<FieldDecl *, 4> ReverseMemberChain;
   const MemberExpr *TopME = nullptr;
   bool AnyIsPacked = false;
   do {
     QualType BaseType = ME->getBase()->getType();
+    if (BaseType->isDependentType())
+      return;
     if (ME->isArrow())
       BaseType = BaseType->getPointeeType();
     RecordDecl *RD = BaseType->castAs<RecordType>()->getDecl();
     if (RD->isInvalidDecl())
       return;
 
     ValueDecl *MD = ME->getMemberDecl();
     auto *FD = dyn_cast<FieldDecl>(MD);

clang/test/SemaCXX/constant-expression-cxx2a.cpp

namespace std {
   using size_t = decltype(sizeof(0));
   enum class align_val_t : size_t {};
 };
 
 [[nodiscard]] void *operator new(std::size_t, const std::nothrow_t&) noexcept;
 [[nodiscard]] void *operator new(std::size_t, std::align_val_t, const std::nothrow_t&) noexcept;
 [[nodiscard]] void *operator new[](std::size_t, const std::nothrow_t&) noexcept;
 [[nodiscard]] void *operator new[](std::size_t, std::align_val_t, const std::nothrow_t&) noexcept;
+[[nodiscard]] void *operator new[](std::size_t, std::align_val_t);
 void operator delete(void*, const std::nothrow_t&) noexcept;
 void operator delete(void*, std::align_val_t, const std::nothrow_t&) noexcept;
 void operator delete[](void*, const std::nothrow_t&) noexcept;
 void operator delete[](void*, std::align_val_t, const std::nothrow_t&) noexcept;
 
 // Helper to print out values for debugging.
 constexpr void not_defined();
 template<typename T> constexpr void print(T) { not_defined(); }
template<typename T> struct X {
     char *p;
     void dependent();
   };
   template<typename T> void X<T>::dependent() {
     char *p;
     // Ensure that we don't try to evaluate these for overflow and crash. These
     // are all value-dependent expressions.
     p = new char[n];
-    p = new (n) char[n];
+    p = new ((std::align_val_t)n) char[n];
     p = new char(n);
   }
 }
 
 struct placement_new_arg {};
 void *operator new(std::size_t, placement_new_arg);
 void operator delete(void*, placement_new_arg);
 

clang/test/SemaTemplate/dependent-names.cpp

namespace A1 {
     template<typename T>
     struct S {
       void f() {
         for (auto &a : e)
           __range(a); // expected-error {{undeclared identifier '__range'}}
       }
       int e[10];
     };
-    void g() {
-      S<int>().f(); // expected-note {{here}}
-    }
   }
 
   namespace A2 {
     template<typename T>
     struct S {
       void f() {
         for (auto &a : e)
           __range(a); // expected-error {{undeclared identifier '__range'}}

clang/test/SemaTemplate/enum-argument.cpp

 // RUN: %clang_cc1 -fsyntax-only -verify %s
-// expected-no-diagnostics
 
 enum Enum { val = 1 };
 template <Enum v> struct C {
   typedef C<v> Self;
 };
 template struct C<val>;
 
 template<typename T>

 namespace rdar8020920 {
   template<typename T>
   struct X {
     enum { e0 = 32 };
 
     unsigned long long bitfield : e0;
 
     void f(int j) {
-      bitfield + j;
+      bitfield + j; // expected-warning {{expression result unused}}
     }
   };
 }

clang/test/SemaTemplate/member-access-expr.cpp

namespace test6 {
   struct A {};
   struct B {};
   template <class T> class Base;
   template <class T> class Derived : public Base<T> {
     A *field;
     void get(B **ptr) {
       // It's okay if at some point we figure out how to diagnose this
       // at instantiation time.
-      *ptr = field;
+      *ptr = field; // expected-error {{assigning to 'test6::B *' from incompatible type 'test6::A *}}
     }
   };
 }

clang/test/SemaTemplate/non-integral-switch-cond.cpp

+// RUN: %clang_cc1 -fsyntax-only -verify %s
+
+struct NOT_AN_INTEGRAL_TYPE {};
+
+template <typename T>
+struct foo {
+  NOT_AN_INTEGRAL_TYPE Bad;
+  void run() {
+    switch (Bad) { // expected-error {{statement requires expression of integer type ('NOT_AN_INTEGRAL_TYPE' invalid)}}
+    case 0:
+      break;
+    }
+  }
+};