Warn for variables/fields declared with generic type without ?

compiler/AST/AggregateType.cpp

@@ -319,18 +319,11 @@ static bool isFieldTypeExprGeneric(Expr* typeExpr,
   }
 
   // Partial generic expressions with '?'
-  if (CallExpr* call = toCallExpr(typeExpr)) {
-    if (SymExpr* se = toSymExpr(call->baseExpr)) {
-      if (se->symbol()->type->symbol->hasFlag(FLAG_GENERIC)) {
-        for_actuals(actual, call) {
-          if (SymExpr* act = toSymExpr(actual)) {
-            if (act->symbol() == gUninstantiated) {
-              return true;
-            }
-          }
-        }
-      }
-    }
+  if (findSymExprFor(typeExpr, gUninstantiated)) {
+    // Note: this assumes that ? used in a nested call makes the type generic.
+    // That's not strictly true, but is close enough until
+    // we can use the dyno resolver to handle this in a more principled way.
+    return true;
   }
 
   if (UnresolvedSymExpr* urse = toUnresolvedSymExpr(typeExpr)) {
@@ -1888,7 +1881,7 @@ AggregateType* AggregateType::getNewInstantiation(Symbol* sym, Type* symType, Ex
 
   instantiations.push_back(retval);
 
-  checkSurprisingGenericDecls(field->defPoint, /* isField */ true);
+  checkSurprisingGenericDecls(field, field->defPoint->exprType, this);
 
   return retval;
 }

compiler/AST/baseAST.cpp

@@ -577,12 +577,14 @@ void update_symbols(BaseAST* ast, SymbolMap* map) {
         // BENHARSH TODO 2019-06-20: I think we need to do this because in
         // some cases the SymbolMap contains a mapping from the generic 'T' to
         // an instantiation of 'T'. Is that mapping necessary?
-        CallExpr* call = toCallExpr(sym_expr->parentExpr);
-        if (call != NULL && call->baseExpr == sym_expr) {
-          if (y->getValType()->symbol->hasFlag(FLAG_TUPLE) == false &&
-              y->getValType() != dtUnknown &&
-              sym_expr->symbol()->hasFlag(FLAG_TYPE_VARIABLE)) {
-            skip = true;
+        if (isTypeSymbol(sym_expr->parentSymbol)) {
+          CallExpr* call = toCallExpr(sym_expr->parentExpr);
+          if (call != NULL && call->baseExpr == sym_expr) {
+            if (y->getValType()->symbol->hasFlag(FLAG_TUPLE) == false &&
+                y->getValType() != dtUnknown &&
+                sym_expr->symbol()->hasFlag(FLAG_TYPE_VARIABLE)) {
+              skip = true;
+            }
           }
         }
 

compiler/include/resolution.h

@@ -391,7 +391,8 @@ void checkDuplicateDecorators(Type* decorator, Type* decorated, Expr* ctx);
 // emit a warning for
 //   var x: domain;
 // as a field or variable (it should be, var x: domain(?)).
-void checkSurprisingGenericDecls(DefExpr* def, bool isField);
+void checkSurprisingGenericDecls(Symbol* sym, Expr* typeExpr,
+                                 AggregateType* forFieldInHere);
 
 // These enable resolution for functions that don't really match
 // according to the language definition in order to get more errors

compiler/passes/normalize.cpp

@@ -2469,6 +2469,24 @@ static void insertCallTemps(CallExpr* call) {
   }
 }
 
+// adds FLAG_MARKED_GENERIC to 'var' if the type contains a ?
+// actual or an actual that is a temp marked with that flag.
+static void propagateMarkedGeneric(Symbol* var, Expr* typeExpr) {
+  if (SymExpr* se = toSymExpr(typeExpr)) {
+    Symbol* sym = se->symbol();
+    if (sym == gUninstantiated ||
+        (sym->hasFlag(FLAG_MARKED_GENERIC) && sym->hasFlag(FLAG_TEMP)))
+      var->addFlag(FLAG_MARKED_GENERIC);
+  } else if (CallExpr* call = toCallExpr(typeExpr)) {
+    if (call->baseExpr)
+      propagateMarkedGeneric(var, call->baseExpr);
+
+    for_actuals(actual, call) {
+      propagateMarkedGeneric(var, actual);
+    }
+  }
+}
+
 // returns the added call temp's symbol
 static Symbol *insertCallTempsWithStmt(CallExpr* call, Expr* stmt) {
   SET_LINENO(call);
@@ -2516,6 +2534,10 @@ static Symbol *insertCallTempsWithStmt(CallExpr* call, Expr* stmt) {
     tmp->addFlag(FLAG_SUPER_TEMP);
   }
 
+  // Set FLAG_MARKED_GENERIC if there was a (?) argument here or
+  // one of the nested calls used one.
+  propagateMarkedGeneric(tmp, call);
+
   call->replace(new SymExpr(tmp));
 
   stmt->insertBefore(new CallExpr(PRIM_MOVE, tmp, call));
@@ -2660,6 +2682,8 @@ static bool moveMakesTypeAlias(CallExpr* call) {
 ************************************** | *************************************/
 
 static void emitTypeAliasInit(Expr* after, Symbol* var, Expr* init) {
+  propagateMarkedGeneric(var, init);
+
   // Generate a type constructor call for generic-with-defaults types
   // (Note, this does not work correctly during resolution).
   // TODO: get it working in resolution

compiler/resolution/functionResolution.cpp

@@ -8227,6 +8227,8 @@ void resolveInitVar(CallExpr* call) {
     // If the target type is generic, compute the appropriate instantiation
     // type.
     if (genericTgt) {
+      checkSurprisingGenericDecls(dst, targetTypeExpr, nullptr);
+
       Type* inst = getInstantiationType(srcType, NULL, targetType, NULL, call,
                                         /* allowCoercion */ true,
                                         /* implicitBang */ false,
@@ -12905,6 +12907,7 @@ static void resolvePrimInit(CallExpr* call) {
 
   if (SymExpr* se = toSymExpr(typeExpr)) {
     if (se->symbol()->hasFlag(FLAG_TYPE_VARIABLE) == true) {
+      checkSurprisingGenericDecls(val, typeExpr, nullptr);
       resolvePrimInit(call, val, resolveTypeAlias(se));
     } else {
       USR_FATAL(call, "invalid type specification");
@@ -13776,44 +13779,170 @@ void checkDuplicateDecorators(Type* decorator, Type* decorated, Expr* ctx) {
   }
 }
 
-void checkSurprisingGenericDecls(DefExpr* def, bool isField) {
-  if (def == nullptr) {
+static bool isBuiltinGenericType(Type* t) {
+  return isBuiltinGenericClassType(t) ||
+         t == dtAnyComplex || t == dtAnyImag || t == dtAnyReal ||
+         t == dtAnyBool || t == dtAnyEnumerated ||
+         t == dtNumeric || t == dtIntegral ||
+         t == dtIteratorRecord || t == dtIteratorClass ||
+         t == dtAnyPOD ||
+         t == dtOwned || t == dtShared ||
+         t == dtAnyRecord;
+}
+
+std::set<Symbol*> gAlreadyWarnedSurprisingGenericSyms;
+std::set<Symbol*> gAlreadyWarnedSurprisingGenericManagementSyms;
+
+void checkSurprisingGenericDecls(Symbol* sym, Expr* typeExpr,
+                                 AggregateType* forFieldInHere) {
+  if (sym == nullptr || typeExpr == nullptr) {
     return;
   }
 
-  Symbol* sym = def->sym;
-
-  bool hasQuestionArg = false;
-  if (sym && def->exprType) {
-    if (CallExpr* call = toCallExpr(def->exprType)) {
-      for_actuals(actual, call) {
-        if (SymExpr* act = toSymExpr(actual)) {
-          if (act->symbol() == gUninstantiated) {
-            hasQuestionArg = true;
-          }
-        }
-      }
+  if (typeExpr) {
+    Type* declType = nullptr;
+    if (SymExpr* se = toSymExpr(typeExpr)) {
+      declType = resolveTypeAlias(se);
+    } else {
+      declType = typeExpr->typeInfo();
     }
-  }
 
-  if (def->exprType) {
-    Type* declType = def->exprType->typeInfo();
     if (declType->symbol->hasFlag(FLAG_GENERIC) &&
-        !hasQuestionArg &&
         !sym->hasFlag(FLAG_MARKED_GENERIC) &&
         !sym->hasFlag(FLAG_TYPE_VARIABLE)) {
-      if (isField && isClassLikeOrManaged(declType)) {
+
+      // is it a field? check to see if it's a temp within an initializer
+      // initializing field (in which case, we should think about
+      // this as a field).
+      bool isField = false;
+
+      if (forFieldInHere) {
+        AggregateType* ct = forFieldInHere->getRootInstantiation();
+        Symbol* field = ct->getField(sym->name);
+        isField = true;
+        sym = field;
+      } else if (sym->hasFlag(FLAG_TEMP)) {
+        for_SymbolSymExprs(se, sym) {
+          if (CallExpr* c = toCallExpr(se->parentExpr)) {
+            if ((c->isPrimitive(PRIM_SET_MEMBER) ||
+                 c->isPrimitive(PRIM_INIT_FIELD)) && se == c->get(3)) {
+              isField = true;
+
+              // replace 'sym' with the field for the error location
+              AggregateType* ct = toAggregateType(c->get(1)->getValType());
+              ct = ct->getRootInstantiation();
+              SymExpr* nameSe = toSymExpr(c->get(2));
+              VarSymbol* nameVar = toVarSymbol(nameSe->symbol());
+              const char* name = astr(nameVar->immediate->v_string.c_str());
+              Symbol* field = ct->getField(name);
+              sym = field;
+              break;
+            }
+          }
+        }
+      }
+
+      if (isClassLikeOrManaged(declType)) {
         auto dec = classTypeDecorator(declType);
-        if (isDecoratorUnknownManagement(dec)) {
+        if (isField && isDecoratorUnknownManagement(dec)) {
+          auto pair = gAlreadyWarnedSurprisingGenericManagementSyms.insert(sym);
+          if (!pair.second) {
+            // don't warn twice for the same field
+            return;
+          }
+
           USR_WARN(sym, "field is declared with generic memory management");
           USR_PRINT("consider adding 'owned', 'shared', or 'borrowed'");
           USR_PRINT("if generic memory management is desired, "
                     "use a 'type' field to store the class type");
         }
+
+        // consider the class type ignoring management for
+        // the rest of the checks.
+        declType = canonicalClassType(declType);
+      }
+
+
+      // supress the warning for builtin types like
+      // 'integral', 'record', 'borrowed'.
+      if (isBuiltinGenericType(declType)) {
+        return;
+      }
+
+      // suppress the warning for formal temps e.g.
+      // proc f(out arg: R) for a generic record R
+      if (sym->hasFlag(FLAG_FORMAL_TEMP))
+        return;
+
+
+      // supress the warning for fields within owned/shared
+      // themselves (better to see the warning at uses of owned/shared
+      // that create generic owned/shared).
+      if (TypeSymbol* ts = toTypeSymbol(sym->defPoint->parentSymbol))
+        if (ts->hasFlag(FLAG_MANAGED_POINTER))
+          return;
+
+      bool hasQuestionArg = sym->hasFlag(FLAG_MARKED_GENERIC);
+
+      // Inspect the AST to decide if there was a question mark arg
+      // (such as 'R(?)'). This is particularly important for variable
+      // declarations with the current AST.
+      while (hasQuestionArg == false) {
+        if (SymExpr* se = toSymExpr(typeExpr)) {
+          if (se->symbol()->hasFlag(FLAG_MARKED_GENERIC)) {
+            hasQuestionArg = true;
+            break;
+          }
+
+          if (SymExpr* singleDef = se->symbol()->getSingleDef()) {
+            // Figure out the RHS of the singleDef
+            if (CallExpr* call = toCallExpr(singleDef->parentExpr)) {
+              if (call->isPrimitive(PRIM_MOVE) && call->get(1) == singleDef) {
+                  typeExpr = call->get(2);
+                  continue;
+              }
+            }
+          }
+        } else if (CallExpr* call = toCallExpr(typeExpr)) {
+          for_actuals(actual, call) {
+            if (SymExpr* act = toSymExpr(actual)) {
+              if (act->symbol() == gUninstantiated) {
+                hasQuestionArg = true;
+                break;
+              }
+            }
+          }
+        }
+
+        break;
       }
-      if (declType->symbol->hasFlag(FLAG_DOMAIN)) {
-        USR_WARN(sym, "please use 'domain(?)' for the type of a generic %s "
-                      "storing any domain", isField?"field":"variable");
+
+      if (declType->symbol->hasFlag(FLAG_GENERIC) && !hasQuestionArg) {
+        auto pair = gAlreadyWarnedSurprisingGenericSyms.insert(sym);
+        if (!pair.second) {
+          // don't warn twice for the same variable/field
+          return;
+        }
+
+        if (declType->symbol->hasFlag(FLAG_DOMAIN)) {
+          USR_WARN(sym, "please use 'domain(?)' for the type of a generic %s "
+                        "storing any domain", isField?"field":"variable");
+        } else {
+          std::string s = toString(declType, false);
+          // figure out what to suggest
+          //  * if there is a (...args...), add a ? arg
+          //  * if there is no argument list, add (?)
+          if (!s.empty() && s.back() == ')') {
+            s.insert(s.size()-1, ",?");
+          } else {
+            s.append("(?)");
+          }
+
+          const char* fieldOrVar = isField?"field":"variable";
+          USR_WARN(sym, "please use '?' when declaring a %s with generic type",
+                   fieldOrVar);
+          USR_PRINT(sym, "for example with '%s'", s.c_str());
+        }
       }
     }
   }

compiler/resolution/postFold.cpp

@@ -126,6 +126,27 @@ Expr* postFold(Expr* expr) {
 
         if (doFoldAway) {
           retval = se->copy();
+          // if call is RHS of a PRIM_MOVE setting a temporary,
+          // record existence of ? argument by setting FLAG_MARKED_GENERIC.
+          if (CallExpr* parentCall = toCallExpr(call->parentExpr)) {
+            if (parentCall->isPrimitive(PRIM_MOVE)) {
+              if (SymExpr* lhsSe = toSymExpr(parentCall->get(1))) {
+                bool hasQuestionArg = false;
+                for_actuals(actual, call) {
+                  if (SymExpr* act = toSymExpr(actual)) {
+                    if (act->symbol() == gUninstantiated) {
+                      hasQuestionArg = true;
+                    }
+                  }
+                }
+                // TODO: Is this still necessary? The normalizer probably
+                // covers this case now.
+                if (hasQuestionArg && lhsSe->symbol()->hasFlag(FLAG_TEMP)) {
+                  lhsSe->symbol()->addFlag(FLAG_MARKED_GENERIC);
+                }
+              }
+            }
+          }
           call->replace(retval);
         }
       }

compiler/resolution/virtualDispatch.cpp

@@ -204,6 +204,8 @@ static FnSymbol* getInstantiatedFunction(FnSymbol* pfn,
 
   if (_thisAt->isGeneric() == true) {
     subs.put(_this, ct->symbol);
+  } else {
+    INT_ASSERT(!_thisAt->symbol->hasFlag(FLAG_GENERIC));
   }
 
   for (int i = 3; i <= cfn->numFormals(); i++) {

modules/internal/ChapelDistribution.chpl

@@ -626,7 +626,7 @@ module ChapelDistribution {
 
   record SparseIndexBuffer {
     param rank: int;
-    var obj: borrowed BaseSparseDom;
+    var obj: borrowed BaseSparseDom(?);
 
     type idxType = if rank==1 then int else rank*int;
     var bufDom = domain(1);

modules/internal/DefaultAssociative.chpl

@@ -36,7 +36,7 @@ module DefaultAssociative {
 
   // helps to move around array elements when rehashing the domain
   class DefaultAssociativeDomRehashHelper : chpl__rehashHelpers {
-    var dom: unmanaged DefaultAssociativeDom;
+    var dom: unmanaged DefaultAssociativeDom(?);
     override proc startRehash(newSize: int) {
       for arr in dom._arrs {
         arr._startRehash(newSize);

modules/packages/Channel.chpl

@@ -276,8 +276,8 @@ module Channel {
     var recvIdx = 0;
     var count = 0;
     var closed = false;
-    var sendWaiters : owned WaiterQueue;
-    var recvWaiters : owned WaiterQueue;
+    var sendWaiters : owned WaiterQueue(eltType);
+    var recvWaiters : owned WaiterQueue(eltType);
     var lock$ = new _LockWrapper();
 
     proc init(type elt, size = 0) {

modules/packages/Sort.chpl

@@ -273,7 +273,7 @@ defaultComparator = new DefaultComparator();
    default sort order.
 
  */
-const reverseComparator: ReverseComparator;
+const reverseComparator: ReverseComparator(DefaultComparator);
 reverseComparator = new ReverseComparator();
 
 /* Private methods */