Add warnings for some numeric implicit conversions (chapel-lang#24529)

This PR adds an opt-in warning for implicit numeric conversion from an
int/uint of size < 64 bits to a real/complex of non-default size. This
warning is meant to address issue chapel-lang#24496. If we can improve this warning
to be more focused, we might turn it on-by-default in the future. This
warning can be activated with `--warn-small-integral-to-float`.

It also adds an unstable warning for such cases with 8- or 16- bit
`int`/`uint` implicitly converting to `real(32)`, since something like
`sqrt(myInt16)` might well result in a different type after we add
`real(16)`. This unstable warning will appear if compiling with
`--warn-unstable`.

In addition, this PR adds a number of other off-by-default warnings to
address chapel-lang#20687. These will only warn for user code (rather than standard
modules), just to avoid a large amount of useless information coming
from them. However, that approach has the drawback that an implicit
conversion could occur in an internal module and propagate to user code.
However, I think these warnings will still be useful to users even with
that caveat.

The new flags are:
 * `--[no-]warn-int-to-uint` to warn for int->uint
* `--[no-]warn-small-integral-to-float` to warn for int/uint 8/16/32 ->
real(32)/complex(64)
* `--[no-]warn-integral-to-float` to warn for any int/uint ->
real/complex
* `--[no-]warn-float-to-float` to warn for real(32) -> real(64) and
similar for imag/complex
* `--[no-]warn-integral-to-integral` to warn for int/uint to larger
int/uint
* `--[no-]warn-implicit-numeric-conversions` to activate all of the
above
* `--[no-]warn-param-implicit-numeric-conversions` to make the above
more sensitive to param values

Reviewed by @dlongnecke-cray and @bradcray - thanks!

- [x] comm=none testing

compiler/AST/type.cpp

@@ -1502,6 +1502,13 @@ int get_width(Type *t) {
   return 0;
 }
 
+int get_component_width(Type *t) {
+  if (is_complex_type(t)) {
+    return get_width(t) / 2;
+  }
+  return get_width(t);
+}
+
 // numbers between -2**width .. 2**width
 // will fit exactly in a floating-point representation.
 int get_mantissa_width(Type *t) {

compiler/include/driver.h

@@ -239,8 +239,16 @@ extern bool ignore_errors;
 extern bool ignore_user_errors;
 extern bool ignore_errors_for_pass;
 extern int  squelch_header_errors;
-extern bool fWarnConstLoops;
+
 extern bool fWarnIntUint;
+extern bool fWarnSmallIntegralFloat;
+extern bool fWarnIntegralFloat;
+extern bool fWarnFloatFloat;
+extern bool fWarnIntegralIntegral;
+extern bool fWarnImplicitNumericConversions;
+extern bool fWarnParamImplicitNumericConversions;
+
+extern bool fWarnConstLoops;
 extern bool fWarnUnstable;
 extern bool fWarnUnstableStandard;
 extern bool fWarnUnstableInternal;

compiler/include/resolution.h

@@ -303,8 +303,9 @@ void resolveNormalCallCompilerWarningStuff(CallExpr* call, FnSymbol* resolvedFn)
 
 void checkMoveIntoClass(CallExpr* call, Type* lhs, Type* rhs);
 
-void warnForIntUintConversion(BaseAST* context, Type* formalType,
-                              Type* actualType, Symbol* actual);
+// warn for some int -> uint and small int -> real
+void warnForSomeNumericConversions(BaseAST* context, Type* formalType,
+                                   Type* actualType, Symbol* actual);
 
 void lvalueCheck(CallExpr* call);
 

compiler/include/type.h

@@ -589,7 +589,12 @@ bool is_imag_type(Type*);
 bool is_complex_type(Type*);
 bool is_enum_type(Type*);
 bool isLegalParamType(Type*);
+// returns the width in bytes of a numeric type
 int  get_width(Type*);
+// returns the component width in bytes of a numeric type
+// like get_width but for complex types, returns get_width/2
+// since that is the width of the real or imaginary component.
+int  get_component_width(Type*);
 int  get_mantissa_width(Type*);
 int  get_exponent_width(Type*);
 bool isClass(Type* t); // includes ref, ddata, classes; not unmanaged

compiler/main/driver.cpp

@@ -219,9 +219,18 @@ FILE* printPassesFile = NULL;
 // flag for llvmWideOpt
 bool fLLVMWideOpt = false;
 
+// warnings for various implicit numeric conversions
+bool fWarnIntUint = false;
+bool fWarnSmallIntegralFloat = false;
+bool fWarnIntegralFloat = false;
+bool fWarnFloatFloat = false;
+bool fWarnIntegralIntegral = false;
+bool fWarnImplicitNumericConversions = false;
+bool fWarnParamImplicitNumericConversions = false;
+
+// other warnings
 bool fWarnArrayOfRange = true;
 bool fWarnConstLoops = true;
-bool fWarnIntUint = false;
 bool fWarnUnstable = false;
 bool fWarnUnstableStandard = false;
 bool fWarnUnstableInternal = false;
@@ -663,6 +672,17 @@ static void addUsingAttributeToolname(const ArgumentDescription* desc,
   usingAttributeToolNames.push_back(name);
 }
 
+/* called for --warn-implicit-numeric-conversions to enable the warnings */
+static void setNumericWarnings(const ArgumentDescription* desc,
+                               const char* arg) {
+  bool shouldWarn = fWarnImplicitNumericConversions;
+  fWarnIntUint = shouldWarn;
+  fWarnSmallIntegralFloat = shouldWarn;
+  fWarnIntegralFloat = shouldWarn;
+  fWarnFloatFloat = shouldWarn;
+  fWarnIntegralIntegral = shouldWarn;
+}
+
 // In order to handle accumulating ccflags arguments, the argument
 // processing calls this function. This function appends the flags
 // to the ccflags variable, so that multiple --ccflags arguments
@@ -1231,12 +1251,19 @@ static ArgumentDescription arg_desc[] = {
  {"print-search-dirs", ' ', NULL, "[Don't] print module search path", "N", &printSearchDirs, "CHPL_PRINT_SEARCH_DIRS", NULL},
 
  {"", ' ', NULL, "Warning and Language Control Options", NULL, NULL, NULL, NULL},
- {"permit-unhandled-module-errors", ' ', NULL, "Permit unhandled errors in explicit modules; such errors halt at runtime", "N", &fPermitUnhandledModuleErrors, "CHPL_PERMIT_UNHANDLED_MODULE_ERRORS", NULL},
+ {"permit-unhandled-module-errors", ' ', NULL, "Permit unhandled thrown errors; such errors halt at runtime", "N", &fPermitUnhandledModuleErrors, "CHPL_PERMIT_UNHANDLED_MODULE_ERRORS", NULL},
  {"warn-unstable", ' ', NULL, "Enable [disable] warnings for uses of language features that are in flux", "N", &fWarnUnstable, "CHPL_WARN_UNSTABLE", NULL},
  {"warnings", ' ', NULL, "Enable [disable] output of warnings", "n", &ignore_warnings, "CHPL_WARNINGS", NULL},
  {"warn-unknown-attribute-toolname", ' ', NULL, "Enable [disable] warnings when an unknown tool name is found in an attribute", "N", &fWarnUnknownAttributeToolname, "CHPL_WARN_UNKNOWN_ATTRIBUTE_TOOLNAME", NULL},
  {"using-attribute-toolname", ' ', "<toolname>", "Specify additional tool names for attributes that are expected in the source", "S", NULL, "CHPL_ATTRIBUTE_TOOLNAMES", addUsingAttributeToolname},
-{"warn-potential-races", ' ', NULL, "Enable [disable] output of warnings for potential race conditions", "N", &fWarnPotentialRaces, "CHPL_WARN_POTENTIAL_RACES", NULL},
+ {"warn-potential-races", ' ', NULL, "Enable [disable] output of warnings for potential race conditions", "N", &fWarnPotentialRaces, "CHPL_WARN_POTENTIAL_RACES", NULL},
+ {"warn-int-to-uint", ' ', NULL, "Enable [disable] warnings for implicitly converting a potentially negative int value of any width to a uint", "N", &fWarnIntUint, "CHPL_WARN_INT_TO_UINT", NULL},
+ {"warn-small-integral-to-float", ' ', NULL, "Enable [disable] warnings for implicitly converting a small int/uint to a small real/complex", "N", &fWarnSmallIntegralFloat, "CHPL_WARN_SMALL_INTEGRAL_TO_FLOAT", NULL},
+ {"warn-integral-to-float", ' ', NULL, "Enable [disable] warnings for implicitly converting an int/uint to a real/complex of any width", "N", &fWarnIntegralFloat, "CHPL_WARN_INTEGRAL_TO_FLOAT", NULL},
+ {"warn-float-to-float", ' ', NULL, "Enable [disable] warnings for implicitly converting a real/imag/complex to a real/imag/complex with different precision", "N", &fWarnFloatFloat, "CHPL_WARN_REAL_REAL", NULL},
+ {"warn-integral-to-integral", ' ', NULL, "Enable [disable] warnings for implicitly converting an int/uint to an int/uint with different size", "N", &fWarnIntegralIntegral, "CHPL_WARN_INTEGRAL_TO_INTEGRAL", NULL},
+ {"warn-implicit-numeric-conversions", ' ', NULL, "Enable [disable] warnings for implicitly converting a value of numeric type to a different numeric type", "N", &fWarnImplicitNumericConversions, "CHPL_WARN_IMPLICIT_NUMERIC_CONVERSIONS", setNumericWarnings},
+ {"warn-param-implicit-numeric-conversions", ' ', NULL, "Enable [disable] int-to-uint, real-to-real, and integral-to-integral implicit conversion warnings to apply to 'param' values", "N", &fWarnParamImplicitNumericConversions, "CHPL_WARN_PARAM_IMPLICIT_NUMERIC_CONVERSIONS", NULL},
 
  {"", ' ', NULL, "Parallelism Control Options", NULL, NULL, NULL, NULL},
  {"local", ' ', NULL, "Target one [many] locale[s]", "N", &fLocal, "CHPL_LOCAL", setLocal},
@@ -1487,7 +1514,6 @@ static ArgumentDescription arg_desc[] = {
  {"warn-array-of-range", ' ', NULL, "Enable [disable] warnings about arrays of range literals", "N", &fWarnArrayOfRange, "CHPL_WARN_ARRAY_OF_RANGE", NULL},
  {"warn-const-loops", ' ', NULL, "Enable [disable] warnings for some 'while' loops with constant conditions", "N", &fWarnConstLoops, "CHPL_WARN_CONST_LOOPS", NULL},
  {"warn-domain-literal", ' ', NULL, "Enable [disable] old domain literal syntax warnings", "n", &fNoWarnDomainLiteral, "CHPL_WARN_DOMAIN_LITERAL", setWarnDomainLiteral},
- {"warn-int-uint", ' ', NULL, "Enable [disable] warnings for potentially negative 'int' values implicitly converted to 'uint'", "N", &fWarnIntUint, "CHPL_WARN_INT_UINT", NULL},
  {"warn-tuple-iteration", ' ', NULL, "Enable [disable] warnings for tuple iteration", "n", &fNoWarnTupleIteration, "CHPL_WARN_TUPLE_ITERATION", setWarnTupleIteration},
  {"warn-special", ' ', NULL, "Enable [disable] special warnings", "n", &fNoWarnSpecial, "CHPL_WARN_SPECIAL", setWarnSpecial},
  {"warn-unstable-internal", ' ', NULL, "Enable [disable] unstable warnings in internal modules", "N", &fWarnUnstableInternal, NULL, NULL},

compiler/resolution/functionResolution.cpp

@@ -8174,39 +8174,134 @@ void checkMoveIntoClass(CallExpr* call, Type* lhs, Type* rhs) {
 }
 
 
-void warnForIntUintConversion(BaseAST* context,
-                              Type* formalType,
-                              Type* actualType,
-                              Symbol* actual) {
-  if (fWarnIntUint || shouldWarnUnstableFor(context)) {
-    Type* formalVt = formalType->getValType();
-    Type* actualVt = actualType->getValType();
-    if (is_uint_type(formalVt) && is_int_type(actualVt)) {
-      if (get_width(formalVt) <= get_width(actualVt)) {
-        bool isParam = false;
-        bool isNegParam = false;
-
-        if (VarSymbol* var = toVarSymbol(actual)) {
-          if (Immediate* imm = var->immediate) {
-            isParam = true;
-            isNegParam = is_negative(imm);
-          }
+void warnForSomeNumericConversions(BaseAST* context,
+                                   Type* formalType,
+                                   Type* actualType,
+                                   Symbol* actual) {
+
+  Type* formalVt = formalType->getValType();
+  Type* actualVt = actualType->getValType();
+  bool formalFloatingPoint = is_real_type(formalVt) ||
+                             is_imag_type(formalVt) ||
+                             is_complex_type(formalVt);
+  bool actualFloatingPoint = is_real_type(actualVt) ||
+                             is_imag_type(actualVt) ||
+                             is_complex_type(actualVt);
+  bool formalIntUint = is_int_type(formalVt) || is_uint_type(formalVt);
+  bool actualIntUint = is_int_type(actualVt) || is_uint_type(actualVt);
+
+  // nothing to do if the formal is not numeric
+  if (!formalFloatingPoint && !formalIntUint) return;
+  // nothing to do if the actual is not numeric
+  if (!actualFloatingPoint && !actualIntUint) return;
+
+  int formalWidth = get_component_width(formalVt);
+  int actualWidth = get_component_width(actualVt);
+
+  bool actualIsParam = false;
+  if (VarSymbol* var = toVarSymbol(actual)) {
+    if (var->immediate != nullptr) {
+      actualIsParam = true;
+    }
+  }
+
+  bool userModule = false;
+  if (ModuleSymbol* mod = context->getModule()) {
+    userModule = mod->modTag == MOD_USER;
+  }
+
+  // consider warning for int -> uint implicit conversion
+  if (formalIntUint && actualIntUint &&
+      is_int_type(actualVt) && is_uint_type(formalVt)) {
+    // note: used to check formalWidth <= actualWidth
+    // but that doesn't make sense to me; if the concern is
+    // it could a be negative int, the widths don't matter
+
+    if (fWarnIntUint || shouldWarnUnstableFor(context)) {
+      bool isParam = actualIsParam;
+      bool isNegParam = false;
+
+      if (VarSymbol* var = toVarSymbol(actual)) {
+        if (Immediate* imm = var->immediate) {
+          isNegParam = is_negative(imm);
         }
+      }
 
-        // If it's a param, warn only if it is negative.
-        // Otherwise, warn.
-        if (isNegParam || !isParam) {
-          if (fWarnIntUint) {
-            USR_WARN(context, "potentially surprising int->uint implicit conversion");
-          } else {
-            USR_WARN(context, "int->uint implicit conversion is unstable");
-          }
-          USR_PRINT(context, "add a cast :%s to avoid this warning",
-                    toString(formalVt));
+      // If it's a param, warn only if it is negative.
+      // Otherwise, warn.
+      if (isNegParam || !isParam ||
+          (fWarnParamImplicitNumericConversions && userModule)) {
+        if (fWarnIntUint) {
+          USR_WARN(context, "potentially surprising implicit conversion "
+                            "from '%s' to '%s'",
+                            toString(actualVt), toString(formalVt));
+        } else {
+          USR_WARN(context, "int->uint implicit conversion is unstable");
         }
+        USR_PRINT(context, "add a cast :%s to avoid this warning",
+                  toString(formalVt));
+        return;
+      }
+    }
+  }
+
+  // consider warning for small int -> small real implicit conversion
+  if (actualIntUint && formalFloatingPoint &&
+      actualWidth < 64 && formalWidth != 64) {
+    if (fWarnSmallIntegralFloat || shouldWarnUnstableFor(context)) {
+      if (fWarnSmallIntegralFloat) {
+        USR_WARN(context, "potentially surprising implicit conversion "
+                          "from '%s' to '%s'",
+                          toString(actualVt), toString(formalVt));
+        USR_PRINT(context, "add a cast :%s to avoid this warning",
+                  toString(formalVt));
+        return;
+      } else if (actualWidth < 32) {
+        // unstable warning focuses on 8 and 16 bit int/uint
+        USR_WARN(context, "implicit conversion from 8- and 16- bit integral "
+                          "types to 'real(32)' is unstable");
+        USR_PRINT(context, "add a cast :%s to avoid this warning",
+                  toString(formalVt));
+        return;
       }
     }
   }
+
+  // consider warning for any int -> real implicit conversion
+  if (fWarnIntegralFloat && userModule &&
+      actualIntUint && formalFloatingPoint) {
+    if (!actualIsParam || fWarnParamImplicitNumericConversions) {
+      USR_WARN(context, "implicit conversion from '%s' to '%s'",
+               toString(actualVt), toString(formalVt));
+      USR_PRINT(context, "add a cast :%s to avoid this warning",
+                toString(formalVt));
+      return;
+    }
+  }
+
+  // consider warning for real(t) -> real(u) implicit conversion
+  if (fWarnFloatFloat && actualFloatingPoint && formalFloatingPoint &&
+      actualWidth != formalWidth && userModule) {
+    if (!actualIsParam || fWarnParamImplicitNumericConversions) {
+      USR_WARN(context, "implicit conversion from '%s' to '%s'",
+               toString(actualVt), toString(formalVt));
+      USR_PRINT(context, "add a cast :%s to avoid this warning",
+                toString(formalVt));
+      return;
+    }
+  }
+
+  // consider warning for int/uint -> int/uint implicit conversion
+  if (fWarnIntegralIntegral && actualIntUint && formalIntUint &&
+      actualWidth != formalWidth && userModule) {
+    if (!actualIsParam || fWarnParamImplicitNumericConversions) {
+      USR_WARN(context, "implicit conversion from '%s' to '%s'",
+               toString(actualVt), toString(formalVt));
+      USR_PRINT(context, "add a cast :%s to avoid this warning",
+                toString(formalVt));
+      return;
+    }
+  }
 }
 
 

compiler/resolution/postFold.cpp

@@ -173,6 +173,17 @@ static Expr* postFoldNormal(CallExpr* call) {
     VarSymbol* ret = toVarSymbol(fn->getReturnSymbol());
 
     if (ret != NULL && ret->immediate != NULL) {
+
+      // warn for certain numeric implicit conversions before we forget
+      // everything about the call
+      for_formals_actuals(formal, actual, call) {
+        if (SymExpr* actualSe = toSymExpr(actual)) {
+          warnForSomeNumericConversions(call,
+                                        formal->typeInfo(), actual->typeInfo(),
+                                        actualSe->symbol());
+        }
+      }
+
       retval = new SymExpr(ret);
 
       call->replace(retval);

compiler/resolution/resolveFunction.cpp

@@ -2763,7 +2763,7 @@ static void insertInitConversion(Symbol* to, Symbol* toType, Symbol* from,
     INT_ASSERT(toValType == toType->type);
 
     // generate a warning in some cases for int->uint implicit conversion
-    warnForIntUintConversion(insertBefore, toValType, fromValType, from);
+    warnForSomeNumericConversions(insertBefore, toValType, fromValType, from);
   }
 
   // seemingly redundant toType->type->symbol is for lowered runtime type vars

compiler/resolution/wrappers.cpp

@@ -1550,8 +1550,9 @@ static void addArgCoercion(FnSymbol*  fn,
   SET_LINENO(actual);
 
   // generate a warning in some cases for int->uint implicit conversion
-  warnForIntUintConversion(call, formal->type, actual->symbol()->type,
-                           actual->symbol());
+  // generate a warning in some cases for small int -> real implicit conversion
+  warnForSomeNumericConversions(call, formal->type, actual->symbol()->type,
+                                actual->symbol());
 
   Symbol*     prevActual = actual->symbol();
   TypeSymbol* ats        = prevActual->type->symbol;

man/chpl.rst

@@ -143,6 +143,51 @@ OPTIONS
     operation may be race condition and will warn with this flag. Defaults to
     not printing race condition warnings.
 
+**\--[no-]warn-int-to-uint**
+
+    Enable [disable] compilation warnings for when implicitly converting
+    from a value of ``int`` type of any width to a ``uint`` value.
+
+**\--[no-]warn-small-integral-to-float**
+
+    Enable [disable] compilation warnings for when implicitly converting
+    from a value of small integral type to a small floating-point value.
+    More specifically, it will warn when implicitly converting something
+    of type ``int(t)`` or ``uint(t)`` where ``t<64``, to something of
+    type ``real(u)`` or ``complex(2*u)`` where ``u<64``.
+
+**\--[no-]warn-integral-to-float**
+
+    Enable [disable] compilation warnings for when implicitly converting
+    from a value of ``int`` or ``uint`` type of any width to a ``real``
+    or ``complex`` type of any width.
+
+**\--[no-]warn-float-to-float**
+
+    Enable [disable] compilation warnings for when implicitly converting
+    from a floating-point type of one precision to another. That includes
+    implicitly converting from ``real(32)`` to ``real(64)`` as well as
+    similar cases with ``imag`` and ``complex`` types.
+
+**\--[no-]warn-integral-to-integral**
+
+    Enable [disable] compilation warnings for when implicitly converting
+    from a value of integral type to another integral type of different width.
+    (An integral type is an ``int`` or ``uint`` type).
+
+**\--[no-]warn-implicit-numeric-conversions**
+
+    Enable [disable] the above compilation warnings for implicitly
+    converting between numeric types.
+
+**\--[no-]warn-param-implicit-numeric-conversions**
+
+    When used in conjunction with ``warn-int-uint``,
+    ``--warn-real-real``, or ``--warn-integral-integral``, this flag
+    enables [or disables] these compilation warnings about implicitly
+    converting between numeric types to also apply when the converted
+    value is a ``param``.
+
 *Parallelism Control Options*
 
 **\--[no-]local**