-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

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

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

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);
+      bool SilenceableCondValNotSet =
+          SilenceableCondVal && SilenceableCondVal->getBegin().isInvalid();
+      bool IsSubExprConfigValue =
+      // Update the silenceable condition value source range only if the range
+      // was set in the child expression.
+      if (SilenceableCondValNotSet && SilenceableCondVal->getBegin().isValid())
+        *SilenceableCondVal = UO->getSourceRange();
+      return UO->getOpcode() == UO_LNot && IsSubExprConfigValue;
     }
     default:
       return false;
   }
 }
 
 static bool isConfigurationValue(const ValueDecl *D, Preprocessor &PP) {
   if (const EnumConstantDecl *ED = dyn_cast<EnumConstantDecl>(D))

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;

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 unaryOpFixit() {
+  if (- 1)  // 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]]:10-[[@LINE-2]]:10}:")"
+  unaryOpFixit(); // expected-warning {{code will never be executed}}
+}