Fix false positives for for-loop-analysis warning

[steven_wu]

Summary

For PseudoObjectExpr, the DeclMatcher need to search only all the semantics
but also need to search pass OpaqueValueExpr for all potential uses for the Decl.

[rtrieu]

This seems rather strange. Usually, a OpaqueValueExpr will hold an expression that is held elsewhere in the AST. However, in this case, it appears that all the references to --i are all behind OpaqueValueExpr's, so they aren't processed.

`-CStyleCastExpr 0x5675d30 <line:11:5, col:19> 'void' <ToVoid>
  `-PseudoObjectExpr 0x5675d00 <col:11, col:19> 'id':'id'
    |-ObjCSubscriptRefExpr 0x5675c90 <col:11, col:19> '<pseudo-object type>' lvalue Kind=ArraySubscript GetterForArray="(null)" SetterForArray="(null)
    | |-OpaqueValueExpr 0x5675c50 <col:11> 'MyArray *'
    | | `-ImplicitCastExpr 0x5675bf0 <col:11> 'MyArray *' <LValueToRValue>
    | |   `-DeclRefExpr 0x5675b80 <col:11> 'MyArray *' lvalue Var 0x56756d0 'test' 'MyArray *'
    | `-OpaqueValueExpr 0x5675c70 <col:16, col:18> 'unsigned int'
    |   `-UnaryOperator 0x5675bd0 <col:16, col:18> 'unsigned int' prefix '--'
    |     `-DeclRefExpr 0x5675ba8 <col:18> 'unsigned int' lvalue Var 0x56759e0 'i' 'unsigned int'
    |-OpaqueValueExpr 0x5675c50 <col:11> 'MyArray *'
    | `-ImplicitCastExpr 0x5675bf0 <col:11> 'MyArray *' <LValueToRValue>
    |   `-DeclRefExpr 0x5675b80 <co        `-CStyleCastExpr 0x5675d30 <line:11:5, col:19> 'void' <ToVoid>
  `-PseudoObjectExpr 0x5675d00 <col:11, col:19> 'id':'id'
    |-ObjCSubscriptRefExpr 0x5675c90 <col:11, col:19> '<pseudo-object type>' lvalue Kind=ArraySubscript GetterForArray="(null)" SetterForArray="(null)
    | |-OpaqueValueExpr 0x5675c50 <col:11> 'MyArray *'
    | | `-ImplicitCastExpr 0x5675bf0 <col:11> 'MyArray *' <LValueToRValue>
    | |   `-DeclRefExpr 0x5675b80 <col:11> 'MyArray *' lvalue Var 0x56756d0 'test' 'MyArray *'
    | `-OpaqueValueExpr 0x5675c70 <col:16, col:18> 'unsigned int'
    |   `-UnaryOperator 0x5675bd0 <col:16, col:18> 'unsigned int' prefix '--'
    |     `-DeclRefExpr 0x5675ba8 <col:18> 'unsigned int' lvalue Var 0x56759e0 'i' 'unsigned int'
    |-OpaqueValueExpr 0x5675c50 <col:11> 'MyArray *'
    | `-ImplicitCastExpr 0x5675bf0 <col:11> 'MyArray *' <LValueToRValue>
    |   `-DeclRefExpr 0x5675b80 <col:11> 'MyArray *' lvalue Var 0x56756d0 'test' 'MyArray *'
    |-OpaqueValueExpr 0x5675c70 <col:16, col:18> 'unsigned int'
    | `-UnaryOperator 0x5675bd0 <col:16, col:18> 'unsigned int' prefix '--'
    |   `-DeclRefExpr 0x5675ba8 <col:18> 'unsigned int' lvalue Var 0x56759e0 'i' 'unsigned int'
    `-ObjCMessageExpr 0x5675cc8 <col:11> 'id':'id' selector=objectAtIndexedSubscript:
      |-OpaqueValueExpr 0x5675c50 <col:11> 'MyArray *'
      | `-ImplicitCastExpr 0x5675bf0 <col:11> 'MyArray *' <LValueToRValue>
      |   `-DeclRefExpr 0x5675b80 <col:11> 'MyArray *' lvalue Var 0x56756d0 'test' 'MyArray *'
      `-OpaqueValueExpr 0x5675c70 <col:16, col:18> 'unsigned int'
        `-UnaryOperator 0x5675bd0 <col:16, col:18> 'unsigned int' prefix '--'
          `-DeclRefExpr 0x5675ba8 <col:18> 'unsigned int' lvalue Var 0x56759e0 'i' 'unsigned int'l:11> 'MyArray *' lvalue Var 0x56756d0 'test' 'MyArray *'
    |-OpaqueValueExpr 0x5675c70 <col:16, col:18> 'unsigned int'
    | `-UnaryOperator 0x5675bd0 <col:16, col:18> 'unsigned int' prefix '--'
    |   `-DeclRefExpr 0x5675ba8 <col:18> 'unsigned int' lvalue Var 0x56759e0 'i' 'unsigned int'
    `-ObjCMessageExpr 0x5675cc8 <col:11> 'id':'id' selector=objectAtIndexedSubscript:
      |-OpaqueValueExpr 0x5675c50 <col:11> 'MyArray *'
      | `-ImplicitCastExpr 0x5675bf0 <col:11> 'MyArray *' <LValueToRValue>
      |   `-DeclRefExpr 0x5675b80 <col:11> 'MyArray *' lvalue Var 0x56756d0 'test' 'MyArray *'
      `-OpaqueValueExpr 0x5675c70 <col:16, col:18> 'unsigned int'
        `-UnaryOperator 0x5675bd0 <col:16, col:18> 'unsigned int' prefix '--'
          `-DeclRefExpr 0x5675ba8 <col:18> 'unsigned int' lvalue Var 0x56759e0 'i' 'unsigned int'

Typically, we skip OpaqueValueExpr so that we don't process the same expression multiple times. I don't know enough about Objective C to know whether this is the intended pattern or an oversight. Instead of visiting all the sub-expressions of OpaqueValueExpr's, try to make a tighter visitor to avoid repeating work. I think checking the sub-expressions of ObjCSubscriptRefExpr, and looking through the OpaqueValueExpr's there would be the best way for this.

[steven_wu]

Looking through the subscript sounds fine but I would like to know if this is indeed the only case that is being ignored because of OpaqueValueExpr and if everything is behind OpaqueValueExpr is a bug in building AST.

John, Doug, any opinion on this?

[steven_wu]

I did some more digging of the issue. It seems ObjCSubscriptRefExpr always
rebuilds and hinds both base and key behind OpaqueValueExpr. See
ObjCSubscriptOpBuilder::rebuildAndCaptureObject so it looks the AST generated
is intentional. If that is the case, Richard's suggestion is the best fix I
can think about. Update the patch to reflect the suggestion.

[steven_wu]

ping

[rjmccall]

The right fix is probably at the PseudoObjectExpr level; you should probably be looking at the semantic expressions instead of the syntactic.

[steven_wu]

Update the patch to address John's feedback. We shouldn't even checking
ObjCSubscript but looking at the Semantics for PseudoObjectExpr only.

[rjmccall]

lib/Sema/SemaStmt.cpp

1448 ↗

(On Diff #50186)

"Look past the OVE into the expression it binds."

1449 ↗

(On Diff #50186)

I believe this is never null.

[steven_wu]

Update according to feedback. I agree that OVE should never be null as semantics
of PseudoObjectExpr.

[rjmccall]

Thanks, LGTM.

cfe/trunk/lib/Sema/SemaStmt.cpp

public:
     }
 
     void VisitDeclRefExpr(DeclRefExpr *E) {
       if (VarDecl *VD = dyn_cast<VarDecl>(E->getDecl()))
         if (Decls.count(VD))
           FoundDecl = true;
     }
 
+    void VisitPseudoObjectExpr(PseudoObjectExpr *POE) {
+      // Only need to visit the semantics for POE.
+      // SyntaticForm doesn't really use the Decal.
+      for (auto *S : POE->semantics()) {
+        if (auto *OVE = dyn_cast<OpaqueValueExpr>(S))
+          // Look past the OVE into the expression it binds.
+          Visit(OVE->getSourceExpr());
+        else
+          Visit(S);
+      }
+    }
+
     bool FoundDeclInUse() { return FoundDecl; }
 
   };  // end class DeclMatcher
 
   void CheckForLoopConditionalStatement(Sema &S, Expr *Second,
                                         Expr *Third, Stmt *Body) {
     // Condition is empty
     if (!Second) return;

cfe/trunk/test/SemaObjC/warn-loop-analysis.m

+// RUN: %clang_cc1 -fsyntax-only -Wloop-analysis -verify %s
+// expected-no-diagnostics
+
+@interface MyArray
+- (id)objectAtIndexedSubscript:(unsigned int)idx;
+@end
+
+// Do not warn on objc classes has objectAtIndexedSubscript method.
+MyArray *test;
+void foo()
+{
+  unsigned int i;
+  for (i = 42; i > 0;) // No warnings here
+    (void)test[--i];
+}