Improve errors for missue of Sort with arrays and domains (#25863)

Improves the errors thrown by the Sort module (and .sorted() method) when it is used incorrectly by passing unsupported array.domain types. This PR also adds a warning to `assocDomain.sorted()`, but it does not deprecate it to maintain backwards compatibility. - [x] tested with a full paratest with/without comm - [x] checked that docs build and render properly Resolves #25109 [Reviewed by @ShreyasKhandekar]
chapel-lang · Sep 4, 2024 · 382fdc9 · 382fdc9
2 parents e732ba1 + 33040d3
commit 382fdc9
Show file tree

Hide file tree

Showing 75 changed files with 340 additions and 143 deletions.
diff --git a/modules/internal/ChapelDomain.chpl b/modules/internal/ChapelDomain.chpl
@@ -46,6 +46,9 @@ module ChapelDomain {
   @chpldoc.nodoc
   config param noNegativeStrideWarnings = false;
 
+  @chpldoc.nodoc
+  config param noSortedWarnings = false;
+
   pragma "no copy return"
   pragma "return not owned"
   proc _getDomain(value) {
@@ -2791,11 +2794,23 @@ module ChapelDomain {
     }
 
     // associative array interface
-    /* Yields the domain indices in sorted order. */
+    /*
+      Yields the domain indices in sorted order. This method is only supported
+      on associative domains.
+
+      .. warning::
+
+         It is recommended to use :proc:`Sort.sorted` instead of this method.
+
+    */
     iter sorted(comparator:?t = chpl_defaultComparator()) {
-      for i in _value.dsiSorted(comparator) {
-        yield i;
-      }
+      if !this.isAssociative() then
+        compilerError("'.sorted()' is only supported on associative domains");
+      if !noSortedWarnings then
+        compilerWarning(
+          "It is recommended to use 'Sort.sorted' instead of this method. ",
+          "Compile with '-snoSortedWarnings' to suppress this warning.");
+      for x in this._value.dsiSorted(comparator) do yield x;
     }
 
     @chpldoc.nodoc

diff --git a/modules/packages/ReplicatedVar.chpl b/modules/packages/ReplicatedVar.chpl
@@ -146,7 +146,8 @@ proc rcCollect(replicatedVar: [?D] ?MYTYPE, ref collected: [?CD] MYTYPE): void
   var targetLocales = _rcTargetLocalesHelper(replicatedVar);
   assert(replicatedVar.domain == rcDomainBase);
   for idx in collected.domain do assert(targetLocales.domain.contains(idx));
-  coforall (idx, col) in zip(targetLocales.domain.sorted(), collected) do
+  import Sort;
+  coforall (idx, col) in zip(Sort.sorted(targetLocales.domain), collected) do
     on targetLocales[idx] do
       col = replicatedVar[rcDomainIx];
 }

diff --git a/modules/standard/Sort.chpl b/modules/standard/Sort.chpl
@@ -811,6 +811,11 @@ proc sort(ref x: [?Dom] , comparator:? = new DefaultComparator(), param stable:b
   where Dom.rank != 1 || !x.isRectangular() {
     compilerError("sort() is currently only supported for 1D rectangular arrays");
 }
+@chpldoc.nodoc
+proc sort(ref x: domain,
+          comparator:? = new DefaultComparator(),
+          param stable:bool = false) do
+  compilerError("sort() is not supported on domains");
 
 /*
    Check if array `x` is in sorted order
@@ -868,9 +873,12 @@ proc isSorted(x: list(?), comparator:? = new DefaultComparator()): bool {
 @chpldoc.nodoc
 /* Error message for multi-dimension arrays */
 proc isSorted(x: [], comparator:? = new DefaultComparator())
-  where x.domain.rank != 1 {
-    compilerError("isSorted() requires 1-D array");
+  where x.rank != 1 || !x.isRectangular() {
+    compilerError("isSorted() is currently only supported for 1D rectangular arrays");
 }
+@chpldoc.nodoc
+proc isSorted(x: domain, comparator:? = new DefaultComparator()) do
+  compilerError("isSorted() is not supported on domains");
 
 /*
    Check if array `Data` is in sorted order
@@ -890,6 +898,8 @@ proc isSorted(Data: [?Dom] ?eltType, comparator:?rec=defaultComparator): bool {
 
 @chpldoc.nodoc
 iter sorted(x : domain, comparator:? = new DefaultComparator()) {
+  if !x.isAssociative() then
+    compilerError("sorted() is currently only supported on associative domains");
   for i in x._value.dsiSorted(comparator) {
     yield i;
   }
@@ -909,8 +919,8 @@ iter sorted(x : domain, comparator:? = new DefaultComparator()) {
 //
 // TODO - Make standalone or leader/follower parallel iterator
 /*
-   Yield the elements of argument `x` in sorted order, using sort
-   algorithm.
+   Yield the elements of argument `x` in sorted order, using the same algorithm
+   as :proc:`sort`.
 
    .. note:
 
@@ -935,7 +945,10 @@ iter sorted(x, comparator:? = new DefaultComparator()) {
     }
   } else if isArrayValue(x) && Reflection.canResolveMethod(x._value, "dsiSorted") {
     compilerError(x._value.type:string + " does not support dsiSorted(comparator)");
-  } else {
+  } else if isArrayValue(x) && x.isSparse() {
+    compilerError("sorted() is not supported on sparse arrays");
+  }
+  else {
     var y = x; // need to do before isArrayValue test in case x is an iterable
     param iterable = isArrayValue(y) || isSubtype(y.type, List.list(?));
     if iterable {

diff --git a/test/arrays/diten/enumArrayMethods.chpl b/test/arrays/diten/enumArrayMethods.chpl
@@ -1,20 +1,21 @@
+import Sort;
 enum E {a=1, b=0, c=2};
 
 var D: domain(E) = E.a..E.c;
 
-writeln(D.sorted());
+writeln(Sort.sorted(D));
 
 D -= E.a;
-writeln(D.sorted());
+writeln(Sort.sorted(D));
 
 D -= E.b;
-writeln(D.sorted());
+writeln(Sort.sorted(D));
 
 D += E.a;
-writeln(D.sorted());
+writeln(Sort.sorted(D));
 
 D += E.b;
-writeln(D.sorted());
+writeln(Sort.sorted(D));
 
 D.clear();
-writeln(D.sorted());
+writeln(Sort.sorted(D));
diff --git a/test/arrays/diten/nestSortedIter.chpl b/test/arrays/diten/nestSortedIter.chpl
@@ -1,7 +1,8 @@
+import Sort;
 var D: domain(int);
 D += 1; D += 2; D+=4; D += 3;
-for a in D.sorted() {
-  for b in D.sorted() {
+for a in Sort.sorted(D) {
+  for b in Sort.sorted(D) {
     writeln((a,b));
   }
 }
diff --git a/test/associative/bharshbarg/domains/recordWithRange.chpl b/test/associative/bharshbarg/domains/recordWithRange.chpl
@@ -1,3 +1,4 @@
+import Sort;
 use List;
 
 config const n = 100;
@@ -58,5 +59,5 @@ for 1..n {
   assert(D.size == recs.size);
 }
 
-for d in D.sorted() do
+for d in Sort.sorted(D) do
   writeln(d, " => ", A[d]);
diff --git a/test/associative/bradc/assocRecord.chpl b/test/associative/bradc/assocRecord.chpl
@@ -1,3 +1,4 @@
+import Sort;
 record Coord {
   var x: int;
   var y: int;
@@ -20,7 +21,7 @@ Name(c1) = "z basis coordinate";
 Name(c2) = "y basis coordinate";
 Name(c3) = "x basis coordinate";
 
-for coord in D.sorted() {
+for coord in Sort.sorted(D) {
   writeln("coord ", coord, " is called ", Name(coord));
 }
 

diff --git a/test/associative/deitz/arrays/test_indefinite4.chpl b/test/associative/deitz/arrays/test_indefinite4.chpl
@@ -1,3 +1,4 @@
+import Sort;
 use printHelp;
 
 var d : domain(int);
@@ -12,5 +13,5 @@ a(5) = 4;
 writelnSorted(d);
 writelnSortedByDom(a);
 
-for i in d.sorted() do
+for i in Sort.sorted(d) do
   writeln(i, " -> ", a(i));
diff --git a/test/associative/deitz/arrays/test_indefinite5.chpl b/test/associative/deitz/arrays/test_indefinite5.chpl
@@ -1,3 +1,4 @@
+import Sort;
 use printHelp;
 
 var d : domain(int);
@@ -12,5 +13,5 @@ a(0) = 4;
 writelnSorted(d);
 writelnSortedByDom(a);
 
-for i in d.sorted() do
+for i in Sort.sorted(d) do
   writeln(i, " -> ", a(i));
diff --git a/test/associative/deitz/arrays/test_indefinite6.chpl b/test/associative/deitz/arrays/test_indefinite6.chpl
@@ -1,3 +1,4 @@
+import Sort;
 use printHelp;
 
 var d : domain(int);
@@ -12,5 +13,5 @@ a(0) = 4;
 writelnSorted(d);
 writelnSortedByDom(a);
 
-for i in d.sorted() do
+for i in Sort.sorted(d) do
   writeln(i, " -> ", a(i));
diff --git a/test/associative/deitz/arrays/test_indefinite7.chpl b/test/associative/deitz/arrays/test_indefinite7.chpl
@@ -1,3 +1,4 @@
+import Sort;
 use printHelp;
 
 var d : domain(string);
@@ -12,5 +13,5 @@ a("zero") = 4;
 writelnSorted(d);
 writelnSortedByDom(a);
 
-for i in d.sorted() do
+for i in Sort.sorted(d) do
   writeln(i, " -> ", a(i));
diff --git a/test/associative/deitz/domains/test_indefinite_domain_of_tuple.chpl b/test/associative/deitz/domains/test_indefinite_domain_of_tuple.chpl
@@ -1,3 +1,4 @@
+import Sort;
 use printHelp;
 
 type t = (int, int);
@@ -8,5 +9,5 @@ writelnSorted(D);
 var A: [D] int;
 A((1, 2)) = 5;
 A((3, 4)) = 6;
-for i in D.sorted() do
+for i in Sort.sorted(D) do
   writeln((i, A(i)));
diff --git a/test/associative/ferguson/plus-reduce-assoc.chpl b/test/associative/ferguson/plus-reduce-assoc.chpl
@@ -1,3 +1,4 @@
+import Sort;
 // Can I use + reduce on associative domain?
 var D: domain(int);
 
@@ -9,8 +10,8 @@ proc makeD(x:int) {
 
 D = + reduce [i in 1..10] makeD(i);
 
-writeln(D.sorted());
+writeln(Sort.sorted(D));
 
 var D2 = + reduce for i in 1..10 do makeD(i);
 
-writeln(D2.sorted());
+writeln(Sort.sorted(D2));
diff --git a/test/associative/ferguson/plus-reduce-intent-assoc.chpl b/test/associative/ferguson/plus-reduce-intent-assoc.chpl
@@ -1,8 +1,9 @@
+import Sort;
 // Can use + reduce intent on associative domain?
 var D: domain(int);
 
 forall x in 1..10 with (+ reduce D) {
   D += x;
 }
 
-writeln(D.sorted());
+writeln(Sort.sorted(D));
diff --git a/test/classes/bradc/arrayInClass/arrayOfArithInClass.chpl b/test/classes/bradc/arrayInClass/arrayOfArithInClass.chpl
@@ -1,3 +1,4 @@
+import Sort;
 // declare class types
 
 class ArithC {
@@ -22,7 +23,7 @@ class EnumC {
 }
 
 iter sortedAssoc(arr) {
-  for k in arr.domain.sorted() {
+  for k in Sort.sorted(arr.domain) {
     yield arr[k];
   }
 }
@@ -31,7 +32,7 @@ iter sortedAssoc(arr) {
 
 proc foo(C) {
   if (C.x.domain.isAssociative()) {
-    writeln("C.x.domain is: ", C.x.domain.sorted());
+    writeln("C.x.domain is: ", Sort.sorted(C.x.domain));
     writeln("x is: ", sortedAssoc(C.x));
   } else {
     writeln("C.x.domain is: ", C.x.domain);

diff --git a/test/classes/bradc/arrayInClass/genericArrayInClass-arith.chpl b/test/classes/bradc/arrayInClass/genericArrayInClass-arith.chpl
@@ -1,3 +1,4 @@
+import Sort;
 // declare class types
 
 class ArithC {
@@ -30,7 +31,7 @@ class EnumC {
 }
 
 iter sortedAssoc(arr) {
-  for k in arr.domain.sorted() {
+  for k in Sort.sorted(arr.domain) {
     yield arr[k];
   }
 }
@@ -39,7 +40,7 @@ iter sortedAssoc(arr) {
 
 proc foo(C) {
   if (C.x.domain.isAssociative()) {
-    writeln("C.x.domain is: ", C.x.domain.sorted());
+    writeln("C.x.domain is: ", Sort.sorted(C.x.domain));
     writeln("x is: ", sortedAssoc(C.x));
   } else {
     writeln("C.x.domain is: ", C.x.domain);

diff --git a/test/classes/bradc/arrayInClass/genericArrayInClass-otharrs.chpl b/test/classes/bradc/arrayInClass/genericArrayInClass-otharrs.chpl
@@ -1,3 +1,4 @@
+import Sort;
 // declare class types
 
 class ArithC {
@@ -31,7 +32,7 @@ class EnumC {
 
 /* We have to handle nested associative types */
 iter sortedOut(arr) {
-  const ks = if arr.domain.isAssociative() then arr.domain.sorted() else arr.domain;
+  const ks = if arr.domain.isAssociative() then Sort.sorted(arr.domain) else arr.domain;
   for k in ks {
     const x = arr[k];
     if isArrayType(x.type) && x.domain.isAssociative() {
@@ -47,7 +48,7 @@ iter sortedOut(arr) {
 
 proc foo(C) {
   if (C.x.domain.isAssociative()) {
-    writeln("C.x.domain is: ", C.x.domain.sorted());
+    writeln("C.x.domain is: ", Sort.sorted(C.x.domain));
     writeln("x is: ", sortedOut(C.x));
   } else {
     writeln("C.x.domain is: ", C.x.domain);

diff --git a/test/classes/bradc/arrayInClass/genericArrayInClass-real.chpl b/test/classes/bradc/arrayInClass/genericArrayInClass-real.chpl
@@ -1,3 +1,4 @@
+import Sort;
 // declare class types
 
 class ArithC {
@@ -30,7 +31,7 @@ class EnumC {
 }
 
 iter sortedAssoc(arr) {
-  for k in arr.domain.sorted() {
+  for k in Sort.sorted(arr.domain) {
     yield arr[k];
   }
 }
@@ -39,7 +40,7 @@ iter sortedAssoc(arr) {
 
 proc foo(C) {
   if (C.x.domain.isAssociative()) {
-    writeln("C.x.domain is: ", C.x.domain.sorted());
+    writeln("C.x.domain is: ", Sort.sorted(C.x.domain));
     writeln("x is: ", sortedAssoc(C.x));
   } else {
     writeln("C.x.domain is: ", C.x.domain);

diff --git a/test/classes/bradc/arrayInClass/genericClassArray-named.chpl b/test/classes/bradc/arrayInClass/genericClassArray-named.chpl
@@ -1,5 +1,6 @@
+import Sort;
 iter sortedAssoc(arr) {
-  for k in arr.domain.sorted() {
+  for k in Sort.sorted(arr.domain) {
     yield arr[k];
   }
 }
@@ -11,7 +12,7 @@ class C {
 
   proc foo() {
     if (x.domain.isAssociative()) {
-      writeln("x.domain is: ", x.domain.sorted());
+      writeln("x.domain is: ", Sort.sorted(x.domain));
       writeln("x is: ", sortedAssoc(x));
     } else {
       writeln("x.domain is: ", x.domain);

diff --git a/test/distributions/bradc/assoc/userAssoc-array-domain-zipper.chpl b/test/distributions/bradc/assoc/userAssoc-array-domain-zipper.chpl
@@ -1,3 +1,4 @@
+import Sort;
 use HashedDist;
 
 config const verbose = false;
@@ -20,7 +21,7 @@ D += 2.4;
 D += 3.5;
 
 writeln("D is:");
-for d in D.sorted() {
+for d in Sort.sorted(D) {
   writeln(d);
 }
 
@@ -31,7 +32,7 @@ A(2.4) = "two point four";
 A(3.5) = "three point five";
 
 writeln("A is:");
-for d in D.sorted() {
+for d in Sort.sorted(D) {
   writeln(A[d]);
   if verbose then
     writeln(A[d], " on locale ", A[d].locale);