@llvm/pr-subscribers-clang

Author: Hans Wennborg (zmodem)

diff --git a/clang/lib/Sema/SemaStmt.cpp b/clang/lib/Sema/SemaStmt.cpp
index e8c1f8490342a..990d2fadaf5aa 100644
--- a/clang/lib/Sema/SemaStmt.cpp
+++ b/clang/lib/Sema/SemaStmt.cpp
@@ -1667,8 +1667,12 @@ Sema::ActOnFinishSwitchStmt(SourceLocation SwitchLoc, Stmt *Switch,
         // Don't warn about omitted unavailable EnumConstantDecls.
         switch (EI->second->getAvailability()) {
         case AR_Deprecated:
-          // Omitting a deprecated constant is ok; it should never materialize.
+          // Deprecated enumerators still need to be handled: they may be
+          // deprecated, but can still occur.
+          break;
+
         case AR_Unavailable:
+          // Omitting an unavailable enumerator is ok; it should never occur.
           continue;
 
         case AR_NotYetIntroduced:
diff --git a/clang/test/Sema/switch-availability.c b/clang/test/Sema/switch-availability.c
index 888edddac463d..b4f8726addc0b 100644
--- a/clang/test/Sema/switch-availability.c
+++ b/clang/test/Sema/switch-availability.c
@@ -15,7 +15,7 @@ enum SwitchTwo {
 };
 
 void testSwitchTwo(enum SwitchTwo st) {
-  switch (st) {} // expected-warning{{enumeration values 'Vim' and 'Emacs' not handled in switch}}
+  switch (st) {} // expected-warning{{enumeration values 'Ed', 'Vim' and 'Emacs' not handled in switch}}
 }
 
 enum SwitchThree {

Fly-by comment: I drafted the following alternative comment in another thread. Please feel free to disregard as I don't know the tone and style of comments in these files. (But I appreciate your consideration for the contents in the message I'd like to see!)

      // We currently treat deprecated constants as if they don't exist;
      // however, note that this current behavior leads to compile-time
      // false negatives for coverage checking of switch statements. If
      // a switch is missing a case for a deprecated constant, we will not
      // emit a diagnostic, even though the deprecated constant might still
      // be present in legacy use.

Fly-by comment: I drafted the following alternative comment in another thread. Please feel free to disregard as I don't know the tone and style of comments in these files. (But I appreciate your consideration for the contents in the message I'd like to see!)

      // We currently treat deprecated constants as if they don't exist;
      // however, note that this current behavior leads to compile-time
      // false negatives for coverage checking of switch statements. If
      // a switch is missing a case for a deprecated constant, we will not
      // emit a diagnostic, even though the deprecated constant might still
      // be present in legacy use.

My patch changes the behavior so that deprecated constants are not ignored by the switch coverage warning.

I wonder if we should also do something special about the -Wdeprecated-declarations diagnostic in switches.

If the user fixes the -Wswitch / -Wreturn-type warnings from testSwitchFour the natural way:

int testSwitchFour(enum SwitchFour e) {
  switch (e) {
  case Red:   return 1;
  case Green: return 2;
  case Blue:  return 3;
  }
} 

They will get warning: 'Blue' is deprecated [-Wdeprecated-declarations] instead.

Maybe we should suppress that warning for switch cases? Or suggest adding a default label instead?

Maybe we should suppress that warning for switch cases? Or suggest adding a default label instead?

I think silencing the warning is better than suggesting a default case, which may not be considered good practice.
Maybe we can add a -Wdeprecated-switch-case warning that people can control independently of -Wdeprecated-declarations

I've added suppression of deprecated values in case expressions. Please take another look!

Now we have a special Wdeprecated-declarations carveouts for switches, but if you unpack the switch into if / else chain comparisons, you get deprecation warnings.

Agreed, that it is a bit inconsistent. There is prior art though: ba87c62 makes exactly the same carve-out for -Wunguarded-availability (though the mechanism is different).

I wonder if we should also do something special about the -Wdeprecated-declarations diagnostic in switches.

If the user fixes the -Wswitch / -Wreturn-type warnings from testSwitchFour the natural way:

int testSwitchFour(enum SwitchFour e) {
  switch (e) {
  case Red:   return 1;
  case Green: return 2;
  case Blue:  return 3;
  }
} 

They will get warning: 'Blue' is deprecated [-Wdeprecated-declarations] instead.

Maybe we should suppress that warning for switch cases? Or suggest adding a default label instead?

I don't think we should suppress the warning; the enumerator is deprecated and the user should be told that when they enable deprecation warnings. Isn't the better solution in this case for the user to use __builtin_unreachable outside of the switch so they don't have to mention the deprecated case, and locally suppress -Wswitch if they intentionally do not want the switch to be fully covered but they want other switches to continue to be checked?

I don't think we should suppress the warning; the enumerator is deprecated and the user should be told that when they enable deprecation warnings.

But the user also needs to handle it in the switch: deprecated doesn't mean the enumerator can't occur (a recent experience of this motivated the patch ;)

Isn't the better solution in this case for the user to use __builtin_unreachable outside of the switch so they don't have to mention the deprecated case, and locally suppress -Wswitch if they intentionally do not want the switch to be fully covered but they want other switches to continue to be checked?

I'm not sure I'm following. __builtin_unreachable wouldn't suppress the -Wswitch warning, and anyway we don't want to trigger UB when the deprecated enumerator comes up, we want to handle it.

I think this is exactly ba87c62 but for deprecated constants, and that we should handle it the same.

Maybe we can add a -Wdeprecated-switch-case warning that people can control independently of -Wdeprecated-declarations

I'd be okay with this too. We'd turn it off though, and would it actually catch real issues?

I don't think we should suppress the warning; the enumerator is deprecated and the user should be told that when they enable deprecation warnings.

But the user also needs to handle it in the switch: deprecated doesn't mean the enumerator can't occur (a recent experience of this motivated the patch ;)

Sure! But both sides of that are "the problem"; once you mark the enumerator as deprecated, you'll get diagnostics when you assign that value or when you test against that value.

Isn't the better solution in this case for the user to use __builtin_unreachable outside of the switch so they don't have to mention the deprecated case, and locally suppress -Wswitch if they intentionally do not want the switch to be fully covered but they want other switches to continue to be checked?

I'm not sure I'm following. __builtin_unreachable wouldn't suppress the -Wswitch warning, and anyway we don't want to trigger UB when the deprecated enumerator comes up, we want to handle it.

Oh good point, that case still needs to exist to handle the enumerator. I was thinking about it handling the -Wreturn-type diagnostic.

I think this is exactly ba87c62 but for deprecated constants, and that we should handle it the same.

I can see the rationale for handling both the same way. But the flip side is, then you get false negatives which can also come up legitimately. Consider:

// Enumeration which has existed since the dawn of time.
enum Stuff {
  GoodStuff,
  BetterStuff,
  BsetStuff [[deprecated("use BestStuff instead")]], // Ugh, typos are the worst.
  BestStuff = BesterStuff
};
...
// Brand-new code written just now
void do_interesting_things(Stuff S) {
  switch (S) {
  case GoodStuff: whatever(); break;
  case BetterStuff: whatever_else(); break;
  case BsetStuff: wahoo(); break; // Oops, meant to use BestStuff instead
  }
}

That code will continue to work great right up until the enumerator is removed.

I think silencing the warning is better than suggesting a default case, which may not be considered good practice.

I'm not sure, I think our perspectives as compiler people might be a bit off base. We're always forming closed algebraic sum types, like variants, AST nodes, that kind of thing. I think normal programs dealing with untrusted input coming from a file or over the wire typically need to be more conservative, and pushing users towards adding a default case or code to the fallthrough block without explicitly mentioning the deprecated enumerator might be the best outcome. Consider that -Wcovered-switch-default is not part of -Wall/-Wextra: https://godbolt.org/z/PrjqqxKrP

That code will continue to work great right up until the enumerator is removed.

To Aaron's point about deprecation being a tool for removal, if the goal of deprecation is to remove the enumerator, then we do want folks to use default/fallthrough code patterns that don't explicitly mention the enum.

Thanks Aaron, that's a good example.

This is a pickle; it doesn't seem like there's an obviously Right Solution(tm) here.

I think we're agreeing on the first part, that unhandled deprecated enums should trigger the "not covered" warning.

Some users will prefer to handle that with a default: to avoid using the deprecated enum, while others will want to handle the enum explicitly and suppress the warning.

Corentin's flag suggestion seems like the best way to satisfy both camps. I've updated the PR.

Thanks Aaron, that's a good example.

This is a pickle; it doesn't seem like there's an obviously Right Solution(tm) here.

That's the same conclusion I'm coming to. These situations are kind of mutually exclusive.

I think we're agreeing on the first part, that unhandled deprecated enums should trigger the "not covered" warning.

Yup!

Some users will prefer to handle that with a default: to avoid using the deprecated enum, while others will want to handle the enum explicitly and suppress the warning.

Corentin's flag suggestion seems like the best way to satisfy both camps. I've updated the PR.

I agree, thank you for heading that direction (and good suggestion @cor3ntin)!