Nilability checking improvements #2 (chapel-lang#13677) by vass

* Implement some previously-missing checks.
* Improve the error messages in some cases.
* Minor fixes in two existing tests.
* Add testing of all combinations of nilability and memory management
  of lhs and rhs when initializing a variable this way:

        var lhs: <lhs type> = rhs;

The tests are added under:

    test/classes/errors/nilability-init-var/

Some improvements are implemented as additional `init=` methods or
otherwise in the modules. Other improvements are from compiler changes.

In some cases, the following code:

    var lhs: <lhs type> = rhs;
    compilerError("something");

results in compilerError() firing, whereas without that call
the compiler would report an error on the `var lhs` line.
This is because the `var lhs` error would be reported in
`insertAndResolveCasts(fn)`, which is called after
`resolveBlockStmt(fn->body)` completes successfully.

Some of the compiler changes in this PR make the compiler still report
the `var lhs` error even in the presence of a subsequent compilerError().

The all-combinations tests were created by running this script:

    classes/errors/nilability-generate.chpl

then manually adjusting two files to comment out compilerError(), see below.
I set to test them with --no-codegen, to save testing time. This is a
consideration because 28 of them compile without errors.

FUTURE WORK

* Remove the error upon `var lhs: <shared?> = <owned!>`, which is legal.
  See `init-var-oknil-shared-from-nonnil-owned.future`

* The following cases would still fire compilerError() despite an error
  in the preceding `var lhs` line. I commented out compilerError() in those
  tests to trigger the desired error messages. We want to see those messages
  ahead of compilerError().

    init-var-nonnil-borrowed-from-oknil-nil.chpl
    init-var-nonnil-unmanaged-from-oknil-nil.chpl

* In chapel-lang#11118 we preferred the error message pattern `cannot assign to <lhs>  from <rhs>`.
  There are a few places in the modules and in the compiler that
  either switch the order or say `assign <lhs> to <rhs>`.
  It would be nice to make those consistent with chapel-lang#11118.

Reviewed by / with suggestions from:  @mppf, @lydia-duncan

compiler/resolution/functionResolution.cpp

@@ -5616,6 +5616,41 @@ static void resolveInitField(CallExpr* call) {
   resolveSetMember(call); // Can we remove some of the above with this?
 }
 
+// Should this be public, like isNilableClassType() ?
+static bool isUnmanagedClass(Type* t) {
+  if (DecoratedClassType* dt = toDecoratedClassType(t))
+    if (dt->isUnmanaged())
+      return true;
+  return false;
+}
+
+// Todo: ideally this would be simply something like:
+//   isChapelManagedType(t) || isChapelBorrowedType(t)
+static bool isOwnedOrSharedOrBorrowed(Type* t) {
+  if (isClass(t))
+    return true; // borrowed, non-nilable
+
+  if (DecoratedClassType* dt = toDecoratedClassType(t))
+    if (! dt->isUnmanaged())
+      return true; // anything not unmanaged
+
+  if (isManagedPtrType(t))
+    return true; // owned or shared
+
+  return false;
+}
+
+static void checkMoveIntoClass(CallExpr* call, Type* lhs, Type* rhs) {
+  if (! fLegacyNilableClasses &&
+      isNonNilableClassType(lhs) && isNilableClassType(rhs))
+    USR_FATAL(call, "cannot assign to non-nilable %s from nilable %s",
+              toString(lhs), toString(rhs));
+
+  if (isUnmanagedClass(lhs) && isOwnedOrSharedOrBorrowed(rhs))
+    USR_FATAL(call, "cannot assign to %s from %s",
+              toString(lhs), toString(rhs));
+}
+
 /************************************* | **************************************
 *                                                                             *
 * This handles expressions of the form                                        *
@@ -5687,6 +5722,10 @@ static void resolveInitVar(CallExpr* call) {
         isSingleType(src->getValType()) ||
         (targetType->symbol->hasFlag(FLAG_TUPLE) && mismatch) ||
         (isRecord(targetType->getValType()) == false && mismatch)) {
+
+      if (mismatch)
+        checkMoveIntoClass(call, targetType->getValType(), src->getValType());
+
       VarSymbol* tmp = newTemp(primCoerceTmpName, targetType);
       tmp->addFlag(FLAG_EXPR_TEMP);
 

modules/internal/OwnedObject.chpl

@@ -245,16 +245,37 @@ module OwnedObject {
        refer to `nil` after this call.
      */
     proc init=(pragma "leaves arg nil" pragma "nil from arg" ref src:_owned) {
+      if isNonNilableClass(this.type) && isNilableClass(src) &&
+         !chpl_legacyNilClasses
+      then
+        compilerError("cannot create a non-nilable owned variable from a nilable class instance");
+
       // Use 'this.type.chpl_t' in case RHS is a subtype
       this.chpl_t = this.type.chpl_t;
       this.chpl_p = src.release();
+      this.complete();
+    }
+
+    proc init=(src: shared) {
+      compilerError("cannot create an owned variable from a shared class instance");
+      this.chpl_t = int; //dummy
+    }
+
+    proc init=(src: borrowed) {
+      compilerError("cannot create an owned variable from a borrowed class instance");
+      this.chpl_t = int; //dummy
+    }
+
+    proc init=(src: unmanaged) {
+      compilerError("cannot create an owned variable from an unmanaged class instance");
+      this.chpl_t = int; //dummy
     }
 
     pragma "no doc"
     proc init=(src : _nilType) {
       this.init(this.type.chpl_t);
 
-      if _to_nilable(chpl_t) != chpl_t && !chpl_legacyNilClasses {
+      if isNonNilableClass(chpl_t) && !chpl_legacyNilClasses {
         compilerError("Assigning non-nilable owned to nil");
       }
     }

modules/internal/SharedObject.chpl

@@ -274,6 +274,11 @@ module SharedObject {
     //   var s : shared = ownedThing;
     pragma "no doc"
     proc init=(pragma "nil from arg" in take: owned) {
+      if isNonNilableClass(this.type) && isNilableClass(take) &&
+         !chpl_legacyNilClasses
+      then
+        compilerError("cannot create a non-nilable shared variable from a nilable class instance");
+
       this.init(take);
     }
 
@@ -283,6 +288,11 @@ module SharedObject {
        These will share responsibility for managing the instance.
      */
     proc init=(pragma "nil from arg" const ref src:_shared(?)) {
+      if isNonNilableClass(this.type) && isNilableClass(src) &&
+         !chpl_legacyNilClasses
+      then
+        compilerError("cannot create a non-nilable shared variable from a nilable class instance");
+
       this.chpl_t = this.type.chpl_t;
       this.chpl_p = src.chpl_p;
       this.chpl_pn = src.chpl_pn;
@@ -293,6 +303,16 @@ module SharedObject {
         this.chpl_pn!.retain();
     }
 
+    proc init=(src: borrowed) {
+      compilerError("cannot create a shared variable from a borrowed class instance");
+      this.chpl_t = int; //dummy
+    }
+
+    proc init=(src: unmanaged) {
+      compilerError("cannot create a shared variable from an unmanaged class instance");
+      this.chpl_t = int; //dummy
+    }
+
     proc init=(src : _nilType) {
       this.init(this.type.chpl_t);
 

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

@@ -26,7 +26,7 @@ class MyClass {
 }
 
 record RMyClass {
-  var c:owned MyClass;
+  var c:owned MyClass?;
   proc init() {
     this.c = new owned(nil:unmanaged MyClass?);
   }

test/classes/errors/nilability-generate.chpl

@@ -0,0 +1,147 @@
+/*
+This script generates the various combinations of lhs and rhs class types
+for `lhs = rhs` in various contexts of initialization/assignment, fields/
+vars, etc.
+
+It is always an error upon --no-legacy-nilable-classes when
+`lhs` is non-nilable while `rhs` is nilable or `nil`.
+
+It is always OK when `lhs` is nilable and `rhs` is `nil`.
+
+Otherwise, `lhs` and `rhs` are OK for nilability. Then, an error is given
+for the combinations of memory management that are not "ok" in this table:
+
+    | lhs       | owned | shared | borrowed | unmanaged |
+    | :-------- | :---: | :----: | :------: | :-------: |
+    | owned     |  ok   |        |          |           |
+    | shared    |  ok   |  ok    |          |           |
+    | borrowed  |  ok   |  ok    |  ok      |  ok       |
+    | unmanaged |       |        |          |  ok       |
+*/
+
+use FileSystem;
+
+enum MM { mOwn, mShr, mBor, mUnm, mNil };
+use MM;
+
+const allM = mOwn..mUnm;
+const allMnil = mOwn..mNil;
+proc isNil(arg:MM) return arg==mNil;
+
+// Is 'lhs = rhs' OK w.r.t. memory management?
+const mmOK = makeMMOK();
+
+private proc makeMMOK() {
+  var result: [allM, allMnil] bool;
+  proc ok(lhs:MM, rhs:MM) { result[lhs,rhs] = true; }
+  proc er(lhs:MM, rhs:MM) { result[lhs,rhs] = false; } //no-op, really
+
+  // See the above table; plus a row for mNil.
+  ok(mOwn,mOwn);  er(mOwn,mShr);  er(mOwn,mBor);  er(mOwn,mUnm);  ok(mOwn,mNil);
+  ok(mShr,mOwn);  ok(mShr,mShr);  er(mShr,mBor);  er(mShr,mUnm);  ok(mShr,mNil);
+  ok(mBor,mOwn);  ok(mBor,mShr);  ok(mBor,mBor);  ok(mBor,mUnm);  ok(mBor,mNil);
+  er(mUnm,mOwn);  er(mUnm,mShr);  er(mUnm,mBor);  ok(mUnm,mUnm);  ok(mUnm,mNil);
+
+  return result;
+}
+
+/////////////////////////////////////////////////////////////////////////////
+
+proc MM.strm {
+  select this {
+    when mOwn do return "owned";
+    when mShr do return "shared";
+    when mBor do return "borrowed";
+    when mUnm do return "unmanaged";
+    when mNil do return "nil";
+  }
+  return "<invalid>";
+}
+
+proc bool.strn return if this then "oknil" else "nonnil";
+
+proc str(nlb:bool, mm:MM)  return mm.strm + if nlb then "?" else "!";
+
+proc errstr(errNlb:bool, errM:bool) return
+  if errNlb then
+    if errM then "errors: nlb, mm" else "error: nlb"
+  else
+    if errM then "error: mm" else "ok";
+
+/////////////////////////////////////////////////////////////////////////////
+
+proc checkConfig(lhsNlb: bool, lhsM: MM, rhsNlb: bool, rhsM: MM,
+                 skipIfNonNilableNil: bool)
+{
+  assert(lhsM != mNil);
+  if rhsM == mNil && ! rhsNlb {
+    if skipIfNonNilableNil then return true;
+    else halt("invalid combination");
+  }
+  return false;
+// another option: rhsNlb = rhsNlbArg | rhsM == nil;
+}
+
+config const ivDir = "nilability-init-var";
+
+proc cInitVar(lhsNlb: bool, lhsM: MM, rhsNlb: bool, rhsM: MM,
+              skipIfNNNil = false)
+{
+  if checkConfig(lhsNlb, lhsM, rhsNlb, rhsM, skipIfNNNil) then return;
+  const errNlb = !lhsNlb && rhsNlb;
+  const errM   = ! mmOK(lhsM, rhsM);
+  const errS   = errstr(errNlb, errM);
+
+  var path = "init-var-%s-%s-from-%s-%s".format(
+    lhsNlb.strn, lhsM.strm, rhsNlb.strn, rhsM.strm);
+  writeln(path);
+  if ! ivDir.isEmpty() {
+    mkdir(ivDir, parents=true); path = ivDir + "/" + path; }
+  var chan = open(path + ".chpl", iomode.cw).writer();
+  proc write(args...) { chan.writef((...args)); }
+
+  var rhsDef = "", rhsUse = "rhs";
+  if rhsM == mNil then rhsUse = "nil";
+  else if rhsNlb then rhsDef = 'var rhs: %s MyClass?;'.format(rhsM.strm);
+  else                rhsDef = 'var rhs = new %s MyClass();'.format(rhsM.strm);
+
+  var lhsQ = if lhsNlb then "?" else "";
+
+  var lastStmt = "";
+  if errNlb || errM then lastStmt = 'compilerError("done");';
+  else if rhsM == mUnm && !rhsNlb then lastStmt = 'delete rhs;';
+
+  write("//  lhs: %s  rhs: %s  %s\n",
+        str(lhsNlb, lhsM), str(rhsNlb, rhsM), errS);
+  write("""
+class MyClass {
+  var x: int;
+}
+
+%s
+
+var lhs: %s MyClass%s = %s;
+
+%s
+""", rhsDef, lhsM.strm, lhsQ, rhsUse, lastStmt);
+
+  // now, the .good file
+  var good = open(path + ".good", iomode.cw).writer();
+  if errNlb || errM then
+    good.writeln(errS);
+  // else leave it empty
+}
+
+writeln(); writeln();
+writeln("cInitVar: dir=", ivDir);
+writeln(); writeln();
+for lhs in allM {
+  for rhs in allMnil {
+    cInitVar(true, lhs, true, rhs);
+    cInitVar(true, lhs, false, rhs, true);
+    cInitVar(false, lhs, true, rhs);
+    cInitVar(false, lhs, false, rhs, true);
+    writeln();
+  }
+  writeln();
+}

test/classes/errors/nilability-generate.notest

@@ -0,0 +1 @@
+This is a script to generate tests.

test/classes/errors/nilability-init-var/COMPOPTS

@@ -0,0 +1 @@
+--no-codegen --no-legacy-nilable-classes

test/classes/errors/nilability-init-var/NOEXEC

@@ -0,0 +1 @@
+Save some C compilation time - do not execute these tests.

test/classes/errors/nilability-init-var/init-var-nonnil-borrowed-from-nonnil-borrowed.chpl

@@ -0,0 +1,11 @@
+//  lhs: borrowed!  rhs: borrowed!  ok
+
+class MyClass {
+  var x: int;
+}
+
+var rhs = new borrowed MyClass();
+
+var lhs: borrowed MyClass = rhs;
+
+

test/classes/errors/nilability-init-var/init-var-nonnil-borrowed-from-nonnil-owned.chpl

@@ -0,0 +1,11 @@
+//  lhs: borrowed!  rhs: owned!  ok
+
+class MyClass {
+  var x: int;
+}
+
+var rhs = new owned MyClass();
+
+var lhs: borrowed MyClass = rhs;
+
+

test/classes/errors/nilability-init-var/init-var-nonnil-borrowed-from-nonnil-shared.chpl

@@ -0,0 +1,11 @@
+//  lhs: borrowed!  rhs: shared!  ok
+
+class MyClass {
+  var x: int;
+}
+
+var rhs = new shared MyClass();
+
+var lhs: borrowed MyClass = rhs;
+
+

test/classes/errors/nilability-init-var/init-var-nonnil-borrowed-from-nonnil-unmanaged.chpl

@@ -0,0 +1,11 @@
+//  lhs: borrowed!  rhs: unmanaged!  ok
+
+class MyClass {
+  var x: int;
+}
+
+var rhs = new unmanaged MyClass();
+
+var lhs: borrowed MyClass = rhs;
+
+delete rhs;

test/classes/errors/nilability-init-var/init-var-nonnil-borrowed-from-oknil-borrowed.chpl

@@ -0,0 +1,11 @@
+//  lhs: borrowed!  rhs: borrowed?  error: nlb
+
+class MyClass {
+  var x: int;
+}
+
+var rhs: borrowed MyClass?;
+
+var lhs: borrowed MyClass = rhs;
+
+compilerError("done");

test/classes/errors/nilability-init-var/init-var-nonnil-borrowed-from-oknil-borrowed.good

@@ -0,0 +1 @@
+init-var-nonnil-borrowed-from-oknil-borrowed.chpl:9: error: cannot assign to non-nilable borrowed MyClass from nilable borrowed MyClass?

test/classes/errors/nilability-init-var/init-var-nonnil-borrowed-from-oknil-nil.chpl

@@ -0,0 +1,11 @@
+//  lhs: borrowed!  rhs: nil?  error: nlb
+
+class MyClass {
+  var x: int;
+}
+
+
+
+var lhs: borrowed MyClass = nil;
+
+//todo: uncomment this without changing .good:   compilerError("done");

test/classes/errors/nilability-init-var/init-var-nonnil-borrowed-from-oknil-nil.good

@@ -0,0 +1 @@
+init-var-nonnil-borrowed-from-oknil-nil.chpl:9: error: cannot assign to borrowed MyClass from nil

test/classes/errors/nilability-init-var/init-var-nonnil-borrowed-from-oknil-owned.chpl

@@ -0,0 +1,11 @@
+//  lhs: borrowed!  rhs: owned?  error: nlb
+
+class MyClass {
+  var x: int;
+}
+
+var rhs: owned MyClass?;
+
+var lhs: borrowed MyClass = rhs;
+
+compilerError("done");

test/classes/errors/nilability-init-var/init-var-nonnil-borrowed-from-oknil-owned.good

@@ -0,0 +1 @@
+init-var-nonnil-borrowed-from-oknil-owned.chpl:9: error: cannot assign to non-nilable borrowed MyClass from nilable owned MyClass?

test/classes/errors/nilability-init-var/init-var-nonnil-borrowed-from-oknil-shared.chpl

@@ -0,0 +1,11 @@
+//  lhs: borrowed!  rhs: shared?  error: nlb
+
+class MyClass {
+  var x: int;
+}
+
+var rhs: shared MyClass?;
+
+var lhs: borrowed MyClass = rhs;
+
+compilerError("done");

test/classes/errors/nilability-init-var/init-var-nonnil-borrowed-from-oknil-shared.good

@@ -0,0 +1 @@
+init-var-nonnil-borrowed-from-oknil-shared.chpl:9: error: cannot assign to non-nilable borrowed MyClass from nilable shared MyClass?