Require return type for iterators with no reachable yields (chapel-la…

…ng#24450)

Resolves chapel-lang#21265.

This PR makes it so that iterators with no reachable yield statements
must declare their return type. It also makes it an error to declare an
array or list of type `nothing`. While here, improve how `nothing`
instances are cleaned up and refactor multiple helpers to fetch an
array's element type into a pair of methods on `AggregateType`.

TESTING

- [x] `linux64`, `standard`
- [x] `linux64`, `gasnet`

Reviewed by @vasslitvinov. Thanks!

compiler/AST/AggregateType.cpp

@@ -3288,3 +3288,31 @@ int64_t AggregateType::cArrayLength() const {
     USR_FATAL(symbol, "c_array must have positive size");
   return sizeInt;
 }
+
+Type* AggregateType::arrayElementType() const {
+  if (!symbol->hasFlag(FLAG_ARRAY)) return nullptr;
+  Type* ret = nullptr;
+  Type* instType = this->getField("_instance")->type;
+  AggregateType* instClass = toAggregateType(canonicalClassType(instType));
+  if (!instClass) return nullptr;
+  TypeSymbol* ts = getDataClassType(instClass->symbol);
+  // if no eltType here, go to the super class
+  while (ts == nullptr) {
+    if (Symbol* super = instClass->getSubstitutionWithName(astr("super"))) {
+        instClass = toAggregateType(canonicalClassType(super->type));
+        ts = getDataClassType(instClass->symbol);
+    } else break;
+  }
+  if (ts != nullptr) ret = ts->type;
+  return ret;
+}
+
+Type* AggregateType::finalArrayElementType() const {
+  AggregateType* arrayType = (AggregateType*) this;
+  Type* ret = nullptr;
+  do {
+    ret = arrayType->arrayElementType();
+    arrayType = toAggregateType(ret);
+  } while (arrayType && arrayType->symbol->hasFlag(FLAG_ARRAY));
+  return ret;
+}

compiler/AST/type.cpp

@@ -1843,37 +1843,6 @@ bool isAtomicType(const Type* t) {
   return t->symbol->hasFlag(FLAG_ATOMIC_TYPE);
 }
 
-// Returns the element type, given an array type.
-static Type* arrayElementType(AggregateType* arrayType) {
-  Type* eltType = nullptr;
-  INT_ASSERT(arrayType->symbol->hasFlag(FLAG_ARRAY));
-  Type* instType = arrayType->getField("_instance")->type;
-  AggregateType* instClass = toAggregateType(canonicalClassType(instType));
-  TypeSymbol* ts = getDataClassType(instClass->symbol);
-  // if no eltType here, go to the super class
-  while (ts == nullptr) {
-    if (Symbol* super = instClass->getSubstitutionWithName(astr("super"))) {
-        instClass = toAggregateType(canonicalClassType(super->type));
-        ts = getDataClassType(instClass->symbol);
-    } else break;
-  }
-  if (ts != NULL) eltType = ts->type;
-
-  return eltType;
-}
-
-// Returns the element type, given an array type.
-// Recurse into it if it is still an array.
-static Type* finalArrayElementType(AggregateType* arrayType) {
-  Type* eltType = nullptr;
-  do {
-    eltType = arrayElementType(arrayType);
-    arrayType = toAggregateType(eltType);
-  } while (arrayType != nullptr && arrayType->symbol->hasFlag(FLAG_ARRAY));
-
-  return eltType;
-}
-
 static bool isOrContains(Type *type, Flag flag, bool checkRefs = true) {
   if (type == nullptr) {
     return false;
@@ -1889,7 +1858,7 @@ static bool isOrContains(Type *type, Flag flag, bool checkRefs = true) {
     if (AggregateType* at = toAggregateType(vt)) {
       // get backing array instance and recurse
       if (at->symbol->hasFlag(FLAG_ARRAY)) {
-        Type* eltType = finalArrayElementType(at);
+        Type* eltType = at->finalArrayElementType();
         if (isOrContains(eltType, flag, checkRefs)) return true;
       } else if (at->symbol->hasFlag(FLAG_TUPLE)) {
         // if its a tuple, search the tuple type substitutions

compiler/include/AggregateType.h

@@ -177,6 +177,8 @@ class AggregateType final : public Type {
 
   Type*                       cArrayElementType()                        const;
   int64_t                     cArrayLength()                             const;
+  Type*                       arrayElementType()                         const;
+  Type*                       finalArrayElementType()                    const;
 
   //
   // Public fields

compiler/passes/initializerRules.cpp

@@ -1452,6 +1452,9 @@ static int insertPostInit(AggregateType* at, bool insertSuper) {
   bool found = false;
 
   forv_Vec(FnSymbol, method, at->methods) {
+    // Happens because we can set slots to 'nullptr' in 'cleanAst'...
+    if (method == nullptr) continue;
+
     if (method->isPostInitializer()) {
       if (method->throwsError() == true) {
         USR_FATAL_CONT(method, "postinit cannot be declared as throws yet");

compiler/resolution/cleanups.cpp

@@ -854,10 +854,16 @@ static void cleanupNothingVarsAndFields() {
             seenNothing = true;
           }
         }
-        if (seenNothing && fn->hasFlag(FLAG_AUTO_DESTROY_FN)) {
-          INT_ASSERT(call->numActuals() == 0);
+        if (seenNothing) {
           // A 0-arg call to autoDestroy would upset later passes.
-          call->remove();
+          if (fn->hasFlag(FLAG_AUTO_DESTROY_FN)) {
+            INT_ASSERT(call->numActuals() == 0);
+            call->remove();
+          } else if (fn->name == astr_initCopy &&
+                     fn->retType == dtNothing) {
+            SET_LINENO(call);
+            call->replace(new SymExpr(gNone));
+          }
         }
       }
   }
@@ -884,36 +890,40 @@ static void cleanupNothingVarsAndFields() {
       }
   }
 
-  // Set for loop index variables that are nothing to the global nothing value
+  // Set for loop index variables that are nothing to the global nothing value.
+  // TODO: If we follow this through, does it actually make it past the pass
+  // 'lowerIterators'?
   for_alive_in_Vec(BlockStmt, block, gBlockStmts) {
     if (ForLoop* loop = toForLoop(block)) {
-      if (loop->indexGet() && loop->indexGet()->typeInfo() == dtNothing) {
-        loop->indexGet()->setSymbol(gNone);
+      auto idx = loop->indexGet();
+      if (idx && idx->typeInfo() == dtNothing) {
+        for_SymbolSymExprs(se, idx->symbol()) se->setSymbol(gNone);
       }
     }
   }
 
   // Now that uses of nothing have been cleaned up, remove the
   // DefExprs for nothing variables.
   for_alive_in_Vec(DefExpr, def, gDefExprs) {
-      if (isNothingType(def->sym->type) ||
-          def->sym->type == dtNothing->refType) {
-        if (VarSymbol* var = toVarSymbol(def->sym)) {
-          // Avoid removing the "_val" field from refs
-          // and forall statements' induction/shadow variables.
-          if (! def->parentSymbol->hasFlag(FLAG_REF) &&
-              ! isForallIterVarDef(def)              &&
-              ! preserveShadowVar(var)               ) {
-            if (var != gNone) {
-              def->remove();
-            }
-          }
+    if (isNothingType(def->sym->type) ||
+        def->sym->type == dtNothing->refType) {
+      if (VarSymbol* var = toVarSymbol(def->sym)) {
+        // Avoid removing the "_val" field from refs
+        // and forall statements' induction/shadow variables.
+        if (!def->parentSymbol->hasFlag(FLAG_REF) &&
+            !isForallIterVarDef(def) &&
+            !preserveShadowVar(var) &&
+            var != gNone) {
+          // Otherwise we may be left with SymExpr that point to garbage.
+          for_SymbolSymExprs(se, var) se->setSymbol(gNone);
+          def->remove();
         }
       } else if (def->sym->type == dtUninstantiated &&
                  isVarSymbol(def->sym) &&
                  !def->parentSymbol->hasFlag(FLAG_REF)) {
         def->remove();
       }
+    }
   }
 
   adjustNothingShadowVariables();
@@ -923,10 +933,19 @@ static void cleanupNothingVarsAndFields() {
   // be left in the tree if optimizations are disabled, and can cause codegen
   // failures later on (at least under LLVM).
   //
-  // Solution: Remove SymExprs to none if the expr is at the
-  // statement level.
+  // Solution: Remove SymExprs to none if the expr is at the statement level.
   for_SymbolSymExprs(se, gNone) {
+    bool removeParent = false;
+    bool remove = false;
     if (se == se->getStmtExpr()) {
+      remove = true;
+    } else if (auto call = toCallExpr(se->parentExpr)) {
+      remove = call->isPrimitive(PRIM_END_OF_STATEMENT);
+      removeParent = remove && call->numActuals() == 1;
+    }
+    if (removeParent) {
+      se->parentExpr->remove();
+    } else if (remove) {
       se->remove();
     }
   }

compiler/resolution/functionResolution.cpp

@@ -4417,28 +4417,6 @@ static void resolveNormalCallConstRef(CallExpr* call) {
   }
 }
 
-// Returns the element type, given an array type.
-static Type* arrayElementType(AggregateType* arrayType) {
-  Type* eltType = NULL;
-  INT_ASSERT(arrayType->symbol->hasFlag(FLAG_ARRAY));
-  Type* instType = arrayType->getField("_instance")->type;
-  AggregateType* instClass = toAggregateType(canonicalClassType(instType));
-  eltType = instClass->getField("eltType")->getValType();
-  return eltType;
-}
-
-// Returns the element type, given an array type.
-// Recurse into it if it is still an array.
-static Type* finalArrayElementType(AggregateType* arrayType) {
-  Type* eltType = NULL;
-  do {
-    eltType = arrayElementType(arrayType);
-    arrayType = toAggregateType(eltType);
-  } while (arrayType != NULL && arrayType->symbol->hasFlag(FLAG_ARRAY));
-
-  return eltType;
-}
-
 // Is it OK to default-initialize an array with this element type?
 static bool okForDefaultInitializedArray(Type* eltType) {
   return isDefaultInitializable(eltType);
@@ -4497,7 +4475,7 @@ static void checkDefaultNonnilableArrayArg(CallExpr* call, FnSymbol* fn) {
          ! formal->hasFlag(FLAG_UNSAFE))
       if (AggregateType* actualType = toAggregateType(actualSym->getValType()))
        if (actualType->symbol->hasFlag(FLAG_ARRAY) &&
-           ! okForDefaultInitializedArray(finalArrayElementType(actualType)))
+           ! okForDefaultInitializedArray(actualType->finalArrayElementType()))
         //
         // Acceptable handling of the default actual is this:
         //   def default_arg_xxx: _array(...)
@@ -4513,7 +4491,7 @@ static void checkDefaultNonnilableArrayArg(CallExpr* call, FnSymbol* fn) {
          USR_FATAL_CONT(call, "cannot default-initialize the array field"
                         " %s because it has a non-nilable element type '%s'",
                         userFieldNameForError(actualSym),
-                        toString(finalArrayElementType(actualType), true));
+                        toString(actualType->finalArrayElementType(), true));
 }
 
 static void resolveNormalCallFinalChecks(CallExpr* call) {
@@ -13075,7 +13053,7 @@ static void lowerRuntimeTypeInit(CallExpr* call,
       USR_FATAL(call, "noinit is only supported for arrays");
     } else if (fAllowNoinitArrayNotPod == false) {
       // noinit of an array
-      Type* eltType = arrayElementType(at);
+      Type* eltType = at->arrayElementType();
       bool notPOD = propagateNotPOD(eltType);
       if (notPOD) {
         USR_FATAL_CONT(call, "noinit is only supported for arrays of trivially copyable types");
@@ -13444,9 +13422,10 @@ void lowerPrimInit(CallExpr* call, Expr* preventingSplitInit) {
       name = val->name;
     }
 
-    INT_ASSERT(val->type != dtUnknown && val->type != dtAny);
+    INT_ASSERT(isAggregateType(val->type));
     if (name != NULL) {
-      Type* eltType = finalArrayElementType(toAggregateType(val->type));
+      auto at = toAggregateType(val->type);
+      Type* eltType = at->finalArrayElementType();
       if (! okForDefaultInitializedArray(eltType))
         USR_FATAL_CONT(call, "cannot default-initialize the array %s"
                        " because it has a non-nilable element type '%s'",

compiler/resolution/resolveFunction.cpp

@@ -2118,12 +2118,14 @@ void resolveReturnTypeAndYieldedType(FnSymbol* fn, Type** yieldedType) {
     if (!fn->iteratorInfo) {
       if (retTypes.n == 0) {
         if (isIterator) {
-          // This feels like it should be:
-          // retType = dtVoid;
-          //
-          // but that leads to compiler generated assignments of 'void' to
-          // variables, which isn't allowed.  If we fib and claim that it
-          // returns 'nothing', those assignments get removed and all is well.
+          const bool emitError = !fn->hasFlag(FLAG_PROMOTION_WRAPPER);
+          if (emitError) {
+            // TODO: Right now this has to be USR_FATAL in order to avoid
+            // the possibility of subsequent errors about 'nothing'.
+            USR_FATAL(fn, "iterators with no reachable 'yield' statements "
+                          "must declare their return type");
+          }
+
           retType = dtNothing;
         } else {
           retType = dtVoid;

modules/internal/ChapelArray.chpl

@@ -935,6 +935,13 @@ module ChapelArray {
                              doiBulkTransferFromAny,  doiBulkTransferToAny,
                              chpl__serialize, chpl__deserialize;
 
+    // Hook into 'postinit' since arrays do not seem to offer 'init'.
+    proc postinit() {
+      if eltType == nothing {
+        compilerError("cannot initialize array with element type 'nothing'");
+      }
+    }
+
     @chpldoc.nodoc
     proc deinit() {
       _do_destroy_array(this);

modules/internal/ChapelRange.chpl

@@ -3318,7 +3318,7 @@ private proc isBCPindex(type t) param do
   // An error overload for trying to iterate over '..'
   pragma "order independent yielding loops"
   @chpldoc.nodoc
-  iter range.these() where !hasLowBoundForIter(this) && !hasHighBoundForIter(this) {
+  iter range.these(): nothing where !hasLowBoundForIter(this) && !hasHighBoundForIter(this) {
     compilerError("iteration over a range with no bounds");
   }
 

modules/standard/List.chpl

@@ -116,6 +116,9 @@ module List {
       compilerError("list element type cannot currently be generic");
       // In the future we might support it if the list is not default-inited
     }
+    if eltType == nothing {
+      compilerError("cannot initialize list with element type 'nothing'");
+    }
   }
 
   @chpldoc.nodoc

test/errhandling/parallel/forall-iterator-throws-at-end.chpl

@@ -20,7 +20,7 @@ iter myiter(nn: int, nt: int, param tag: iterKind) throws where tag == iterKind.
 }
 
 // for loop in follower with yield should get vector pragma
-iter myiter(nn:int, nt: int, followThis, param tag: iterKind) throws where tag == iterKind.follower {
+iter myiter(nn:int, nt: int, followThis, param tag: iterKind): int throws where tag == iterKind.follower {
   throw new owned StringError("Test error");
 }
 

test/errhandling/parallel/forall-iterator-throws-follower.chpl

@@ -20,8 +20,10 @@ iter myiter(nn: int, nt: int, param tag: iterKind) throws where tag == iterKind.
 }
 
 // for loop in follower with yield should get vector pragma
-iter myiter(nn:int, nt: int, followThis, param tag: iterKind) throws where tag == iterKind.follower {
+iter myiter(nn:int, nt: int, followThis, param tag: iterKind): nothing throws where tag == iterKind.follower {
   throw new owned StringError("Test error");
+  // This code is not reachable, so 'yield' does not color return type. Thus
+  // we say that the iterator returns 'nothing'.
   for i in followThis {
     yield i;
   }

test/errhandling/parallel/forall-iterator-throws-setup.chpl

@@ -4,20 +4,22 @@ config const n = 10;
 config const t = 2;
 
 
-iter myiter(nn: int, nt: int) throws {
+iter myiter(nn: int, nt: int): int throws {
 
   throw new owned StringError("Test error");
 
+  // This is not reachable.
   for i in 0..#nt {
     for j in i*nn..#nn {
       yield j;
     }
   }
 }
 
-iter myiter(nn: int, nt: int, param tag: iterKind) throws where tag == iterKind.standalone {
+iter myiter(nn: int, nt: int, param tag: iterKind): int throws where tag == iterKind.standalone {
   throw new owned StringError("Test error");
 
+  // This is not reachable.
   coforall i in 0..#nt {
     for j in i*nn..#nn {
       yield j;
@@ -26,9 +28,10 @@ iter myiter(nn: int, nt: int, param tag: iterKind) throws where tag == iterKind.
 }
 
 // coforall loop in leader should NOT get vector pragma
-iter myiter(nn: int, nt: int, param tag: iterKind) throws where tag == iterKind.leader {
+iter myiter(nn: int, nt: int, param tag: iterKind): range throws where tag == iterKind.leader {
   throw new owned StringError("Test error");
 
+  // This is not reachable.
   coforall i in 0..#nt {
     yield i*nn..#nn;
   }

test/functions/iterators/Issue21265.1.good

@@ -0,0 +1 @@
+Issue21265.chpl:6: error: iterators with no reachable 'yield' statements must declare their return type

test/functions/iterators/Issue21265.2.good

@@ -0,0 +1,3 @@
+$CHPL_HOME/modules/standard/List.chpl:204: In initializer:
+$CHPL_HOME/modules/standard/List.chpl:205: error: cannot initialize list with element type 'nothing'
+  Issue21265.chpl:11: called as list.init(type eltType = nothing, param parSafe = false)

test/functions/iterators/Issue21265.3.good

@@ -0,0 +1,2 @@
+Issue21265.chpl:14: In function 'test3':
+Issue21265.chpl:15: error: cannot initialize array with element type 'nothing'