Diagnose derivatives that use a variable before its declaration

clad assembles derivative bodies by hand, so nothing enforces that a
declaration precedes its uses the way parsing does for user code, and the
generated function is never handed to Sema::ActOnFinishFunctionBody,
which would re-check it. Such a body can therefore name a variable that
is not in scope at that point and still compile.

Add findUseBeforeDecl alongside the existing structural checks. It walks
the body in execution order carrying the block-scope locals declared so
far, which also validates lambda bodies against what is visible at the
lambda's definition point -- notably catching references built outside a
closure that never became captures, and which LambdaExpr::captures()
therefore cannot report. It is gated on a clean derivation like StrayRef:
an unsupported construct is diagnosed and its body is not expected to be
well-formed.

The check found three violations, fixed here. Cloning a lambda body
created fresh VarDecls but left later statements referring to the
original lambda's variables, so register the clones for remapping. A
range-for declared its adjoint iterator and loop variable inside the
forward loop while the reverse sweep, a sibling block, restored them --
for a nested loop no block encloses both short of the function body, so
declare them there and initialize in place.

Author: vgvassilev

lib/Differentiator/ASTIntegrity.cpp

@@ -96,10 +96,82 @@ const ValueDecl* findOriginalRef(const Stmt* Derivative,
   return F.Stray;
 }
 
+/// Visit a block, scoping the declarations it introduces to it.
+static const ValueDecl* walkScope(const Stmt* S,
+                                  llvm::DenseSet<const VarDecl*> Declared);
+
+/// Visit \p S in order, threading \p Declared through so later siblings see
+/// what earlier ones declared. Returns the first offender, or null.
+static const ValueDecl* walkStmt(const Stmt* S,
+                                 llvm::DenseSet<const VarDecl*>& Declared) {
+  if (!S)
+    return nullptr;
+
+  if (const auto* DRE = dyn_cast<DeclRefExpr>(S)) {
+    const auto* VD = dyn_cast<VarDecl>(DRE->getDecl());
+    // Only block-scope locals can be used before their declaration. Parameters
+    // and globals are live for the whole body.
+    if (VD && VD->isLocalVarDecl() && !Declared.count(VD))
+      return VD;
+    return nullptr;
+  }
+
+  if (const auto* LE = dyn_cast<LambdaExpr>(S)) {
+    llvm::DenseSet<const VarDecl*> Inner = Declared;
+    if (const CXXMethodDecl* Call = LE->getCallOperator())
+      for (const ParmVarDecl* P : Call->parameters())
+        Inner.insert(P);
+    return walkScope(LE->getBody(), std::move(Inner));
+  }
+
+  // A block's declarations do not escape it.
+  if (isa<CompoundStmt>(S))
+    return walkScope(S, Declared);
+
+  if (const auto* DS = dyn_cast<DeclStmt>(S)) {
+    // An initializer is evaluated before its own variable is in scope.
+    for (const Decl* D : DS->decls())
+      if (const auto* VD = dyn_cast<VarDecl>(D)) {
+        if (const ValueDecl* Bad = walkStmt(VD->getInit(), Declared))
+          return Bad;
+        Declared.insert(VD);
+      }
+    return nullptr;
+  }
+
+  for (const Stmt* Child : S->children())
+    if (const ValueDecl* Bad = walkStmt(Child, Declared))
+      return Bad;
+  return nullptr;
+}
+
+static const ValueDecl* walkScope(const Stmt* S,
+                                  llvm::DenseSet<const VarDecl*> Declared) {
+  if (!S)
+    return nullptr;
+  if (!isa<CompoundStmt>(S))
+    return walkStmt(S, Declared);
+  for (const Stmt* Child : cast<CompoundStmt>(S)->body())
+    if (const ValueDecl* Bad = walkStmt(Child, Declared))
+      return Bad;
+  return nullptr;
+}
+
+const ValueDecl* findUseBeforeDecl(const Stmt* Derivative,
+                                   const FunctionDecl* Derived) {
+  llvm::DenseSet<const VarDecl*> Declared;
+  if (Derived)
+    for (const ParmVarDecl* P : Derived->parameters())
+      Declared.insert(P);
+  return walkScope(Derivative, std::move(Declared));
+}
+
 IntegrityReport verifyDerivative(const Stmt* Derivative,
-                                 const FunctionDecl* Original) {
+                                 const FunctionDecl* Original,
+                                 const FunctionDecl* Derived) {
   IntegrityReport R;
   R.SharedNode = findSharedNode(Derivative);
+  R.UseBeforeDecl = findUseBeforeDecl(Derivative, Derived);
   if (Original) {
     if (const Stmt* PrimalBody = Original->getBody())
       R.PrimalNode = findPrimalSharedNode(Derivative, PrimalBody);

lib/Differentiator/ASTIntegrity.h

@@ -50,6 +50,16 @@ const clang::Stmt* findPrimalSharedNode(const clang::Stmt* Derivative,
 const clang::ValueDecl* findOriginalRef(const clang::Stmt* Derivative,
                                         const clang::FunctionDecl* Original);
 
+/// Return the first block-scope variable \p Derivative references before its
+/// declaration, or nullptr if every use is preceded by its decl. clad builds
+/// bodies by hand, so nothing enforces the ordering that parsing guarantees for
+/// user code. Also validates lambda bodies against what is visible at the
+/// lambda's definition point, which catches references built outside the
+/// closure's scope that never became captures. \p Derived supplies the
+/// generated function whose parameters are in scope throughout.
+const clang::ValueDecl* findUseBeforeDecl(const clang::Stmt* Derivative,
+                                          const clang::FunctionDecl* Derived);
+
 /// The structural violations a generated derivative body may exhibit, each the
 /// first offending node/decl or null. Purely a function of the AST -- the
 /// caller supplies the differentiation context (whether the derivation was
@@ -62,6 +72,8 @@ struct IntegrityReport {
   /// A reference left bound to one of the original function's own
   /// params/locals.
   const clang::ValueDecl* StrayRef = nullptr;
+  /// A variable referenced before its declaration.
+  const clang::ValueDecl* UseBeforeDecl = nullptr;
 };
 
 /// Run every structural integrity check on a generated \p Derivative body.
@@ -70,7 +82,8 @@ struct IntegrityReport {
 /// the AST; StrayRef in particular is meaningful only for a clean derivation,
 /// which the caller must establish before acting on it.
 IntegrityReport verifyDerivative(const clang::Stmt* Derivative,
-                                 const clang::FunctionDecl* Original);
+                                 const clang::FunctionDecl* Original,
+                                 const clang::FunctionDecl* Derived);
 
 } // namespace clad
 

lib/Differentiator/DerivativeBuilder.cpp

@@ -666,7 +666,7 @@ static void registerDerivative(Decl* D, Sema& S, const DiffRequest& R) {
         // Compute cleanliness before the diagnostics below inflate the tally.
         bool CleanDerivation =
             Diags.getNumWarnings() + Diags.getNumErrors() == DiagsBefore;
-        IntegrityReport Report = verifyDerivative(Body, request.Function);
+        IntegrityReport Report = verifyDerivative(Body, request.Function, FD);
 
         // A derivative must be a proper tree in its Stmt child-edge structure:
         // no node the child of two parents, because a later in-place edit of a
@@ -701,6 +701,21 @@ static void registerDerivative(Decl* D, Sema& S, const DiffRequest& R) {
         // unsupported construct is cloned wholesale and knowingly keeps such
         // references in a derivative that is not used.
         if (CleanDerivation) {
+          // Nothing enforces declaration-before-use in a hand-assembled body
+          // the way parsing does for user code, so a reference can name a
+          // variable whose declaration has not been reached -- or one owned by
+          // the original function. Gated with StrayRef: an unsupported
+          // construct is diagnosed and its body is not expected to be
+          // well-formed.
+          assert(!Report.UseBeforeDecl &&
+                 "clad generated a use of a variable before its declaration");
+          if (Report.UseBeforeDecl)
+            diag(DiagnosticsEngine::Warning, FD->getLocation(),
+                 "clad referenced '%0' before its declaration while "
+                 "differentiating %1; this is a clad bug -- please report it "
+                 "at %2")
+                << Report.UseBeforeDecl << FD << getCladRepositoryURL();
+
           assert(!Report.StrayRef &&
                  "derivative references an un-remapped decl of the original");
           if (Report.StrayRef)

lib/Differentiator/ReverseModeVisitor.cpp

@@ -1078,8 +1078,18 @@ Expr* ReverseModeVisitor::getStdInitListSizeExpr(const Expr* E) {
     if (VisitRange.getDecl_dx())
       addToCurrentBlock(BuildDeclStmt(VisitRange.getDecl_dx()));
     addToCurrentBlock(BuildDeclStmt(VisitBegin.getDecl()));
-    if (VisitBegin.getDecl_dx())
-      addToCurrentBlock(BuildDeclStmt(VisitBegin.getDecl_dx()));
+    // The reverse sweep decrements the adjoint iterator, but its loop is a
+    // sibling of the forward one -- for a nested loop no block encloses both
+    // short of the function body. Hoist the declaration to function scope so
+    // both sweeps name the same object, and keep the initialization here,
+    // where the adjoint range it reads is in scope.
+    if (VarDecl* DBegin = VisitBegin.getDecl_dx()) {
+      Expr* Init = DBegin->getInit();
+      DBegin->setInit(nullptr);
+      addToBlock(BuildDeclStmt(DBegin), m_Globals);
+      if (Init)
+        addToCurrentBlock(BuildOp(BO_Assign, BuildDeclRef(DBegin), Init));
+    }
 
     const auto* EndDecl = cast<VarDecl>(FRS->getEndStmt()->getSingleDecl());
     QualType endType = CloneType(EndDecl->getType());
@@ -1119,13 +1129,17 @@ Expr* ReverseModeVisitor::getStdInitListSizeExpr(const Expr* E) {
     StmtDiff storeAdjLoop;
     if (LoopVDDiff.getDecl_dx())
       storeAdjLoop = StoreAndRestore(BuildDeclRef(LoopVDDiff.getDecl_dx()));
+    // The reverse sweep restores the loop variable and its adjoint from the
+    // tape, but it does so from a sibling block of the forward loop. Declare
+    // both at function scope so the restore names the same object; each is
+    // zero-initialized, so nothing in the initializer needs the loop's scope.
     if (LoopVDDiff.getDecl_dx())
-      addToCurrentBlock(BuildDeclStmt(LoopVDDiff.getDecl_dx()));
+      addToBlock(BuildDeclStmt(LoopVDDiff.getDecl_dx()), m_Globals);
     Expr* loopInit = LoopVDDiff.getDecl()->getInit();
     SetDeclInit(LoopVDDiff.getDecl(),
                 getZeroInit(LoopVDDiff.getDecl()->getType()));
     if (LoopVDDiff.getDecl())
-      addToCurrentBlock(BuildDeclStmt(LoopVDDiff.getDecl()));
+      addToBlock(BuildDeclStmt(LoopVDDiff.getDecl()), m_Globals);
     Expr* assignLoop =
         BuildOp(BO_Assign, BuildDeclRef(LoopVDDiff.getDecl()), loopInit);
 

lib/Differentiator/VisitorBase.cpp

@@ -1029,6 +1029,20 @@ namespace clad {
       utils::ReferencesUpdater up(m_Sema, getCurrentScope(), CallOp,
                                   m_DeclReplacements);
       up.TraverseStmt(clonedS);
+      // Cloning a DeclStmt produces a fresh VarDecl, but StmtClone records that
+      // only in its own decl mapping. Register it here too, or a later
+      // statement's clone keeps referring to the original lambda's variable --
+      // a reference into the user's AST that outlives differentiation.
+      if (const auto* DS = dyn_cast<DeclStmt>(S))
+        if (auto* ClonedDS = dyn_cast<DeclStmt>(clonedS)) {
+          auto O = DS->decl_begin();
+          auto C = ClonedDS->decl_begin();
+          for (; O != DS->decl_end() && C != ClonedDS->decl_end(); ++O, ++C)
+            if (const auto* OVD = dyn_cast<VarDecl>(*O))
+              if (auto* CVD = dyn_cast<VarDecl>(*C))
+                if (OVD != CVD)
+                  m_DeclReplacements[OVD] = CVD;
+        }
       addToCurrentBlock(clonedS);
     }
     CompoundStmt* ClonedBody = endBlock();