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

[eandrews]

Summary

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. A few lit tests started to fail once I applied this patch because errors are now diagnosed earlier (does not wait till instantiation). I've modified these tests.

This patch fixes Bug 40982. Please note my earlier attempt at fixing this bug delayed checks for dependent expressions till instantiation. I changed the approach after feedback from Richard and cpplearner (ensadc) - https://bugs.llvm.org/show_bug.cgi?id=40982

[eandrews]

I apologize for the confusion. I was working on an incorrect branch earlier, and so abandoned that patch and uploaded a new revision.

[cpplearner]

(I am ensadc at bugzilla)

Does this cause regression in the following case?

template<int *N>
void f() {
    switch (N) case 0:; // should be diagnosed
    switch (0) case N:; // should be diagnosed
}

Currently clang diagnoses these switch statements even if the template is not instantiated, GCC does not.

AFAIK both compilers are correct under [temp.res]/8, but clang's current behavior seems more helpful.

[eandrews]

I ran the test you provided and it does throw errors without instantiation

bash-4.2$ clang -cc1 test/SemaTemplate/test2.cpp
test/SemaTemplate/test2.cpp:3:7: error: statement requires expression of integer type ('int *' invalid)
      switch (N) case 0:; // should be diagnosed
      ^       ~
test/SemaTemplate/test2.cpp:4:23: error: value of type 'int *' is not implicitly convertible to 'int'
      switch (0) case N:; // should be diagnosed
                      ^
test/SemaTemplate/test2.cpp:4:25: warning: switch statement has empty body
      switch (0) case N:; // should be diagnosed
                        ^
test/SemaTemplate/test2.cpp:4:25: note: put the semicolon on a separate line to silence this warning
1 warning and 2 errors generated.

However, I am surprised it does since I expected it not to :) For example the lit test in this patch will not throw an error without the instantiation. I need to debug this further to understand what's happening. Thanks for taking a look!

[eandrews]

ping

I just realized I modified the summary when I uploaded a new patch, but did not add a comment about the changes I made. Based on feedback from the bug report (https://bugs.llvm.org/show_bug.cgi?id=40982), I changed my approach for this crash. This patch changes the type dependency of the member which causes the crash.

Currently, when member expressions are created, their type dependency is set based on the class it is part of and not its own type. I don't quite understand why, and changing it broke a whole lot of lit tests. So, in this patch I only attempted to modify the type-dependency of 'members of current instantiation' to set it based on it's type as opposed to the containing classes' type.

[eandrews]

ping

[rnk]

Looks good to me. I was hoping Richard would take a look, but I reproduced the crash, and I'd rather see the fix land sooner than later. Thanks!

[eandrews]

Thanks for taking a look Reid! I rebased the patch and had a conflict with one of Richard's patches and so ran the lit tests again and noticed there are several new failures. I am taking a look at those now.

[gribozavr]

Sorry, but this change broke ClangTidy tests: http://lab.llvm.org:8011/builders/clang-x86_64-debian-fast/builds/16398. I reverted it.

[eandrews]

In D61027#1627655, @gribozavr wrote:

Sorry, but this change broke ClangTidy tests: http://lab.llvm.org:8011/builders/clang-x86_64-debian-fast/builds/16398. I reverted it.

Yes I've been trying to reproduce it locally. Could you please let me know how you run clang-tidy tests? check-clang and check-all isn't showing this failure locally. I tried setting up clang tooling as described here - http://clang.llvm.org/docs/HowToSetupToolingForLLVM.html but I don't find clang-tidy in builds

[riccibruno]

I think that you have to run check-clang-tools. I too was surprised that it was not included in check-clang.

[gribozavr]

Like @riccibruno said, check-clang-tools will run them. However, before committing a patch, please run check-all -- you never know what your patch can affect.

[Szelethus]

In D61027#1627785, @gribozavr wrote:

Like @riccibruno said, check-clang-tools will run them. However, before committing a patch, please run check-all -- you never know what your patch can affect.

And also you'll need to have the clang-tools-extra project enabled in the CMake configuration.

[eandrews]

Thank you for letting me know. I did try reproducing the issue with check-all yesterday but was unable to. I do not think I build with 'clang-tools-extra' project enabled though. I'll take another look today.

lib/Sema/SemaStmt.cpp

Sema::ActOnCaseExpr(SourceLocation CaseLoc, ExprResult Val) {
   if (!getCurFunction()->SwitchStack.empty()) {
     Expr *CondExpr =
         getCurFunction()->SwitchStack.back().getPointer()->getCond();
     if (!CondExpr)
       return ExprError();
     QualType CondType = CondExpr->getType();
 
     auto CheckAndFinish = [&](Expr *E) {
-      if (CondType->isDependentType() || E->isTypeDependent())
+      if (CondType->isDependentType() || CondExpr->isInstantiationDependent() ||
+          E->isTypeDependent())
         return ExprResult(E);
 
       if (getLangOpts().CPlusPlus11) {
         // C++11 [stmt.switch]p2: the constant-expression shall be a converted
         // constant expression of the promoted type of the switch condition.
         llvm::APSInt TempVal;
         return CheckConvertedConstantExpression(E, CondType, TempVal,
                                                 CCEK_CaseValue);
ExprResult Sema::CheckSwitchCondition(SourceLocation SwitchLoc, Expr *Cond) {
   return UsualUnaryConversions(Cond);
 }
 
 StmtResult Sema::ActOnStartOfSwitchStmt(SourceLocation SwitchLoc,
                                         Stmt *InitStmt, ConditionResult Cond) {
   Expr *CondExpr = Cond.get().second;
   assert((Cond.isInvalid() || CondExpr) && "switch with no condition");
 
-  if (CondExpr && !CondExpr->isTypeDependent()) {
+  if (CondExpr && !CondExpr->isTypeDependent() &&
+      !CondExpr->isInstantiationDependent()) {
     // We have already converted the expression to an integral or enumeration
     // type, when we parsed the switch condition. If we don't have an
     // appropriate type now, enter the switch scope but remember that it's
     // invalid.
     assert(CondExpr->getType()->isIntegralOrEnumerationType() &&
            "invalid condition type");
     if (CondExpr->isKnownToHaveBooleanValue()) {
       // switch(bool_expr) {...} is often a programmer error, e.g.

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;
+    }
+  }
+};
+
+int main() {
+  foo<int> instance;
+  instance.run(); // expected-note {{in instantiation of member function 'foo<int>::run' requested here}}
+}