Warn for generic declared return types without (?) (chapel-lang#23356)

For Cray/chapel-private#5229 /
Cray/chapel-private#4736 (comment)

Continuing chapel-lang#22745 and chapel-lang#23291, this PR adds a warning for routines
declared with a generic return type that does not include `(?)`. Note
that we already have a number of problems using `domain` or `domain(?)`
as a return or variable type (chapel-lang#23355, chapel-lang#23357, chapel-lang#23358); as a result
`domain(?)` cannot currently be used as a declared return type.

While there, this PR adds checking to fix chapel-lang#23346 by making `(?)` on a
concrete formal type into a compilation error. It also adds testing for
the variable and return type cases to make sure that they also result in
a compilation error.

For example:

``` chapel
record R { type t; }

proc a() : R { // warning
  return new R(int);
}
a();

proc b() : R(?) { // OK
  return new R(int);
}
b();

proc returnGenericType() type { return R; }

proc c() : returnGenericType() { // warning
  return new R(int);
}
c();

proc d() : returnGenericType()(?) { // OK
  return new R(int);
}
d();
```

Reviewed by @vasslitvinov and @lydia-duncan - thanks!

- [x] full comm=none testing
- [x] full gasnet testing

compiler/passes/normalize.cpp

@@ -103,6 +103,7 @@ static void        normalizeCallToTypeConstructor(CallExpr* call);
 static void        applyGetterTransform(CallExpr* call);
 static void        transformIfVar(CallExpr* call);
 static void        insertCallTemps(CallExpr* call);
+static void        propagateMarkedGeneric(Symbol* var, Expr* typeExpr);
 static Symbol*     insertCallTempsWithStmt(CallExpr* call, Expr* stmt);
 
 static void errorIfSplitInitializationRequired(DefExpr* def, Expr* cur);
@@ -1725,6 +1726,7 @@ static void addTypeBlocksForParentTypeOf(CallExpr* call) {
 ************************************** | *************************************/
 
 static void fixupExportedArrayReturns(FnSymbol* fn);
+static void fixupGenericReturnTypes(FnSymbol* fn);
 static bool isVoidReturn(CallExpr* call);
 static bool hasGenericArrayReturn(FnSymbol* fn);
 static void insertRetMove(FnSymbol* fn, VarSymbol* retval, CallExpr* ret,
@@ -1736,6 +1738,7 @@ static void normalizeReturns(FnSymbol* fn) {
   SET_LINENO(fn);
 
   fixupExportedArrayReturns(fn);
+  fixupGenericReturnTypes(fn);
 
   std::vector<CallExpr*> rets;
   std::vector<CallExpr*> calls;
@@ -1911,6 +1914,32 @@ static void fixupExportedArrayReturns(FnSymbol* fn) {
   }
 }
 
+// If the return type was declared generic e.g. as R(?), then
+// mark the function with FLAG_RET_TYPE_MARKED_GENERIC.
+// Also simplifies the simplest case to help with problems with
+//   proc f(): domain(?) { ... }
+static void fixupGenericReturnTypes(FnSymbol* fn) {
+  if (fn->retExprType) {
+    // handle nested cases, e.g. (GenericRecord(?), ) or borrowed GenCls(?)
+    propagateMarkedGeneric(fn, fn->retExprType);
+
+    // simpify the simple case
+    Expr*     tail   = fn->retExprType->body.tail;
+    if (CallExpr* call = toCallExpr(tail)) {
+      if (call->numActuals() == 1) {
+        if (SymExpr* se = toSymExpr(call->get(1))) {
+          if (call->baseExpr && se->symbol() == gUninstantiated) {
+            Expr* type = call->baseExpr->remove();
+            tail->replace(type);
+            // flag should have been added in propagateMarkedGeneric
+            INT_ASSERT(fn->hasFlag(FLAG_RET_TYPE_MARKED_GENERIC));
+          }
+        }
+      }
+    }
+  }
+}
+
 static void normalizeYields(FnSymbol* fn) {
   INT_ASSERT(fn->isIterator());
   SET_LINENO(fn);
@@ -2485,15 +2514,18 @@ static void insertCallTemps(CallExpr* call) {
   }
 }
 
-// adds FLAG_MARKED_GENERIC to 'var' if the type contains a ?
+// adds FLAG_MARKED_GENERIC/FLAG_RET_TYPE_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);
-      //      printf("(A) Adding FLAG_MARKED_GENERIC to %s\n", sym->name);
+      if (isFnSymbol(var))
+        var->addFlag(FLAG_RET_TYPE_MARKED_GENERIC);
+      else
+        var->addFlag(FLAG_MARKED_GENERIC);
     }
   } else if (CallExpr* call = toCallExpr(typeExpr)) {
     if (call->baseExpr)
@@ -2502,6 +2534,11 @@ static void propagateMarkedGeneric(Symbol* var, Expr* typeExpr) {
     for_actuals(actual, call) {
       propagateMarkedGeneric(var, actual);
     }
+  } else if (BlockStmt* blk = toBlockStmt(typeExpr)) {
+    // handle blocks since they are used for return type exprs
+    for_alist(expr, blk->body) {
+      propagateMarkedGeneric(var, expr);
+    }
   }
 }
 
@@ -3476,6 +3513,15 @@ void warnIfGenericFormalMissingQ(ArgSymbol* arg, Type* type, Expr* typeExpr) {
       }
     }
   }
+
+  // error for CR(?) where CR is a concrete record or class type
+  if (!type->symbol->hasFlag(FLAG_GENERIC) &&
+      arg->hasFlag(FLAG_MARKED_GENERIC)) {
+    USR_FATAL(arg,
+              "the formal argument '%s' is marked generic with (?) "
+              "but the type '%s' is not generic",
+              arg->name, toString(type, /*decorators*/ false));
+  }
 }
 
 static void hack_resolve_types(ArgSymbol* arg) {

compiler/resolution/callInfo.cpp

@@ -192,6 +192,9 @@ const char* CallInfo::toString() {
         ii->iterator->hasFlag(FLAG_PROMOTION_WRAPPER) == true) {
       retval = astr(retval, "promoted expression");
 
+    } else if (sym == gUninstantiated) {
+      retval = astr(retval, "?");
+
     } else if (sym->hasFlag(FLAG_TYPE_VARIABLE) == true) {
       retval = astr(retval, "type ", ::toString(type));
 

compiler/resolution/functionResolution.cpp

@@ -13883,6 +13883,42 @@ void checkDuplicateDecorators(Type* decorator, Type* decorated, Expr* ctx) {
 std::set<Symbol*> gAlreadyWarnedSurprisingGenericSyms;
 std::set<Symbol*> gAlreadyWarnedSurprisingGenericManagementSyms;
 
+static bool computeIsField(Symbol*& sym, AggregateType* forFieldInHere) {
+  // 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). If it's a field, replaces 'sym' with the
+  // field to use for error reporting.
+  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;
+        }
+      }
+    }
+  }
+
+  return isField;
+}
+
 void checkSurprisingGenericDecls(Symbol* sym, Expr* typeExpr,
                                  AggregateType* forFieldInHere) {
   if (sym == nullptr || typeExpr == nullptr) {
@@ -13897,40 +13933,22 @@ void checkSurprisingGenericDecls(Symbol* sym, Expr* typeExpr,
       declType = typeExpr->typeInfo();
     }
 
+    bool genericWithDefaults = false;
+    if (AggregateType* at = toAggregateType(declType))
+      genericWithDefaults = at->isGenericWithDefaults();
+
     if (declType->symbol->hasFlag(FLAG_GENERIC) &&
+        declType != dtAny && // workaround for interfaces
+        !genericWithDefaults &&
         !sym->hasFlag(FLAG_MARKED_GENERIC) &&
+        !sym->hasFlag(FLAG_RET_TYPE_MARKED_GENERIC) &&
         !sym->hasFlag(FLAG_TYPE_VARIABLE)) {
 
       // 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;
-            }
-          }
-        }
-      }
+      // Note: this can change 'sym' to a field rather than a tmp.
+      bool isField = computeIsField(sym, forFieldInHere);
 
       if (isClassLikeOrManaged(declType)) {
         auto dec = classTypeDecorator(declType);
@@ -14031,16 +14049,47 @@ void checkSurprisingGenericDecls(Symbol* sym, Expr* typeExpr,
             s.append("(?)");
           }
 
-          const char* fieldOrVar = isField?"field":"variable";
-          USR_WARN(sym, "please use '?' when declaring a %s with generic type",
-                   fieldOrVar);
+          if (isFnSymbol(sym)) {
+            USR_WARN(sym, "please use '?' when declaring a routine "
+                          "with a generic return type");
+
+          } else {
+            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());
         }
         if (fWarnUnstable) {
           USR_PRINT("this warning may be an error in the future");
         }
       }
     }
+
+    // if it was marked generic, but it's not generic (or an instantiation)
+    // then have a fatal error
+    if (sym->hasFlag(FLAG_MARKED_GENERIC) ||
+        sym->hasFlag(FLAG_RET_TYPE_MARKED_GENERIC)) {
+      if (!declType->symbol->hasFlag(FLAG_GENERIC) &&
+          !genericWithDefaults) {
+        bool isField = computeIsField(sym, forFieldInHere);
+
+        const char* thing = "";
+        if (isFnSymbol(sym)) {
+          thing = astr("return type of the routine ", sym->name);
+        } else if (isField) {
+          thing = astr("field ", sym->name);
+        } else {
+          thing = astr("variable ", sym->name);
+        }
+
+        USR_FATAL(sym,
+                  "the %s is marked generic with (?) "
+                  "but the type '%s' is not generic",
+                  thing, toString(declType, /*decorators*/ false));
+      }
+    }
   }
 }
 

compiler/resolution/resolveFunction.cpp

@@ -455,6 +455,8 @@ void resolveSpecifiedReturnType(FnSymbol* fn) {
 
   resolveBlockStmt(fn->retExprType);
 
+  checkSurprisingGenericDecls(fn, fn->retExprType->body.tail, nullptr);
+
   retType = fn->retExprType->body.tail->typeInfo();
 
   if (SymExpr* se = toSymExpr(fn->retExprType->body.tail)) {

frontend/include/chpl/uast/PragmaList.h

@@ -376,6 +376,7 @@ PRAGMA(LOOP_BODY_ARGUMENT_CLASS, npr, "loop body argument class", ncm)
 PRAGMA(MANAGED_POINTER, ypr, "managed pointer", "e.g. Owned and Shared")
 PRAGMA(MANAGED_POINTER_NONNILABLE, npr, "managed pointer nonnilable", "e.g. non-nilable Owned and Shared")
 PRAGMA(MARKED_GENERIC, npr, "marked generic", "marked generic using the type query syntax")
+PRAGMA(RET_TYPE_MARKED_GENERIC, npr, "ret type marked generic", "ret type marked generic with (?)")
 PRAGMA(SUPERCLASS_MARKED_GENERIC, npr, "supreclass marked generic", "superclass is marked generic")
 PRAGMA(MAYBE_ARRAY_TYPE, npr, "maybe array type", "function may be computing array type")
 PRAGMA(MAYBE_COPY_ELIDED, npr, "maybe copy elided", "symbol might be dead early due to copy elision")

modules/dists/ReplicatedDist.chpl

@@ -156,14 +156,14 @@ record Replicated {
   }
 
   @chpldoc.nodoc
-  inline operator ==(d1: Replicated(?), d2: Replicated(?)) {
+  inline operator ==(d1: Replicated, d2: Replicated) {
     if (d1._value == d2._value) then
       return true;
     return d1._value.dsiEqualDMaps(d2._value);
   }
 
   @chpldoc.nodoc
-  inline operator !=(d1: Replicated(?), d2: Replicated(?)) {
+  inline operator !=(d1: Replicated, d2: Replicated) {
     return !(d1 == d2);
   }
 
@@ -204,7 +204,7 @@ proc ReplicatedImpl.init(targetLocales: [] locale = Locales,
     writeln("ReplicatedImpl initializer over ", targetLocales);
 }
 
-proc ReplicatedImpl.dsiEqualDMaps(that: ReplicatedImpl(?)) {
+proc ReplicatedImpl.dsiEqualDMaps(that: ReplicatedImpl) {
   return this.targetLocales.equals(that.targetLocales);
 }
 

modules/standard/Version.chpl

@@ -626,7 +626,7 @@ module Version {
     return v2 <= v1;
   }
 
-  private proc spaceship(v1: version(?),
+  private proc spaceship(v1: version,
                          v2: versionValue(?)) : int {
     const majComp = spaceship(v1.major, v2.major);
     if majComp != 0 {

test/classes/delete-free/coercions-upon-return.chpl

@@ -5,7 +5,7 @@ class Gen {
 
 var Glob = new Gen(real);
 
-proc rets(): unmanaged Gen {
+proc rets(): unmanaged Gen(?) {
   return Glob.borrow();
 }
 

test/classes/delete-free/coercions-upon-yield.chpl

@@ -5,7 +5,7 @@ class Gen {
 
 var Glob = new Gen(real);
 
-iter rets(): unmanaged Gen {
+iter rets(): unmanaged Gen(?) {
   yield Glob.borrow();
 }
 

test/functions/compilation-errors/recursive-generic-return.good

@@ -1,2 +1,6 @@
+recursive-generic-return.chpl:8: warning: please use '?' when declaring a routine with a generic return type
+recursive-generic-return.chpl:8: note: for example with 'RR(?)'
 recursive-generic-return.chpl:12: warning: RR(int(64))
+recursive-generic-return.chpl:15: warning: please use '?' when declaring a routine with a generic return type
+recursive-generic-return.chpl:15: note: for example with 'RR(?)'
 recursive-generic-return.chpl:15: error: could not determine the concrete type for the generic return type 'RR'

test/functions/generic/genericReturnQ.chpl

@@ -0,0 +1,50 @@
+record CR { var x: int; }
+record GR { type t; var x: t; }
+record GRD { type t=int; var x: t; }
+class CC { var x: int; }
+class GC { type t; var x: t; }
+
+proc crNoQ(): CR { // OK
+  return new CR(1);
+}
+writeln(crNoQ());
+
+
+proc grNoQ(): GR { // expect a warning
+  return new GR(int, 1);
+}
+writeln(grNoQ());
+
+proc grQ(): GR(?) { // OK
+  return new GR(int, 1);
+}
+writeln(grQ());
+
+
+proc grdNoQ(): GRD { // OK
+  return new GRD(int, 1);
+}
+writeln(grdNoQ());
+
+proc grdQ(): GRD(?) { // OK
+  return new GRD(int, 1);
+}
+writeln(grdQ());
+
+
+proc ccNoQ(): CC { // OK
+  return new CC(1);
+}
+writeln(ccNoQ());
+
+
+proc gcNoQ(): GC { // expect a warning
+  return new GC(int, 1);
+}
+writeln(gcNoQ());
+
+proc gcQ(): GC(?) { // OK
+  return new GC(int, 1);
+}
+writeln(gcQ());
+