[ast] Do not auto-initialize Objective-C for-loop variables in Objective-C++ in templatized code under ARC

[george.karpenkov]

Summary

I was very puzzled to learn that AST for the fragment

@interface I
@end

template <typename>
void decode(I *p) {
  for (I *k in p) {}
}

void decode(I *p) {
  decode<int>(p);
}

differs heavily when templatized and non-templatized:

|-FunctionTemplateDecl 0x7fbfe0863940 <line:4:1, line:7:1> line:5:6 decode
| |-TemplateTypeParmDecl 0x7fbfe0863690 <line:4:11> col:11 typename depth 0 index 0
| |-FunctionDecl 0x7fbfe08638a0 <line:5:1, line:7:1> line:5:6 decode 'void (I *__strong)'
| | |-ParmVarDecl 0x7fbfe08637a0 <col:13, col:16> col:16 referenced p 'I *__strong'
| | `-CompoundStmt 0x7fbfe0863b88 <col:19, line:7:1>
| |   `-ObjCForCollectionStmt 0x7fbfe0863b50 <line:6:3, col:20>
| |     |-DeclStmt 0x7fbfe0863a50 <col:8, col:13>
| |     | `-VarDecl 0x7fbfe08639f0 <col:8, col:11> col:11 k 'I *const __strong'
| |     |-ImplicitCastExpr 0x7fbfe0863a90 <col:16> 'I *' <LValueToRValue>
| |     | `-DeclRefExpr 0x7fbfe0863a68 <col:16> 'I *__strong' lvalue ParmVar 0x7fbfe08637a0 'p' 'I *__strong'
| |     `-CompoundStmt 0x7fbfe0863b78 <col:19, col:20>
| `-FunctionDecl 0x7fbfe0863f80 <line:5:1, line:7:1> line:5:6 used decode 'void (I *__strong)'
|   |-TemplateArgument type 'int'
|   |-ParmVarDecl 0x7fbfe0863ef8 <col:13, col:16> col:16 used p 'I *__strong'
|   `-CompoundStmt 0x7fbfe0890cf0 <col:19, line:7:1>
|     `-ObjCForCollectionStmt 0x7fbfe0890cc8 <line:6:3, col:20>
|       |-DeclStmt 0x7fbfe0890c70 <col:8, col:13>
|       | `-VarDecl 0x7fbfe0890c00 <col:8, col:11> col:11 k 'I *__strong' callinit
|       |   `-ImplicitValueInitExpr 0x7fbfe0890c60 <<invalid sloc>> 'I *__strong'
|       |-ImplicitCastExpr 0x7fbfe0890cb0 <col:16> 'I *' <LValueToRValue>
|       | `-DeclRefExpr 0x7fbfe0890c88 <col:16> 'I *__strong' lvalue ParmVar 0x7fbfe0863ef8 'p' 'I *__strong'
|       `-CompoundStmt 0x7fbfe0863b78 <col:19, col:20>

Note how in the instantiated version ImplicitValueInitExpr unexpectedly appears.

While objects are auto-initialized under ARC, it does not make sense to have an initializer for a for-loop variable, and it makes even less sense to have such a different AST for instantiated and non-instantiated version.

Digging deeper, I have found that there are two separate Sema* files for dealing with templates and for dealing with non-templatized code. In a non-templatized version, an initialization was performed only for variables which are not loop variables for an Objective-C loop and not variables for a C++ for-in loop:

  if (FRI && (Tok.is(tok::colon) || isTokIdentifier_in())) {
    bool IsForRangeLoop = false;
    if (TryConsumeToken(tok::colon, FRI->ColonLoc)) {
      IsForRangeLoop = true;
      if (Tok.is(tok::l_brace))
        FRI->RangeExpr = ParseBraceInitializer();
      else
        FRI->RangeExpr = ParseExpression();
    }

    Decl *ThisDecl = Actions.ActOnDeclarator(getCurScope(), D);
    if (IsForRangeLoop)
      Actions.ActOnCXXForRangeDecl(ThisDecl);
    Actions.FinalizeDeclaration(ThisDecl);
    D.complete(ThisDecl);
    return Actions.FinalizeDeclaratorGroup(getCurScope(), DS, ThisDecl);
  }

  SmallVector<Decl *, 8> DeclsInGroup;
  Decl *FirstDecl = ParseDeclarationAfterDeclaratorAndAttributes(
      D, ParsedTemplateInfo(), FRI);

However the code in SemaTemplateInstantiateDecl was inconsistent, guarding only against C++ for-in loops.

rdar://38391075

[rjmccall]

LGTM. I guess it just didn't matter for Objective-C because the variable always has to be an Objective-C pointer and the extra store of nil never hurt anything.

include/clang/AST/Decl.h

class NonParmVarDeclBitfields {
     /// slot of its function, enabling the named return value optimization
     /// (NRVO).
     unsigned NRVOVariable : 1;
 
     /// Whether this variable is the for-range-declaration in a C++0x
     /// for-range statement.
     unsigned CXXForRangeDecl : 1;
 
+    /// Whether this variable is the for-in loop declaration in Objective-C.
+    unsigned ObjCForDecl : 1;
+
     /// Whether this variable is an ARC pseudo-__strong
     /// variable;  see isARCPseudoStrong() for details.
     unsigned ARCPseudoStrong : 1;
 
     /// Whether this variable is (C++1z) inline.
     unsigned IsInline : 1;
 
     /// Whether this variable has (C++1z) inline explicitly specified.
public:
   bool isCXXForRangeDecl() const {
     return isa<ParmVarDecl>(this) ? false : NonParmVarDeclBits.CXXForRangeDecl;
   }
   void setCXXForRangeDecl(bool FRD) {
     assert(!isa<ParmVarDecl>(this));
     NonParmVarDeclBits.CXXForRangeDecl = FRD;
   }
 
+  /// \brief Determine whether this variable is a for-loop declaration for a
+  /// for-in statement in Objective-C.
+  bool isObjCForDecl() const {
+    return NonParmVarDeclBits.ObjCForDecl;
+  }
+
+  void setObjCForDecl(bool FRD) {
+    NonParmVarDeclBits.ObjCForDecl = FRD;
+  }
+
   /// \brief Determine whether this variable is an ARC pseudo-__strong
   /// variable.  A pseudo-__strong variable has a __strong-qualified
   /// type but does not actually retain the object written into it.
   /// Generally such variables are also 'const' for safety.
   bool isARCPseudoStrong() const {
     return isa<ParmVarDecl>(this) ? false : NonParmVarDeclBits.ARCPseudoStrong;
   }
   void setARCPseudoStrong(bool ps) {

lib/Parse/ParseDecl.cpp

       IsForRangeLoop = true;
       if (Tok.is(tok::l_brace))
         FRI->RangeExpr = ParseBraceInitializer();
       else
         FRI->RangeExpr = ParseExpression();
     }
 
     Decl *ThisDecl = Actions.ActOnDeclarator(getCurScope(), D);
-    if (IsForRangeLoop)
+    if (IsForRangeLoop) {
       Actions.ActOnCXXForRangeDecl(ThisDecl);
+    } else {
+      // Obj-C for loop
+      if (auto *VD = dyn_cast_or_null<VarDecl>(ThisDecl))
+        VD->setObjCForDecl(true);
+    }
     Actions.FinalizeDeclaration(ThisDecl);
     D.complete(ThisDecl);
     return Actions.FinalizeDeclaratorGroup(getCurScope(), DS, ThisDecl);
   }
 
   SmallVector<Decl *, 8> DeclsInGroup;
   Decl *FirstDecl = ParseDeclarationAfterDeclaratorAndAttributes(
       D, ParsedTemplateInfo(), FRI);

lib/Sema/SemaTemplateInstantiateDecl.cpp

   if (OldVar->isLocalExternDecl()) {
     NewVar->setLocalExternDecl();
     NewVar->setLexicalDeclContext(Owner);
   } else if (OldVar->isOutOfLine())
     NewVar->setLexicalDeclContext(OldVar->getLexicalDeclContext());
   NewVar->setTSCSpec(OldVar->getTSCSpec());
   NewVar->setInitStyle(OldVar->getInitStyle());
   NewVar->setCXXForRangeDecl(OldVar->isCXXForRangeDecl());
+  NewVar->setObjCForDecl(OldVar->isObjCForDecl());
   NewVar->setConstexpr(OldVar->isConstexpr());
   NewVar->setInitCapture(OldVar->isInitCapture());
   NewVar->setPreviousDeclInSameBlockScope(
       OldVar->isPreviousDeclInSameBlockScope());
   NewVar->setAccess(OldVar->getAccess());
 
   if (!OldVar->isStaticDataMember()) {
     if (OldVar->isUsed(false))

 
       // If the declaration inside the class had an initializer, don't add
       // another one to the out-of-line definition.
       if (OldVar->getFirstDecl()->hasInit())
         return;
     }
 
     // We'll add an initializer to a for-range declaration later.
-    if (Var->isCXXForRangeDecl())
+    if (Var->isCXXForRangeDecl() || Var->isObjCForDecl())
       return;
 
     ActOnUninitializedDecl(Var);
   }
 }
 
 /// \brief Instantiate the definition of the given variable from its
 /// template.

lib/Serialization/ASTReaderDecl.cpp

   VD->VarDeclBits.TSCSpec = Record.readInt();
   VD->VarDeclBits.InitStyle = Record.readInt();
   if (!isa<ParmVarDecl>(VD)) {
     VD->NonParmVarDeclBits.IsThisDeclarationADemotedDefinition =
         Record.readInt();
     VD->NonParmVarDeclBits.ExceptionVar = Record.readInt();
     VD->NonParmVarDeclBits.NRVOVariable = Record.readInt();
     VD->NonParmVarDeclBits.CXXForRangeDecl = Record.readInt();
+    VD->NonParmVarDeclBits.ObjCForDecl = Record.readInt();
     VD->NonParmVarDeclBits.ARCPseudoStrong = Record.readInt();
     VD->NonParmVarDeclBits.IsInline = Record.readInt();
     VD->NonParmVarDeclBits.IsInlineSpecified = Record.readInt();
     VD->NonParmVarDeclBits.IsConstexpr = Record.readInt();
     VD->NonParmVarDeclBits.IsInitCapture = Record.readInt();
     VD->NonParmVarDeclBits.PreviousDeclInSameBlockScope = Record.readInt();
     VD->NonParmVarDeclBits.ImplicitParamKind = Record.readInt();
   }

lib/Serialization/ASTWriterDecl.cpp

void ASTDeclWriter::VisitVarDecl(VarDecl *D) {
   Record.push_back(D->getStorageClass());
   Record.push_back(D->getTSCSpec());
   Record.push_back(D->getInitStyle());
   if (!isa<ParmVarDecl>(D)) {
     Record.push_back(D->isThisDeclarationADemotedDefinition());
     Record.push_back(D->isExceptionVariable());
     Record.push_back(D->isNRVOVariable());
     Record.push_back(D->isCXXForRangeDecl());
+    Record.push_back(D->isObjCForDecl());
     Record.push_back(D->isARCPseudoStrong());
     Record.push_back(D->isInline());
     Record.push_back(D->isInlineSpecified());
     Record.push_back(D->isConstexpr());
     Record.push_back(D->isInitCapture());
     Record.push_back(D->isPreviousDeclInSameBlockScope());
     if (const auto *IPD = dyn_cast<ImplicitParamDecl>(D))
       Record.push_back(static_cast<unsigned>(IPD->getParameterKind()));
void ASTWriter::WriteDeclAbbrevs() {
   // VarDecl
   Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 3)); // SClass
   Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 2)); // TSCSpec
   Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 2)); // InitStyle
   Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 1)); // IsThisDeclarationADemotedDefinition
   Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 1)); // isExceptionVariable
   Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 1)); // isNRVOVariable
   Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 1)); // isCXXForRangeDecl
+  Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 1)); // isObjCForDecl
   Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 1)); // isARCPseudoStrong
   Abv->Add(BitCodeAbbrevOp(0));                         // isInline
   Abv->Add(BitCodeAbbrevOp(0));                         // isInlineSpecified
   Abv->Add(BitCodeAbbrevOp(0));                         // isConstexpr
   Abv->Add(BitCodeAbbrevOp(0));                         // isInitCapture
   Abv->Add(BitCodeAbbrevOp(0));                         // isPrevDeclInSameScope
   Abv->Add(BitCodeAbbrevOp(0));                         // ImplicitParamKind
   Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 3)); // Linkage

test/SemaObjC/foreachtemplatized.mm

+// RUN: %clang_cc1 -fsyntax-only -fobjc-arc -Wno-objc-root-class -std=c++11 -ast-dump %s | FileCheck %s
+
+// CHECK-NOT: ImplicitValueInitExpr
+
+@interface I
+@end
+
+template <typename T>
+void decode(I *p) {
+  for (I *k in p) {}
+}
+
+void decode(I *p) {
+  decode<int>(p);
+}