[C11] Diagnose unreachable generic selection associations

[aaron.ballman]

Summary

The controlling expression of a _Generic selection expression undergoes lvalue conversion, array conversion, and function conversion before picking the association. This means that array types, function types, and qualified types are all unreachable code if they're used as an association. I've been caught by this twice in the past few months and I figure that if a WG14 member can't seem to remember this rule, users are also likely to struggle with it. So this adds an on-by-default unreachable code diagnostic for generic selection expression associations.

Note, we don't have to worry about function types as those are already a constraint violation which generates an error.

[erichkeane]

One minor preference, otherwise LGTM.

clang/lib/Sema/SemaExpr.cpp

1702

Splitting this up into the separate line for only 2 items seems like a waste, would it look better to just do the diag in each 'if'?

[tahonermann]

Looks good to me!

[aaron.ballman]

clang/lib/Sema/SemaExpr.cpp

1702

I tried it both ways and I think using only one call to Diag is better (it makes it easier if we want to update the diagnostic text in the future, say to add more information to it like a source range, etc). However, I don't feel super strongly -- if you'd like to see it changed still, I can do so.

[erichkeane]

clang/lib/Sema/SemaExpr.cpp

1702

I don't feel super strongly either. It just seemed to be 4 lines of rigamarole to 'save' 1 line of copy/paste :D So the judgement comes down to 'how much do we expect the message format/reasons to emit it' will change vs the extra mental 'overhead'.

Since you already committed it, I'd say it is not worth a cleanup patch.

[aeubanks]

$ cat /tmp/a.cc
typedef struct Test {
} Test;

void f() {
  const Test constVal;
  _Generic(constVal, const Test : 0);
}
$ ./build/rel/bin/clang -fsyntax-only  -x c++ /tmp/a.cc -Wall
/tmp/a.cc:6:28: warning: due to lvalue conversion of the controlling expression, association of type 'const Test' will never be selected because it is qualified [-Wunreachable-code-generic-assoc]
  _Generic(constVal, const Test : 0);
                           ^
/tmp/a.cc:6:35: warning: expression result unused [-Wunused-value]
  _Generic(constVal, const Test : 0);
                                  ^
2 warnings generated.
$ ./build/rel/bin/clang -fsyntax-only  -x c /tmp/a.cc -Wall
/tmp/a.cc:6:28: warning: due to lvalue conversion of the controlling expression, association of type 'const Test' (aka 'const struct Test') will never be selected because it is qualified [-Wunreachable-code-generic-assoc]
  _Generic(constVal, const Test : 0);
                           ^
/tmp/a.cc:6:12: error: controlling expression type 'Test' (aka 'struct Test') not compatible with any generic association type
  _Generic(constVal, const Test : 0);
           ^~~~~~~~
1 warning and 1 error generated.

the C++ case looks wrong, it's warning that const Test can't be selected then selects it

[aaron.ballman]

In D125259#3519822, @aeubanks wrote:

$ cat /tmp/a.cc
typedef struct Test {
} Test;

void f() {
  const Test constVal;
  _Generic(constVal, const Test : 0);
}
$ ./build/rel/bin/clang -fsyntax-only  -x c++ /tmp/a.cc -Wall
/tmp/a.cc:6:28: warning: due to lvalue conversion of the controlling expression, association of type 'const Test' will never be selected because it is qualified [-Wunreachable-code-generic-assoc]
  _Generic(constVal, const Test : 0);
                           ^
/tmp/a.cc:6:35: warning: expression result unused [-Wunused-value]
  _Generic(constVal, const Test : 0);
                                  ^
2 warnings generated.
$ ./build/rel/bin/clang -fsyntax-only  -x c /tmp/a.cc -Wall
/tmp/a.cc:6:28: warning: due to lvalue conversion of the controlling expression, association of type 'const Test' (aka 'const struct Test') will never be selected because it is qualified [-Wunreachable-code-generic-assoc]
  _Generic(constVal, const Test : 0);
                           ^
/tmp/a.cc:6:12: error: controlling expression type 'Test' (aka 'struct Test') not compatible with any generic association type
  _Generic(constVal, const Test : 0);
           ^~~~~~~~
1 warning and 1 error generated.

the C++ case looks wrong, it's warning that const Test can't be selected then selects it

Oh wow, good catch! I'll correct this.

[aaron.ballman]

In D125259#3519947, @aaron.ballman wrote:

Oh wow, good catch! I'll correct this.

Oof, the plot thickens... the diagnostic is correct for some types, but is incorrect for others: https://godbolt.org/z/fahzx53W6. I also discovered a parsing issue in C++ where we get confused by elaborated type specifiers: https://godbolt.org/z/1ejxqd9ss.

I'm questioning the benefit to supporting _Generic in C++ and starting to wonder if perhaps we should pull this extension back. C++ already has a rich type system that can accomplish the same thing and we're not following the C semantics so there's not really an argument to be made for the extension letting people write code in both languages. @rsmith, do you have thoughts about that?

[tahonermann]

I'm questioning the benefit to supporting _Generic in C++ and starting to wonder if perhaps we should pull this extension back.

I imagine it may be used in shared system headers. E.g., implementations of tgmath.h that don't use the builtins.

[aaron.ballman]

In D125259#3520544, @tahonermann wrote:

I'm questioning the benefit to supporting _Generic in C++ and starting to wonder if perhaps we should pull this extension back.

I imagine it may be used in shared system headers. E.g., implementations of tgmath.h that don't use the builtins.

That's a good thought! I'm not seeing much evidence of that, at least from my code searches: https://sourcegraph.com/search?q=context:global+file:tgmath%5C.h+_Generic+AND+%28%23ifdef+__cplusplus+OR+%23if+defined%5C%28__cplusplus%5C%29%29&patternType=literal&case=yes

You can definitely find cases of #ifndef __cplusplus in the same code search though (which I've seen protecting uses of _Generic in the file). Also, our own tgmath.h implementation is not using _Generic (we use __attribute__((overloadable)) instead).

[aaron.ballman]

In D125259#3520118, @aaron.ballman wrote:

In D125259#3519947, @aaron.ballman wrote:

Oh wow, good catch! I'll correct this.

Oof, the plot thickens... the diagnostic is correct for some types, but is incorrect for others: https://godbolt.org/z/fahzx53W6. I also discovered a parsing issue in C++ where we get confused by elaborated type specifiers: https://godbolt.org/z/1ejxqd9ss.

https://reviews.llvm.org/D125882 addresses the diagnostic behavior for the unreachable warning.

[aaron.ballman]

In D125259#3522198, @aaron.ballman wrote:

In D125259#3520118, @aaron.ballman wrote:

In D125259#3519947, @aaron.ballman wrote:

Oh wow, good catch! I'll correct this.

Oof, the plot thickens... the diagnostic is correct for some types, but is incorrect for others: https://godbolt.org/z/fahzx53W6. I also discovered a parsing issue in C++ where we get confused by elaborated type specifiers: https://godbolt.org/z/1ejxqd9ss.

https://reviews.llvm.org/D125882 addresses the diagnostic behavior for the unreachable warning.

I landed that fix in 47b8424a533d5c02fd8b3517047cf93e533f00d0, thank you for pointing it out @aeubanks!

clang/docs/ReleaseNotes.rst

 - ``-Wmisexpect`` warns when the branch weights collected during profiling
   conflict with those added by ``llvm.expect``.
 - ``-Wthread-safety-analysis`` now considers overloaded compound assignment and
   increment/decrement operators as writing to their first argument, thus
   requiring an exclusive lock if the argument is guarded.
 - ``-Wenum-conversion`` now warns on converting a signed enum of one type to an
   unsigned enum of a different type (or vice versa) rather than
   ``-Wsign-conversion``.
+- Added the ``-Wunreachable-code-generic-assoc`` diagnostic flag (grouped under
+  the ``-Wunreachable-code`` flag) which is enabled by default and warns the
+  user about ``_Generic`` selection associations which are unreachable because
+  the type specified is an array type or a qualified type.
 
 Non-comprehensive list of changes in this release
 -------------------------------------------------
 
 - Improve __builtin_dump_struct:
 
   - Support bitfields in struct and union.
   - Improve the dump format, dump both bitwidth(if its a bitfield) and field

clang/include/clang/Basic/DiagnosticGroups.td

 // Unreachable code warning groups.
 //
 //  The goal is make -Wunreachable-code on by default, in -Wall, or at
 //  least actively used, with more noisy versions of the warning covered
 //  under separate flags.
 //
 def UnreachableCodeLoopIncrement : DiagGroup<"unreachable-code-loop-increment">;
 def UnreachableCodeFallthrough : DiagGroup<"unreachable-code-fallthrough">;
+def UnreachableCodeGenericAssoc : DiagGroup<"unreachable-code-generic-assoc">;
 def UnreachableCode : DiagGroup<"unreachable-code",
                                 [UnreachableCodeLoopIncrement,
-                                 UnreachableCodeFallthrough]>;
+                                 UnreachableCodeFallthrough,
+                                 UnreachableCodeGenericAssoc]>;
 def UnreachableCodeBreak : DiagGroup<"unreachable-code-break">;
 def UnreachableCodeReturn : DiagGroup<"unreachable-code-return">;
 def UnreachableCodeAggressive : DiagGroup<"unreachable-code-aggressive",
                                     [UnreachableCode,
                                      UnreachableCodeBreak,
                                      UnreachableCodeReturn]>;
 
 // Aggregation warning settings.

clang/include/clang/Basic/DiagnosticSemaKinds.td

 def warn_unreachable_loop_increment : Warning<
   "loop will run at most once (loop increment never executed)">,
   InGroup<UnreachableCodeLoopIncrement>, DefaultIgnore;
 def warn_unreachable_fallthrough_attr : Warning<
   "fallthrough annotation in unreachable code">,
   InGroup<UnreachableCodeFallthrough>, DefaultIgnore;
 def note_unreachable_silence : Note<
   "silence by adding parentheses to mark code as explicitly dead">;
+def warn_unreachable_association : Warning<
+  "due to lvalue conversion of the controlling expression, association of type "
+  "%0 will never be selected because it is %select{of array type|qualified}1">,
+  InGroup<UnreachableCodeGenericAssoc>;
 
 /// Built-in functions.
 def ext_implicit_lib_function_decl : ExtWarn<
   "implicitly declaring library function '%0' with type %1">,
   InGroup<ImplicitFunctionDeclare>;
 def ext_implicit_lib_function_decl_c99 : ExtWarn<
   "call to undeclared library function '%0' with type %1; ISO C99 and later "
   "do not support implicit function declarations">,

clang/lib/Sema/SemaExpr.cpp

if (Types[i]) {
         // complete object type other than a variably modified type."
         unsigned D = 0;
         if (Types[i]->getType()->isIncompleteType())
           D = diag::err_assoc_type_incomplete;
         else if (!Types[i]->getType()->isObjectType())
           D = diag::err_assoc_type_nonobject;
         else if (Types[i]->getType()->isVariablyModifiedType())
           D = diag::err_assoc_type_variably_modified;
+        else {
+          // Because the controlling expression undergoes lvalue conversion,
+          // array conversion, and function conversion, an association which is
+          // of array type, function type, or is qualified can never be
+          // reached. We will warn about this so users are less surprised by
+          // the unreachable association. However, we don't have to handle
+          // function types; that's not an object type, so it's handled above.
+          unsigned Reason = 0;
+          QualType QT = Types[i]->getType();
+          if (QT->isArrayType())
+            Reason = 1;
+          else if (QT.hasQualifiers())
+            Reason = 2;
+
+          if (Reason)
+            Diag(Types[i]->getTypeLoc().getBeginLoc(),
+                 diag::warn_unreachable_association)
+                << QT << (Reason - 1);
+        }
 
         if (D != 0) {
           Diag(Types[i]->getTypeLoc().getBeginLoc(), D)
             << Types[i]->getTypeLoc().getSourceRange()
             << Types[i]->getType();
           TypeErrorFound = true;
         }
 

clang/test/Sema/generic-selection.c

void GH50227(void) {
   // Previously, the controlling expression for the outer _Generic makes it
   // result dependent, and testing whether that controlling expression has side
   // effects would cause a crash.
   _Generic( // ext-warning {{'_Generic' is a C11 extension}}
     n(
       _Generic(n++, int : 0) // expected-error {{cannot increment value of type 'int ()'}} ext-warning {{'_Generic' is a C11 extension}}
     ), int : 0);
 }
+
+void unreachable_associations(const int i) {
+  _Static_assert( // ext-warning {{'_Static_assert' is a C11 extension}}
+    _Generic(i, // ext-warning {{'_Generic' is a C11 extension}}
+      const int : 1,    // expected-warning {{due to lvalue conversion of the controlling expression, association of type 'const int' will never be selected because it is qualified}}
+      volatile int : 2, // expected-warning {{due to lvalue conversion of the controlling expression, association of type 'volatile int' will never be selected because it is qualified}}
+      int[12] : 3,      // expected-warning {{due to lvalue conversion of the controlling expression, association of type 'int[12]' will never be selected because it is of array type}}
+      int : 4,
+      default : 5
+    ) == 4, "we had better pick int!");
+}