Merge pull request #14956 from mppf/init-eq-assign-error

Add check for both init= = or none

Related to issue #8065.

Previously, the compiler generated `init=` and `=` for all types, but did
so separately. That means that one could use the compiler-generated `=`
but a custom `init=` or vice versa. This could in some cases lead to
confusing and hard to debug problems if a custom `init=` or `=` was
forgotten or included the wrong type signature.

To resolve these problems, this PR started by adjusting the compiler to
check that if a type has custom `init=`, it does not use the default `=`;
and that if a type has a custom `=`, it does not use the default `init=`.

The compiler currently only considers the result type (`this` for `init=`
and the LHS for `=`) when doing this check. As a result, a type with an
`init=` or `=` function with a different type RHS causes the error unless
both `=` and `init=` are added for the same type. This does not seem to
be an undue burden but could have an impact on determining which types
are POD. I recommend that in the future types can opt in to being
considered POD (in which case `=` and `init=` will be resolved, and
compilerErrors in them issued, but can be removed during optimization).
The generalized attribute syntax #14141 would be a reasonable way to do
that. Even before this PR, the `range` type needed to opt in to being POD
because its `init=` included various checks.

Next, this PR adjusted the compiler to also check for a mix of uses of
compiler-generated `init=` and custom `init=`; or compiler-generated `=`
and custom `=`. This only required adjusting 2 additional tests beyond
the tests adjusted for the above rule. In all, this PR required adjusting
~30 tests / modules and almost all of these were tests of record
initializers/assignment.

Reviewed by @lydia-duncan - thanks! Thanks to many others for discussion
about the language design direction.

- [x] full local futures testing

compiler/include/flags_list.h

@@ -388,6 +388,10 @@ symbolFlag( FLAG_TYPE_INIT_EQUAL_MISSING , npr, "type has no init=", "type has n
 symbolFlag( FLAG_TYPE_DEFAULT_VALUE , npr, "type has default value" , "type has a default value" )
 symbolFlag( FLAG_TYPE_NO_DEFAULT_VALUE , npr, "type has no default value" , "type has no default value" )
 
+symbolFlag( FLAG_TYPE_DEFAULT_INIT_EQUAL , npr, "type uses default init=" , "type uses compiler-generated default init=" )
+symbolFlag( FLAG_TYPE_CUSTOM_INIT_EQUAL , npr, "type uses custom init=" , "type has user-provided custom init=" )
+symbolFlag( FLAG_TYPE_DEFAULT_ASSIGN , npr, "type uses default =" , "type uses compiler-generated default =" )
+symbolFlag( FLAG_TYPE_CUSTOM_ASSIGN , npr, "type uses custom =" , "type has user-provided custom =" )
 
 symbolFlag( FLAG_TYPE_VARIABLE , npr, "type variable" , "contains a type instead of a value" )
 symbolFlag( FLAG_UNALIAS_FN,  ypr, "unalias fn" , "function to copy array slices when assigning to a user variable")

compiler/main/checks.cpp

@@ -60,6 +60,7 @@ static void checkLowerIteratorsRemovedPrims();
 static void checkFlagRelationships(); // Checks expected relationships between
                                       // flags.
 static void checkAutoCopyMap();
+static void checkDefaultInitEqAndAssign();
 static void checkFormalActualBaseTypesMatch();
 static void checkRetTypeMatchesRetVarType();
 static void checkFormalActualTypesMatch();
@@ -148,6 +149,7 @@ void check_resolve()
   check_afterNormalization();
   checkReturnTypesHaveRefTypes();
   checkAutoCopyMap();
+  checkDefaultInitEqAndAssign();
 }
 
 void check_resolveIntents()
@@ -471,6 +473,7 @@ static void check_afterResolution()
     checkFormalActualBaseTypesMatch();
     checkRetTypeMatchesRetVarType();
     checkAutoCopyMap();
+    checkDefaultInitEqAndAssign();
   }
 }
 
@@ -700,6 +703,86 @@ checkAutoCopyMap()
   }
 }
 
+static FnSymbol* findUserInitEq(AggregateType* at) {
+  for_alive_in_Vec(FnSymbol, fn, gFnSymbols) {
+    if (fn->name == astrInitEquals &&
+        !fn->hasFlag(FLAG_COMPILER_GENERATED) &&
+        fn->numFormals() >= 1) {
+      ArgSymbol* lhs = fn->getFormal(1);
+      Type* t = lhs->getValType();
+      if (t == at)
+        return fn;
+    }
+  }
+  return NULL;
+}
+
+static FnSymbol* findUserAssign(AggregateType* at) {
+
+  for_alive_in_Vec(FnSymbol, fn, gFnSymbols) {
+    if (fn->name == astrSassign &&
+        !fn->hasFlag(FLAG_COMPILER_GENERATED) &&
+        fn->numFormals() >= 1) {
+      ArgSymbol* lhs = fn->getFormal(1);
+      Type* t = lhs->getValType();
+      if (t == at)
+        return fn;
+    }
+  }
+  return NULL;
+}
+
+
+static void
+checkDefaultInitEqAndAssign()
+{
+  for_alive_in_Vec(TypeSymbol, ts, gTypeSymbols) {
+    if (ts->hasFlag(FLAG_EXTERN))
+      continue; // can't make init= for extern types today anyway
+    if (!isRecord(ts->type) && !isUnion(ts->type))
+      continue; // can't make init= for non-records non-unions today anyway
+
+    AggregateType* at = toAggregateType(ts->type);
+    bool defaultInitEq = ts->hasFlag(FLAG_TYPE_DEFAULT_INIT_EQUAL);
+    bool customInitEq = ts->hasFlag(FLAG_TYPE_CUSTOM_INIT_EQUAL);
+    bool defaultAssign = ts->hasFlag(FLAG_TYPE_DEFAULT_ASSIGN);
+    bool customAssign = ts->hasFlag(FLAG_TYPE_CUSTOM_ASSIGN);
+    if (defaultInitEq && customAssign) {
+      USR_FATAL_CONT(ts, "Type '%s' uses compiler-generated default 'init=' "
+                         "but has a custom '=' function. "
+                         "Please add an 'init=' method",
+                         toString(ts->type));
+      if (FnSymbol* userAssign = findUserAssign(at))
+        USR_PRINT(userAssign, "'=' for '%s' defined here", toString(ts->type));
+    }
+    if (defaultInitEq && customInitEq) {
+      USR_FATAL_CONT(ts, "Type '%s' uses compiler-generated default 'init=' "
+                         "but also has a custom 'init=' method. "
+                         "Please add an 'init=' method with the same RHS type",
+                         toString(ts->type));
+      if (FnSymbol* userInitEq = findUserInitEq(at))
+        USR_PRINT(userInitEq, "'init=' for '%s' defined here", toString(ts->type));
+    }
+
+    if (customInitEq && defaultAssign) {
+      USR_FATAL_CONT(ts, "Type '%s' uses compiler-generated default '=' "
+                         "but has a custom 'init=' method. "
+                         "Please add a '=' function.",
+                         toString(ts->type));
+      if (FnSymbol* userInitEq = findUserInitEq(at))
+        USR_PRINT(userInitEq, "'init=' for '%s' defined here", toString(ts->type));
+    }
+    if (defaultAssign && customAssign) {
+      USR_FATAL_CONT(ts, "Type '%s' uses compiler-generated default '=' "
+                         "but also has a custom '=' function. "
+                         "Please add a '=' function with the same RHS type.",
+                         toString(ts->type));
+      if (FnSymbol* userAssign = findUserAssign(at))
+        USR_PRINT(userAssign, "'=' for '%s' defined here", toString(ts->type));
+    }
+  }
+}
+
 
 // TODO: Can this be merged with checkFormalActualTypesMatch()?
 static void

compiler/resolution/resolveFunction.cpp

@@ -466,6 +466,8 @@ void resolveSpecifiedReturnType(FnSymbol* fn) {
 *                                                                             *
 ************************************** | *************************************/
 
+static void markTypesWithDefaultInitEqOrAssign(FnSymbol* fn);
+
 void resolveFunction(FnSymbol* fn, CallExpr* forCall) {
   if (fn->isResolved() == false) {
     if (fn->id == breakOnResolveID) {
@@ -520,11 +522,36 @@ void resolveFunction(FnSymbol* fn, CallExpr* forCall) {
       if (forCall != NULL) {
         resolveAlsoParallelIterators(fn, forCall);
       }
+
+      markTypesWithDefaultInitEqOrAssign(fn);
     }
     popInstantiationLimit(fn);
   }
 }
 
+static void markTypesWithDefaultInitEqOrAssign(FnSymbol* fn) {
+
+  if (fn->name == astrSassign &&
+      fn->numFormals() >= 1) {
+    ArgSymbol* lhs = fn->getFormal(1);
+    Type* t = lhs->getValType();
+    if (fn->hasFlag(FLAG_COMPILER_GENERATED))
+      t->symbol->addFlag(FLAG_TYPE_DEFAULT_ASSIGN);
+    else
+      t->symbol->addFlag(FLAG_TYPE_CUSTOM_ASSIGN);
+  }
+
+  if (fn->name == astrInitEquals &&
+      fn->numFormals() >= 2) {
+    ArgSymbol* lhs = fn->getFormal(2); // 1 is mt
+    Type* t = lhs->getValType();
+    if (fn->hasFlag(FLAG_COMPILER_GENERATED))
+      t->symbol->addFlag(FLAG_TYPE_DEFAULT_INIT_EQUAL);
+    else
+      t->symbol->addFlag(FLAG_TYPE_CUSTOM_INIT_EQUAL);
+  }
+}
+
 /************************************* | **************************************
 *                                                                             *
 *                                                                             *

modules/internal/ChapelRange.chpl

@@ -1121,28 +1121,6 @@ proc _cast(type t, r: range(?)) where isRangeType(t) {
   //#
 
   // Assignment
-  pragma "compiler generated"
-    // The "compiler generated" flag is added so this explicit definition
-    // of assignment does not disable the POD optimization.
-    // Although provided explicitly, this function is effectively trivial,
-    // since it performs what is effectively a bit-wise copy.
-    //
-    // The POD optimization currently removes initCopy, autoCopy and destructor
-    // calls whose arguments are of plain-old-data type.  Future applications
-    // of this optimization may also remove assignment calls.
-    // The determination of whether a type is POD or not is currently based on
-    // whether a destructor or any assignment operators are defined taking that
-    // that type as an operand.  In the future, the initCopy and default
-    // constructor (yet to be defined) functions and possibly the autoCopy
-    // function will be considered as well.
-    // The purpose of considering POD-ness as the criterion for removing
-    // initCopy and autoCopy calls is that destructors are removed at the same
-    // time.  So at least both functions participating in the optimization must
-    // be trivial.  In the future, the optimization may also remove assignments
-    // and default constructor calls, in which case the optimization should
-    // only be applied when all four of these functions (or their generic
-    // equivalents) are trivial.
-    // See also removePODinitDestroy() in removeUnnecessaryAutoCopyCalls.cpp.
   inline proc =(ref r1: range(stridable=?s1), r2: range(stridable=?s2))
   {
     if r1.boundedType != r2.boundedType then
@@ -1752,7 +1730,7 @@ proc _cast(type t, r: range(?)) where isRangeType(t) {
     const r = low..high by stride;
     for i in r do yield i;
   }
-  
+
 
 
   // cases for when stride is a param int (underlying iter can figure out sign

modules/internal/String.chpl

@@ -215,6 +215,7 @@ module String {
        }
 
    */
+  pragma "plain old data"
   record byteIndex {
     pragma "no doc"
     var _bindex  : int;
@@ -224,6 +225,7 @@ module String {
       // Let compiler insert defaults
     }
     proc init(i: int) { _bindex = i; }
+    proc init=(other: byteIndex) { _bindex = other._bindex; }
     proc init=(i: int) { _bindex = i; }
 
     proc writeThis(f) throws {
@@ -254,6 +256,7 @@ module String {
        }
 
    */
+  pragma "plain old data"
   record codepointIndex {
     pragma "no doc"
     var _cpindex  : int;
@@ -2020,13 +2023,22 @@ module String {
   proc =(ref lhs: byteIndex, rhs: int) {
     lhs._bindex = rhs: int;
   }
+  pragma "no doc"
+  proc =(ref lhs: byteIndex, const ref rhs: byteIndex) {
+    lhs._bindex = rhs._bindex;
+  }
 
   /*
      Copies the int `rhs` into the codepointIndex `lhs`.
   */
   proc =(ref lhs: codepointIndex, rhs: int) {
     lhs._cpindex = rhs: int;
   }
+  pragma "no doc"
+  proc =(ref lhs: codepointIndex, const ref rhs: codepointIndex) {
+    lhs._cpindex = rhs._cpindex;
+  }
+
 
   /*
      Copies the string `rhs` into the string `lhs`.

modules/packages/AtomicObjects.chpl

@@ -381,6 +381,10 @@ prototype module AtomicObjects {
 
     forwarding this.getObject()!;
   }
+  proc =(ref lhs: ABA, const ref rhs: lhs.type) {
+    lhs.__ABA_ptr = rhs.__ABA_ptr;
+    lhs.__ABA_cnt = rhs.__ABA_cnt;
+  }
 
   pragma "no doc"
   record _ABAInternal {

test/classes/bradc/compilerErrorInMethod/testClear-snap1.chpl

@@ -4,6 +4,10 @@ record Wrap {
   var _value;
 }
 
+proc Wrap.init=(other: Wrap) {
+  this._value = other._value;
+}
+
 proc =(ref lhs:Wrap, rhs) {
   lhs._value.clearHelp();
 }

test/classes/bradc/compilerErrorInMethod/testClear-snap2.chpl

@@ -4,6 +4,10 @@ record Wrap {
   var _value;
 }
 
+proc Wrap.init=(other: Wrap) {
+  this._value = other._value;
+}
+
 proc =(ref lhs:Wrap, rhs) {
   lhs._value.clearHelp();
 }

test/classes/bradc/compilerErrorInMethod/testClear-workaround.chpl

@@ -4,6 +4,10 @@ record Wrap {
   var _value;
 }
 
+proc Wrap.init=(other: Wrap) {
+  this._value = other._value;
+}
+
 proc =(ref lhs:Wrap, rhs) {
   lhs._value.clearHelp();
 }

test/classes/bradc/compilerErrorInMethod/testClear.bad

@@ -1,2 +1,2 @@
-testClear.chpl:22: In function 'clear':
-testClear.chpl:23: error: Can't clear a dense domain
+testClear.chpl:26: In function 'clear':
+testClear.chpl:27: error: Can't clear a dense domain

test/classes/bradc/compilerErrorInMethod/testClear.chpl

@@ -4,6 +4,10 @@ record Wrap {
   var _value;
 }
 
+proc Wrap.init=(ref other: Wrap) {
+  this._value = other._value;
+}
+
 proc =(ref lhs:Wrap, rhs) {
   lhs._value.clearHelp();
 }

test/classes/bradc/compilerErrorInMethod/testClear2.chpl

@@ -4,6 +4,10 @@ record Wrap {
   var _value;
 }
 
+proc Wrap.init=(other: Wrap) {
+  this._value = other._value;
+}
+
 proc =(ref lhs:Wrap, rhs) {
   lhs._value.clearHelp();
 }

test/classes/delete-free/lifetimes/record-owns.chpl

@@ -33,6 +33,9 @@ record RMyClass {
   proc init(in c:owned MyClass) {
     this.c = c;
   }
+  proc init=(ref other: RMyClass) {
+    this.c = other.c;
+  }
 }
 
 proc =(ref lhs:RMyClass, ref rhs:RMyClass) {

test/classes/hilde/nestedRecordAssignment.chpl

@@ -6,12 +6,22 @@
 // left-hand side of the assignment is overwritten with the value in the
 // like-named field of the record on the right-hand side as if by assignment."
 //
-record R { var i:int;
+record R {
+  var i:int;
 }
-proc =(ref lhs:R, rhs:R)
-{ lhs.i = rhs.i; writeln("overwrote lhs.i with ", rhs.i); }
 
-record S { var r:R; }
+proc R.init=(other:R) {
+  this.i = other.i;
+}
+
+proc =(ref lhs:R, rhs:R) {
+  lhs.i = rhs.i;
+  writeln("overwrote lhs.i with ", rhs.i);
+}
+
+record S {
+  var r:R;
+}
 
 var s,t:S;
 t.r.i = 121;

test/classes/hilde/records/assignReturns-error.chpl

@@ -6,6 +6,10 @@ record R {
   var x: int;
 }
 
+proc R.init=(const ref other:R) {
+  this.x = other.x;
+}
+
 proc =(ref lhs:R, rhs: R) {
   lhs.x = rhs.x + 1;
   return new R();

test/classes/hilde/records/assignReturns-error.good

@@ -1 +1 @@
-assignReturns-error.chpl:9: error: The return value of an assignment operator must be 'void'.
+assignReturns-error.chpl:13: error: The return value of an assignment operator must be 'void'.

test/classes/hilde/records/assignReturns-error2.chpl

@@ -6,6 +6,10 @@ record R {
   var x: int;
 }
 
+proc R.init=(const ref other:R) {
+  this.x = other.x;
+}
+
 proc =(ref lhs:R, rhs: R) {
   lhs.x = rhs.x + 1;
   return true;