Merge pull request #17413 from dlongnecke-cray/public-17401

Fix bugs in set composition operators when `parSafe=true` (#17413)

The composition operators for `set` declare a result that has a
`parSafe` value of `(lhs.parSafe || rhs.parSafe)`.

Prior to this PR, the declared return type of these operators omitted
the value of `parSafe`, which caused it to default to false. This
led to compiler errors when a composition operator was used on a set
with `parSafe=true`.

Omit a declared return type for the set composition operators. The
return type is now correctly inferred.

Add a test covering all composition operators (|, +, -, &, ^) as well
as their compound assignment variants (|=, +=, -=, &=, ^=). The test
is repeated to cover all combinations of `parSafe` values for the LHS
and RHS sets.

Define a cast operator for the `set` type. Currently it requires both
sets to have the same element type. Loosening this restriction (e.g.
for a cast from set of subclass to set of superclass) is possible.
The cast operator was added to keep the compiler from complaining
about assignment between sets with different `parSafe` values. It is
not documented.

Resolves #17401.

Reviewed by @vasslitvinov and @mppf. Thanks!

TESTING:

- [x] `ALL` on `linux64` when `COMM=none`
- [x] `ALL` on `linux64` when `COMM=gasnet`
- [x] Checked updated docs locally

Signed-off-by: David Longnecker <[email protected]>

Author: dlongnecke-cray

modules/standard/Set.chpl

@@ -195,17 +195,26 @@ module Set {
 
       :arg other: A set to initialize this set with.
     */
-    proc init=(const ref other: set(?t, ?)) lifetime this < other {
-      this.eltType = t;
-      this.parSafe = other.parSafe;
+    proc init=(const ref other: set(?t, ?p)) lifetime this < other {
+      this.eltType = if this.type.eltType != ? then
+                        this.type.eltType else t;
+      this.parSafe = if this.type.parSafe != ? then
+                        this.type.parSafe else p;
       this.complete();
 
-      if !isCopyableType(eltType) then
-        compilerError('cannot initialize ' + this.type:string + ' from ' +
-                      other.type:string + ' because element type ' +
-                      eltType:string + ' is not copyable');
-
-      for elem in other do _addElem(elem);
+      // TODO: Relax this to allow if 'isCoercible(t, this.eltType)'?
+      if eltType != t {
+        compilerError('cannot initialize ', this.type:string, ' from ',
+                      other.type:string, ' due to element type ',
+                      'mismatch');
+      } else if !isCopyableType(eltType) {
+        compilerError('cannot initialize ', this.type:string, ' from ',
+                      other.type:string, ' because element type ',
+                      eltType:string, ' is not copyable');
+      } else {
+        // TODO: Use a forall when this.parSafe?
+        for elem in other do _addElem(elem);
+      }
     }
 
     pragma "no doc"
@@ -539,11 +548,14 @@ module Set {
     :arg b: A set to take the union of.
 
     :return: A new set containing the union between `a` and `b`.
-    :rtype: `set(?t, ?)`
   */
-  proc |(const ref a: set(?t, ?), const ref b: set(t, ?)): set(t) {
+  proc |(const ref a: set(?t, ?), const ref b: set(t, ?)) {
     var result: set(t, (a.parSafe || b.parSafe));
 
+    // TODO: Split-init causes weird errors, remove this line and then run
+    // setCompositionParSafe.chpl to see.
+    result;
+
     result = a;
     result |= b;
 
@@ -569,9 +581,8 @@ module Set {
     :arg b: A set to take the union of.
 
     :return: A new set containing the union between `a` and `b`.
-    :rtype: `set(?t, ?)`
   */
-  proc +(const ref a: set(?t, ?), const ref b: set(t, ?)): set(t, ?) {
+  proc +(const ref a: set(?t, ?), const ref b: set(t, ?)) {
     return a | b;
   }
 
@@ -592,13 +603,12 @@ module Set {
     :arg b: A set to take the difference of.
 
     :return: A new set containing the difference between `a` and `b`.
-    :rtype: `set(t)`
   */
-  proc -(const ref a: set(?t, ?), const ref b: set(t, ?)): set(t) {
+  proc -(const ref a: set(?t, ?), const ref b: set(t, ?)) {
     var result = new set(t, (a.parSafe || b.parSafe));
 
     if a.parSafe && b.parSafe {
-      forall x in a do
+      forall x in a with (ref result) do
         if !b.contains(x) then
           result.add(x);
     } else {
@@ -623,7 +633,7 @@ module Set {
   */
   proc -=(ref lhs: set(?t, ?), const ref rhs: set(t, ?)) {
     if lhs.parSafe && rhs.parSafe {
-      forall x in rhs do
+      forall x in rhs with (ref lhs) do
         lhs.remove(x);
     } else {
       for x in rhs do
@@ -638,15 +648,14 @@ module Set {
     :arg b: A set to take the intersection of.
 
     :return: A new set containing the intersection of `a` and `b`.
-    :rtype: `set(t)`
   */
-  proc &(const ref a: set(?t, ?), const ref b: set(t, ?)): set(t) {
+  proc &(const ref a: set(?t, ?), const ref b: set(t, ?)) {
     var result: set(t, (a.parSafe || b.parSafe));
 
     /* Iterate over the smaller set */
     if a.size <= b.size {
       if a.parSafe && b.parSafe {
-        forall x in a do
+        forall x in a with (ref result) do
           if b.contains(x) then
             result.add(x);
       } else {
@@ -656,7 +665,7 @@ module Set {
       }
     } else {
       if a.parSafe && b.parSafe {
-        forall x in b do
+        forall x in b with (ref result) do
           if a.contains(x) then
             result.add(x);
       } else {
@@ -687,14 +696,15 @@ module Set {
     var result: set(t, (lhs.parSafe || rhs.parSafe));
 
     if lhs.parSafe && rhs.parSafe {
-      forall x in lhs do
+      forall x in lhs with (ref result) do
         if rhs.contains(x) then
           result.add(x);
     } else {
       for x in lhs do
         if rhs.contains(x) then
           result.add(x);
     }
+
     lhs = result;
   }
 
@@ -705,11 +715,14 @@ module Set {
     :arg b: A set to take the symmetric difference of.
 
     :return: A new set containing the symmetric difference of `a` and `b`.
-    :rtype: `set(?t, ?)`
   */
-  proc ^(const ref a: set(?t, ?), const ref b: set(t, ?)): set(t) {
+  proc ^(const ref a: set(?t, ?), const ref b: set(t, ?)) {
     var result: set(t, (a.parSafe || b.parSafe));
 
+    // TODO: Split-init causes weird errors, remove this line and then run
+    // setCompositionParSafe.chpl to see.
+    result;
+
     /* Expect the loop in ^= to be more expensive than the loop in =,
        so arrange for the rhs of the ^= to be the smaller set. */
     if a.size <= b.size {
@@ -737,7 +750,7 @@ module Set {
   */
   proc ^=(ref lhs: set(?t, ?), const ref rhs: set(t, ?)) {
     if lhs.parSafe && rhs.parSafe {
-      forall x in rhs {
+      forall x in rhs with (ref lhs) {
         if lhs.contains(x) {
           lhs.remove(x);
         } else {
@@ -784,6 +797,17 @@ module Set {
     return result;
   }
 
+  pragma "no doc"
+  operator :(x: set(?et1, ?p1), type t: set(?et2, ?p2)) {
+    // TODO: Allow coercion between element types? If we do then init=
+    // should also be changed accordingly.
+    if et1 != et2 then
+      compilerError('Cannot cast to set with different ',
+                    'element type: ', t:string);
+    var result: set(et1, p2) = x;
+    return result;
+  }
+
   /*
     Return `true` if the sets `a` and `b` are not equal.
 

test/library/standard/Set/setCompositionParSafe.chpl

@@ -0,0 +1,102 @@
+use Set;
+
+config const lo = 1;
+config const hi = 8;
+
+enum LockMode { neither, left, right, both }
+
+proc addToSets(ref a, ref b) {
+  for i in lo..hi {
+    if !(i % 2) {
+      a.add(i);
+    } else {
+      b.add(i);
+    }
+  }
+}
+
+proc populateSets(param mode: LockMode) where mode == LockMode.left {
+  var lhs: set(int, parSafe=true);
+  var rhs: set(int);
+  addToSets(lhs, rhs);
+  return (lhs, rhs);
+}
+
+proc populateSets(param mode: LockMode) where mode == LockMode.right {
+  var lhs: set(int);
+  var rhs: set(int, parSafe=true);
+  addToSets(lhs, rhs);
+  return (lhs, rhs);
+}
+
+proc populateSets(param mode: LockMode) where mode == LockMode.both {
+  var lhs, rhs: set(int, parSafe=true);
+  addToSets(lhs, rhs);
+  return (lhs, rhs);
+}
+
+proc populateSets(param mode: LockMode) where mode == LockMode.neither {
+  var lhs, rhs: set(int);
+  addToSets(lhs, rhs);
+  return (lhs, rhs);
+}
+
+proc display(op, s) {
+  param space = ' ';
+  write(op, space, ':', space);
+  for i in lo..hi do
+    if s.contains(i) then
+      write(i, space);
+  writeln();
+}
+
+proc doTest(param mode, op) {
+  var (a, b) = populateSets(mode);
+  const opEqual = op + '=';
+  select op {
+    when '|' { var c = a | b; display(op, c); a |= b; display(opEqual, a); }
+    when '+' { var c = a + b; display(op, c); a += b; display(opEqual, a); }
+    when '-' { var c = a - b; display(op, c); a -= b; display(opEqual, a); }
+    when '&' { var c = a & b; display(op, c); a &= b; display(opEqual, a); }
+    when '^' { var c = a ^ b; display(op, c); a ^= b; display(opEqual, a); }
+    otherwise { halt('Unrecognized op: ' + op:string); }
+  }
+}
+
+proc testSetCompositionOps(param mode) {
+  writeln('Which set is \'parSafe=true\': ', mode);
+  doTest(mode, '|');
+  doTest(mode, '+');
+  doTest(mode, '-');
+  doTest(mode, '&');
+  doTest(mode, '^');
+}
+
+proc test1() {
+  writeln('T1');
+  testSetCompositionOps(LockMode.neither);
+  writeln();
+}
+test1();
+
+proc test2() {
+  writeln('T2');
+  testSetCompositionOps(LockMode.left);
+  writeln();
+}
+test2();
+
+proc test3() {
+  writeln('T3');
+  testSetCompositionOps(LockMode.right);
+  writeln();
+}
+test3();
+
+proc test4() {
+  writeln('T4');
+  testSetCompositionOps(LockMode.both);
+  writeln();
+}
+test4();
+

test/library/standard/Set/setCompositionParSafe.good

@@ -0,0 +1,52 @@
+T1
+Which set is 'parSafe=true': neither
+| : 1 2 3 4 5 6 7 8 
+|= : 1 2 3 4 5 6 7 8 
++ : 1 2 3 4 5 6 7 8 
++= : 1 2 3 4 5 6 7 8 
+- : 2 4 6 8 
+-= : 2 4 6 8 
+& : 
+&= : 
+^ : 1 2 3 4 5 6 7 8 
+^= : 1 2 3 4 5 6 7 8 
+
+T2
+Which set is 'parSafe=true': left
+| : 1 2 3 4 5 6 7 8 
+|= : 1 2 3 4 5 6 7 8 
++ : 1 2 3 4 5 6 7 8 
++= : 1 2 3 4 5 6 7 8 
+- : 2 4 6 8 
+-= : 2 4 6 8 
+& : 
+&= : 
+^ : 1 2 3 4 5 6 7 8 
+^= : 1 2 3 4 5 6 7 8 
+
+T3
+Which set is 'parSafe=true': right
+| : 1 2 3 4 5 6 7 8 
+|= : 1 2 3 4 5 6 7 8 
++ : 1 2 3 4 5 6 7 8 
++= : 1 2 3 4 5 6 7 8 
+- : 2 4 6 8 
+-= : 2 4 6 8 
+& : 
+&= : 
+^ : 1 2 3 4 5 6 7 8 
+^= : 1 2 3 4 5 6 7 8 
+
+T4
+Which set is 'parSafe=true': both
+| : 1 2 3 4 5 6 7 8 
+|= : 1 2 3 4 5 6 7 8 
++ : 1 2 3 4 5 6 7 8 
++= : 1 2 3 4 5 6 7 8 
+- : 2 4 6 8 
+-= : 2 4 6 8 
+& : 
+&= : 
+^ : 1 2 3 4 5 6 7 8 
+^= : 1 2 3 4 5 6 7 8 
+

test/library/standard/Set/setInitTypeMismatchError.chpl

@@ -0,0 +1,19 @@
+use Set;
+
+class C1 { var x = 0; }
+class C2 : C1 { var y = 0; }
+
+proc test1() {
+  var s1: set(shared C1);
+  for i in 0..3 do s1.add(new shared C1(i));
+  var s2: set(shared C2) = s1;
+}
+test1();
+
+proc test2() {
+  var s1: set(int);
+  for i in 0..3 do s1.add(i);
+  var s2: set(real) = s1;
+}
+test2();
+

test/library/standard/Set/setInitTypeMismatchError.good

@@ -0,0 +1,6 @@
+setInitTypeMismatchError.chpl:6: In function 'test1':
+setInitTypeMismatchError.chpl:9: error: invalid copy-initialization
+setInitTypeMismatchError.chpl:9: error: cannot initialize set(shared C2,false) from set(shared C1,false) due to element type mismatch
+setInitTypeMismatchError.chpl:13: In function 'test2':
+setInitTypeMismatchError.chpl:16: error: invalid copy-initialization
+setInitTypeMismatchError.chpl:16: error: cannot initialize set(real(64),false) from set(int(64),false) due to element type mismatch

test/library/standard/Set/setSplitInit.chpl

@@ -0,0 +1,11 @@
+use Set;
+
+proc test1() {
+  var a: set(int);
+  var b: set(int, parSafe=true);
+  var c: set(int, (a.parSafe || b.parSafe));
+
+  c = a;
+}
+test1();
+