[SE-0408] Pack iteration review feedback!

include/swift/AST/Decl.h

@@ -5857,6 +5857,9 @@ enum class PropertyWrapperSynthesizedPropertyKind {
 class VarDecl : public AbstractStorageDecl {
   friend class NamingPatternRequest;
   NamedPattern *NamingPattern = nullptr;
+  /// When the variable is declared in context of a for-in loop over the elements of 
+  /// a parameter pack, this is the opened element environment of the pack expansion
+  /// to use as the variable's context generic environment.
   GenericEnvironment *OpenedElementEnvironment = nullptr;
 
 public:

include/swift/Sema/ConstraintSystem.h

@@ -3765,10 +3765,20 @@ class ConstraintSystem {
                                        RememberChoice_t rememberChoice,
                                        ConstraintLocatorBuilder locator,
                                        ConstraintFix *compatFix = nullptr);
-
-  /// Add a materialize constraint for a pack expansion.
+
+  /// Given a tuple with a single unlabeled element that represents a pack
+  /// expansion (either directly via \c PackExpansionType or through a type
+  /// variable constrained to a pack expansion), materialize the pack expansion
+  /// and return its pattern type as a result. The result is a type variable
+  /// because element of the tuple is not required to be resolved at the time of
+  /// the call and operation is delayed until the element is sufficiently
+  /// resolved (see \c simplifyMaterializePackExpansionConstraint)
+  ///
+  /// \param tupleType A tuple with a single unlabeled element that represents a
+  /// pack expansion. 
+  /// \param locator The locator.
   TypeVariableType *
-  addMaterializePackExpansionConstraint(Type patternType,
+  addMaterializePackExpansionConstraint(Type tupleType,
                                         ConstraintLocatorBuilder locator);
 
   /// Add a disjunction constraint.

include/swift/Sema/SyntacticElementTarget.h

@@ -29,8 +29,8 @@
 namespace swift {
 
 namespace constraints {
-/// Describes information specific to a sequence
-/// in a for-each loop.
+/// Describes information about a for-in loop over a sequence that needs to be
+/// tracked in the constraint system.
 struct SequenceIterationInfo {
   /// The type of the sequence.
   Type sequenceType;
@@ -48,18 +48,16 @@ struct SequenceIterationInfo {
   Expr *nextCall;
 };
 
-/// Describes information specific to a pack expansion expression
-/// in a for-each loop.
+/// Describes information about a for-in loop over a pack that needs to be
+/// tracked in the constraint system.
 struct PackIterationInfo {
   /// The type of the pattern that matches the elements.
   Type patternType;
 };
 
-/// Describes information about a for-each loop that needs to be tracked
+/// Describes information about a for-in loop that needs to be tracked
 /// within the constraint system.
-struct ForEachStmtInfo : TaggedUnion<SequenceIterationInfo, PackIterationInfo> {
-  using TaggedUnion::TaggedUnion;
-};
+using ForEachStmtInfo = TaggedUnion<SequenceIterationInfo, PackIterationInfo>;
 
 /// Describes the target to which a constraint system's solution can be
 /// applied.

lib/AST/ASTVerifier.cpp

@@ -2645,7 +2645,7 @@ class Verifier : public ASTWalker {
       }
 
       // Catch cases where there's a missing generic environment.
-      if (var->getTypeInContext()->is<ErrorType>()) {
+      if (var->getTypeInContext()->hasError()) {
         Out << "VarDecl is missing a Generic Environment: ";
         var->getInterfaceType().print(Out);
         Out << "\n";

lib/AST/Decl.cpp

@@ -7149,8 +7149,9 @@ VarDecl::VarDecl(DeclKind kind, bool isStatic, VarDecl::Introducer introducer,
 }
 
 Type VarDecl::getTypeInContext() const {
-  // If we are performing pack iteration, use the generic environment of the
-  // pack expansion expression to get the right context of a local variable.
+  // If the variable is declared in context of a for-in loop over the elements
+  // of a parameter pack, its interface type must be mapped into context using
+  // the opened element environment of the pack expansion.
   if (auto *env = getOpenedElementEnvironment())
     return GenericEnvironment::mapTypeIntoContext(env, getInterfaceType());
 

lib/SILGen/SILGenStmt.cpp

@@ -1246,8 +1246,6 @@ void StmtEmitter::visitRepeatWhileStmt(RepeatWhileStmt *S) {
 }
 
 void StmtEmitter::visitForEachStmt(ForEachStmt *S) {
-  // Emit the 'iterator' variable that we'll be using for iteration.
-  LexicalScope OuterForScope(SGF, CleanupLocation(S));
 
   if (auto *expansion =
           dyn_cast<PackExpansionExpr>(S->getTypeCheckedSequence())) {
@@ -1256,8 +1254,6 @@ void StmtEmitter::visitForEachStmt(ForEachStmt *S) {
             ->getCanonicalType());
 
     JumpDest loopDest = createJumpDest(S->getBody());
-
-    // Set the destinations for 'break' and 'continue'.
     JumpDest endDest = createJumpDest(S->getBody());
 
     SGF.emitDynamicPackLoop(
@@ -1271,6 +1267,7 @@ void StmtEmitter::visitForEachStmt(ForEachStmt *S) {
 
           SGF.emitExprInto(expansion->getPatternExpr(), letValueInit.get());
 
+          // Set the destinations for 'break' and 'continue'.
           SGF.BreakContinueDestStack.push_back({S, endDest, loopDest});
           visit(S->getBody());
           SGF.BreakContinueDestStack.pop_back();
@@ -1284,6 +1281,8 @@ void StmtEmitter::visitForEachStmt(ForEachStmt *S) {
     return;
   }
 
+  // Emit the 'iterator' variable that we'll be using for iteration.
+  LexicalScope OuterForScope(SGF, CleanupLocation(S));
   SGF.emitPatternBinding(S->getIteratorVar(),
                          /*index=*/0, /*debuginfo*/ true);
 

lib/Sema/CSGen.cpp

@@ -4447,8 +4447,8 @@ static bool generateInitPatternConstraints(ConstraintSystem &cs,
   return false;
 }
 
-/// Generate constraints for a for-in statement preamble, expecting a
-/// `PackExpansionExpr`.
+/// Generate constraints for a for-in statement preamble where the expression
+/// is a `PackExpansionExpr`.
 static llvm::Optional<PackIterationInfo>
 generateForEachStmtConstraints(ConstraintSystem &cs, DeclContext *dc,
                                PackExpansionExpr *expansion, Type patternType) {
@@ -4524,6 +4524,7 @@ generateForEachStmtConstraints(ConstraintSystem &cs, DeclContext *dc,
 
     auto *makeIteratorCall =
         CallExpr::createImplicitEmpty(ctx, makeIteratorRef);
+
     Pattern *pattern = NamedPattern::createImplicit(ctx, makeIteratorVar);
     auto *PB = PatternBindingDecl::createImplicit(
         ctx, StaticSpellingKind::None, pattern, makeIteratorCall, dc);
@@ -4540,6 +4541,7 @@ generateForEachStmtConstraints(ConstraintSystem &cs, DeclContext *dc,
       return llvm::None;
 
     sequenceIterationInfo.makeIteratorVar = PB;
+
     // Type of sequence expression has to conform to Sequence protocol.
     //
     // Note that the following emulates having `$generator` separately
@@ -4606,7 +4608,7 @@ generateForEachStmtConstraints(ConstraintSystem &cs, DeclContext *dc,
     cs.setTargetFor(sequenceIterationInfo.nextCall, nextTarget);
   }
 
-  // Generate constraints for the pattern
+  // Generate constraints for the pattern.
   Type initType =
       cs.generateConstraints(typeCheckedPattern, elementLocator,
                              shouldBindPatternVarsOneWay, nullptr, 0);
@@ -4677,7 +4679,7 @@ generateForEachStmtConstraints(ConstraintSystem &cs,
   if (isa<PackExpansionExpr>(forEachExpr)) {
     auto *expansion = cast<PackExpansionExpr>(forEachExpr);
 
-    // Generate constraints for the pattern
+    // Generate constraints for the pattern.
     Type patternType = cs.generateConstraints(
         pattern, elementLocator, target.shouldBindPatternVarsOneWay(), nullptr,
         0);

lib/Sema/CSSimplify.cpp

@@ -15640,11 +15640,11 @@ void ConstraintSystem::addExplicitConversionConstraint(
 }
 
 TypeVariableType *ConstraintSystem::addMaterializePackExpansionConstraint(
-    Type patternType, ConstraintLocatorBuilder locator) {
-  assert(isSingleUnlabeledPackExpansionTuple(patternType));
+    Type tupleType, ConstraintLocatorBuilder locator) {
+  assert(isSingleUnlabeledPackExpansionTuple(tupleType));
   TypeVariableType *packVar =
       createTypeVariable(getConstraintLocator(locator), TVO_CanBindToPack);
-  addConstraint(ConstraintKind::MaterializePackExpansion, patternType, packVar,
+  addConstraint(ConstraintKind::MaterializePackExpansion, tupleType, packVar,
                 getConstraintLocator(locator, {ConstraintLocator::Member}));
   return packVar;
 }