-Wunreachable-code: Avoid multiple diagnostics that are triggered by the same source range and fix the unary operator fixit source range

[arphaman]

Summary

This patch fixes an issue with -Wunreachable-code diagnostic that happens with the following code sample:

struct s {
  void *p;
};

void fn(struct s *s) {
  if (1 || !s->p)
    return;
  fn(s);
}

Currently, clang will emit two warnings (at fn(s) and at !s->p). As well as that, the fixits for the first warning (for fn(s)) have an incorrect source range: it suggests to wrap !s->p in parentheses when it should suggest to wrap 1 instead.

The attached patch ensures that the fixits for the fn(s) warning suggest to wrap the correct expression (1). It also avoids the second warning that was previously shown for !s->p because that warning is caused by the same expression (1).

Thanks for taking a look.

[ahatanak]

If I change the condition to the following,

if (!s->p || 1)

clang suggests enclosing !s->p with a parenthesis, but the comment in ReachableCode.cpp says the parenthesis should enclose the integer literal. It seems like there is a contradiction here?

[arphaman]

You're right, the fixit for if (!s->p || 1) is wrong, but that was another existing bug. The updated patch fixes this issue.

[ahatanak]

lib/Analysis/ReachableCode.cpp

229 ↗

(On Diff #83032)

Thanks, that fixed the incorrect fixit. The patch looks good to me, but there are still cases in which the suggestions clang makes are not accurate. Perhaps you can leave a FIXME somewhere so that we don't forget to fix it later?

For example, if we change the condition in the test case to this,

if (!(s->p && 0))

, clang suggests enclosing the entire expression with a parenthesis (caret points to "!"), but the warning will not go away unless the parenthesis is around "0".

The second example is in function unaryOpFixit in warn-unreachable.c. clang suggests enclosing "-1" with a parenthesis, but it still warns after putting the parenthesis around "-1" or "1".

[arphaman]

Fix more issues with FIXIT for unary operators.

[arphaman]

lib/Analysis/ReachableCode.cpp

229 ↗

(On Diff #83032)

Thanks for pointing out these issues, I've decided to fix them in this patch. Since only the '!' operator can wrap a silenced expression, the updated patch ensures that fixits for unary operators can only be used with '!' (this fixes your second example). I fixed the first example by ensuring that the unary operator source range can be used for fixit only if the previous fixit source range comes from its direct child.

[ahatanak]

Thanks, LGTM

cfe/trunk/lib/Analysis/ReachableCode.cpp

case Stmt::BinaryOperatorClass: {
       IncludeIntegers &= (B->isLogicalOp() || B->isComparisonOp());
       return isConfigurationValue(B->getLHS(), PP, SilenceableCondVal,
                                   IncludeIntegers) ||
              isConfigurationValue(B->getRHS(), PP, SilenceableCondVal,
                                   IncludeIntegers);
     }
     case Stmt::UnaryOperatorClass: {
       const UnaryOperator *UO = cast<UnaryOperator>(S);
-      if (SilenceableCondVal) 
-        *SilenceableCondVal = UO->getSourceRange();      
-      return UO->getOpcode() == UO_LNot &&
-             isConfigurationValue(UO->getSubExpr(), PP, SilenceableCondVal,
-                                  IncludeIntegers, WrappedInParens);
+      if (UO->getOpcode() != UO_LNot)
+        return false;
+      bool SilenceableCondValNotSet =
+          SilenceableCondVal && SilenceableCondVal->getBegin().isInvalid();
+      bool IsSubExprConfigValue =
+      // Update the silenceable condition value source range only if the range
+      // was set directly by the child expression.
+      if (SilenceableCondValNotSet &&
+          SilenceableCondVal->getBegin().isValid() &&
+          *SilenceableCondVal ==
+              UO->getSubExpr()->IgnoreCasts()->getSourceRange())
+        *SilenceableCondVal = UO->getSourceRange();
+      return IsSubExprConfigValue;
     }
     default:
       return false;
   }
 }
 
 static bool isConfigurationValue(const ValueDecl *D, Preprocessor &PP) {
   if (const EnumConstantDecl *ED = dyn_cast<EnumConstantDecl>(D))

cfe/trunk/lib/Sema/AnalysisBasedWarnings.cpp

 
 //===----------------------------------------------------------------------===//
 // Unreachable code analysis.
 //===----------------------------------------------------------------------===//
 
 namespace {
   class UnreachableCodeHandler : public reachable_code::Callback {
     Sema &S;
+    SourceRange PreviousSilenceableCondVal;
+
   public:
     UnreachableCodeHandler(Sema &s) : S(s) {}
 
     void HandleUnreachable(reachable_code::UnreachableKind UK,
                            SourceLocation L,
                            SourceRange SilenceableCondVal,
                            SourceRange R1,
                            SourceRange R2) override {
+      // Avoid reporting multiple unreachable code diagnostics that are
+      // triggered by the same conditional value.
+      if (PreviousSilenceableCondVal.isValid() &&
+          SilenceableCondVal.isValid() &&
+          PreviousSilenceableCondVal == SilenceableCondVal)
+        return;
+      PreviousSilenceableCondVal = SilenceableCondVal;
+
       unsigned diag = diag::warn_unreachable;
       switch (UK) {
         case reachable_code::UK_Break:
           diag = diag::warn_unreachable_break;
           break;
         case reachable_code::UK_Return:
           diag = diag::warn_unreachable_return;
           break;

cfe/trunk/test/Sema/warn-unreachable.c

 // RUN: %clang_cc1 %s -fsyntax-only -verify -fblocks -Wunreachable-code-aggressive -Wno-unused-value -Wno-covered-switch-default -I %S/Inputs
+// RUN: %clang_cc1 -fsyntax-only -fblocks -Wunreachable-code-aggressive -Wno-unused-value -Wno-covered-switch-default -fdiagnostics-parseable-fixits -I %S/Inputs %s 2>&1 | FileCheck %s
 
 #include "warn-unreachable.h"
 
 int halt() __attribute__((noreturn));
 int live();
 int dead();
 
 void test1() {
void test_with_paren_silencing(int x) {
   else
     calledFun();
   
   if (!(1))
     calledFun(); // no-warning
   else
     calledFun();
 }
+
+// rdar://24570531
+
+struct StructWithPointer {
+  void *p;
+};
+
+void emitJustOneWarningForOr(struct StructWithPointer *s) {
+  if (1 || !s->p) // expected-note {{silence by adding parentheses to mark code as explicitly dead}}
+    return; // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:7-[[@LINE-1]]:7}:"/* DISABLES CODE */ ("
+            // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:8-[[@LINE-2]]:8}:")"
+  emitJustOneWarningForOr(s); // expected-warning {{code will never be executed}}
+}
+
+void emitJustOneWarningForOrSilenced(struct StructWithPointer *s) {
+  if ((1) || !s->p)
+    return;
+
+  emitJustOneWarningForOrSilenced(s); // no warning
+}
+
+void emitJustOneWarningForOr2(struct StructWithPointer *s) {
+  if (1 || !s->p) // expected-warning {{code will never be executed}}
+    return; // expected-note@-1 {{silence by adding parentheses to mark code as explicitly dead}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:7-[[@LINE-2]]:7}:"/* DISABLES CODE */ ("
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:8-[[@LINE-3]]:8}:")"
+}
+
+void wrapOneInFixit(struct StructWithPointer *s) {
+  if (!s->p || 1) // expected-note {{silence by adding parentheses to mark code as explicitly dead}}
+    return; // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:16-[[@LINE-1]]:16}:"/* DISABLES CODE */ ("
+            // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:17-[[@LINE-2]]:17}:")"
+  wrapOneInFixit(s); // expected-warning {{code will never be executed}}
+}
+
+void unaryOpNoFixit() {
+  if (- 1)
+    return; // CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]
+  unaryOpNoFixit(); // expected-warning {{code will never be executed}}
+}
+
+void unaryOpStrictFixit(struct StructWithPointer *s) {
+  if (!(s->p && 0)) // expected-note {{silence by adding parentheses to mark code as explicitly dead}}
+    return; // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:17-[[@LINE-1]]:17}:"/* DISABLES CODE */ ("
+            // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:18-[[@LINE-2]]:18}:")"
+  unaryOpStrictFixit(s); // expected-warning {{code will never be executed}}
+}
+
+void unaryOpFixitCastSubExpr(int x) {
+  if (! (int)0) // expected-note {{silence by adding parentheses to mark code as explicitly dead}}
+    return; // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:7-[[@LINE-1]]:7}:"/* DISABLES CODE */ ("
+            // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:15-[[@LINE-2]]:15}:")"
+  unaryOpFixitCastSubExpr(x); // expected-warning {{code will never be executed}}
+}