Change the ref-maybe-const deprecation on foralls into an unstable warning (#23526)

jabraham17 · web-flow · commit 1dd8dcaa17f8 · 2023-09-22T12:27:52.000-07:00
Changes the warning for an implicit ref intent on arrays in foralls into an unstable warning, which is off by default. After offline discussion, concerns raised in #23488 and #23229, and concerns raised by Arkouda developers, we decided to soften our stance on this change for the release. ## Summary of changes - add check for unstable flag in `compiler/resolution/lowerIterators.cpp` where the warning is thrown - updated warning message to indicate the feature is unstable - changed the wording in the language evolution document to reflect these changes - moved `test/unstable/ref-maybe-const-forall-intent.chpl` to the `unstable` tests folder - updated submitted perf tests .good files and perfkeys accordingly - test/studies/shootout/submitted/binarytrees3.chpl - test/studies/shootout/submitted/knucleotide3.chpl - test/studies/shootout/submitted/knucleotide4.chpl - test/studies/shootout/submitted/revcomp3.chpl - test/studies/shootout/submitted/revcomp5.chpl - test/studies/shootout/submitted/revcomp8.chpl - test/studies/shootout/submitted/spectralnorm.chpl - test/studies/shootout/submitted/spectralnorm2.chpl - test/studies/shootout/submitted/mandelbrot.chpl - test/studies/shootout/submitted/mandelbrot3.chpl ## Testing - paratest with futures - paratest with no futures + gasnet - local testing of all modified tests - local testing with `-performance` of all modified perf tests [Reviewed by @mppf]
diff --git a/compiler/resolution/lowerIterators.cpp b/compiler/resolution/lowerIterators.cpp
@@ -3083,9 +3083,10 @@ void maybeIssueRefMaybeConstWarning(ForallStmt* fs, Symbol* sym, std::vector<Cal
   for (auto ce: allCalls) {
     // if a call sets the symbol, warn
     if (callSetsSymbol(sym, ce)) {
+      // checking for --warn-unstable done in checkForallRefMaybeConst
       USR_WARN(fs,
-                "inferring a default intent to be 'ref' is deprecated - "
-                "please add an explicit 'ref' forall intent for '%s'",
+                "inferring a 'ref' intent on an array in a forall is unstable"
+                " - in the future this may require an explicit 'ref' forall intent for '%s'",
                 sym->name);
       break;
     }
@@ -3094,6 +3095,9 @@ void maybeIssueRefMaybeConstWarning(ForallStmt* fs, Symbol* sym, std::vector<Cal
 
 
 static void checkForallRefMaybeConst(ForallStmt* fs, std::set<Symbol*> syms) {
+  // bail out here if we shouldn't be warning for this forall
+  if (!shouldWarnUnstableFor(fs)) return;
+
   // collect all SymExpr used in the iterand of the forall so we dont warn for them
   std::vector<SymExpr*> allIterandSymExprs;
   for_alist(expr, fs->iteratedExpressions()) {
diff --git a/doc/rst/language/evolution.rst b/doc/rst/language/evolution.rst
@@ -60,15 +60,20 @@ The default intent for arrays and records
 *****************************************
 
 In version 1.32, arrays and records now always have a default intent of
-``const``. This means that if arrays and records are modified inside of a loop
-body, a function, or a task body they must use a ``ref`` intent. This also
-means that record methods which modify their implicit ``this`` argument must
-also use a ``ref`` intent. Previously, the compiler would treat these types as
-either ``const ref`` intent or ``ref`` intent, depending on if they were
-modified. This change was motivated by improving the consistency across types
-and making potential problems more apparent.
-
-Consider the following code segment, which contains a ``forall`` statement
+``const``. This means that if arrays and records are modified inside of a
+function, a ``coforall``, a ``begin``, or ``cobegin``,  they must use a ``ref``
+intent. This also means that record methods which modify their implicit
+``this`` argument must also use a ``ref`` intent. Previously, the compiler would treat
+these types as either ``const ref`` intent or ``ref`` intent, depending on if
+they were modified. This change was motivated by improving the consistency
+across types and making potential problems more apparent.
+
+Since there is a lot of user code relying on modifying an outer array, the
+corresponding change for ``forall`` is still under discussion. As a result, it
+will not warn by default, but modifying an outer array from a ``forall`` might
+not be allowed in the future in some or all cases.
+
+Consider the following code segment, which contains a ``coforall`` statement
 which modifies local variables. Prior to version 1.32, this code compiled and
 worked without warning.
 
@@ -78,7 +83,7 @@ worked without warning.
    const myDomain = {1..10};
    var myArray: [myDomain] int;
 
-   forall i in 2..9 with (ref myInt) {
+   coforall i in 2..9 with (ref myInt) {
      myInt += i;
      myArray[i] = myArray[i-1] + 1;
    }
@@ -99,7 +104,7 @@ which can be a source of subtle bugs. In 1.32, the loop is written as:
    const myDomain = {1..10};
    var myArray: [myDomain] int;
 
-   forall i in 2..9 with (ref myInt, ref myArray) {
+   coforall i in 2..9 with (ref myInt, ref myArray) {
      myInt += i;
      myArray[i] = myArray[i-1] + 1;
    }
diff --git a/test/deprecated/ref-maybe-const-forall-intent.good b/test/deprecated/ref-maybe-const-forall-intent.good
diff --git a/test/studies/shootout/submitted/binarytrees3.good b/test/studies/shootout/submitted/binarytrees3.good
@@ -1,5 +1,3 @@
-binarytrees3.chpl:13: In function 'main':
-binarytrees3.chpl:38: warning: inferring a default intent to be 'ref' is deprecated - please add an explicit 'ref' forall intent for 'stats'
 stretch tree of depth 11	 check: 4095
 1024	 trees of depth 4	 check: 31744
 256	 trees of depth 6	 check: 32512
diff --git a/test/studies/shootout/submitted/binarytrees3.perfkeys b/test/studies/shootout/submitted/binarytrees3.perfkeys
@@ -1,3 +1,3 @@
 real
-verify:3: stretch tree of depth \d+\t check: 8388607
+verify:1: stretch tree of depth \d+\t check: 8388607
 verify: long lived tree of depth \d+\t check: 4194303
diff --git a/test/studies/shootout/submitted/knucleotide3.good b/test/studies/shootout/submitted/knucleotide3.good
@@ -42,7 +42,6 @@ knucleotide3.chpl:76: In function 'calculate':
 knucleotide3.chpl:88: warning: 'map.items' is deprecated. Consider 'map.keys' to iterate over keys or 'map.values' to iterate over values.
   knucleotide3.chpl:69: called as calculate(data: [domain(1,int(64),one)] uint(8), param nclSize = 18) from function 'writeCount'
   knucleotide3.chpl:49: called as writeCount(data: [domain(1,int(64),one)] uint(8), param str = "GGTATTTTAATTTATAGT") from function 'main'
-knucleotide3.chpl:100: warning: inferring a default intent to be 'ref' is deprecated - please add an explicit 'ref' forall intent for 'toNum'
 A 30.279
 T 30.113
 G 19.835
diff --git a/test/studies/shootout/submitted/knucleotide3.perfkeys b/test/studies/shootout/submitted/knucleotide3.perfkeys
@@ -1,3 +1,3 @@
 real
-verify:66:GG 3.902
-verify:72:893	GGTATTTTAATTTATAGT
+verify:65:GG 3.902
+verify:71:893	GGTATTTTAATTTATAGT
diff --git a/test/studies/shootout/submitted/knucleotide4.good b/test/studies/shootout/submitted/knucleotide4.good
@@ -48,7 +48,6 @@ knucleotide4.chpl:75: In function 'calculate':
 knucleotide4.chpl:87: warning: 'map.items' is deprecated. Consider 'map.keys' to iterate over keys or 'map.values' to iterate over values.
   knucleotide4.chpl:68: called as calculate(data: [domain(1,int(64),one)] uint(8), param nclSize = 18) from function 'writeCount'
   knucleotide4.chpl:48: called as writeCount(data: [domain(1,int(64),one)] uint(8), param str = "GGTATTTTAATTTATAGT") from function 'main'
-knucleotide4.chpl:99: warning: inferring a default intent to be 'ref' is deprecated - please add an explicit 'ref' forall intent for 'toNum'
 A 30.279
 T 30.113
 G 19.835
diff --git a/test/studies/shootout/submitted/knucleotide4.perfkeys b/test/studies/shootout/submitted/knucleotide4.perfkeys
@@ -1,3 +1,3 @@
 real
-verify:72:GG 3.902
-verify:78:893	GGTATTTTAATTTATAGT
+verify:71:GG 3.902
+verify:77:893	GGTATTTTAATTTATAGT
diff --git a/test/studies/shootout/submitted/mandelbrot.pbm b/test/studies/shootout/submitted/mandelbrot.pbm
diff --git a/test/studies/shootout/submitted/mandelbrot3.pbm b/test/studies/shootout/submitted/mandelbrot3.pbm
diff --git a/test/studies/shootout/submitted/revcomp3.good b/test/studies/shootout/submitted/revcomp3.good
@@ -8,10 +8,6 @@ revcomp3.chpl:15: warning: reader with a 'kind' argument is deprecated, please u
 revcomp3.chpl:17: warning: openfd is deprecated, please use the file initializer with a 'c_int' argument instead
 revcomp3.chpl:17: warning: writer with a 'kind' argument is deprecated, please use Serializers instead
 revcomp3.chpl:55: warning: inferring a default intent to be 'ref' is deprecated - please use an explicit 'ref' intent for the argument 'buf'
-revcomp3.chpl:55: In function 'revcomp':
-revcomp3.chpl:71: warning: inferring a default intent to be 'ref' is deprecated - please add an explicit 'ref' forall intent for 'buf'
-revcomp3.chpl:79: warning: inferring a default intent to be 'ref' is deprecated - please add an explicit 'ref' forall intent for 'buf'
-  revcomp3.chpl:45: called as revcomp(buf: [domain(1,int(64),one)] uint(8)) from function 'main'
 >ONE Homo sapiens alu
 CGGAGTCTCGCTCTGTCGCCCAGGCTGGAGTGCAGTGGCGCGATCTCGGCTCACTGCAAC
 CTCCGCCTCCCGGGTTCAAGCGATTCTCCTGCCTCAGCCTCCCGAGTAGCTGGGATTACA
diff --git a/test/studies/shootout/submitted/revcomp3.perfkeys b/test/studies/shootout/submitted/revcomp3.perfkeys
@@ -1,2 +1,2 @@
 real
-verify:16666685:AACATTACAGGTAATGATAA
+verify:16666681:AACATTACAGGTAATGATAA
diff --git a/test/studies/shootout/submitted/revcomp5.good b/test/studies/shootout/submitted/revcomp5.good
@@ -8,10 +8,6 @@ revcomp5.chpl:22: warning: reader with a 'kind' argument is deprecated, please u
 revcomp5.chpl:24: warning: openfd is deprecated, please use the file initializer with a 'c_int' argument instead
 revcomp5.chpl:24: warning: writer with a 'kind' argument is deprecated, please use Serializers instead
 revcomp5.chpl:64: warning: inferring a default intent to be 'ref' is deprecated - please use an explicit 'ref' intent for the argument 'buf'
-revcomp5.chpl:64: In function 'revcomp':
-revcomp5.chpl:71: warning: inferring a default intent to be 'ref' is deprecated - please add an explicit 'ref' forall intent for 'buf'
-revcomp5.chpl:79: warning: inferring a default intent to be 'ref' is deprecated - please add an explicit 'ref' forall intent for 'buf'
-  revcomp5.chpl:54: called as revcomp(buf: [domain(1,int(64),one)] uint(8), lo: int(64), hi: int(64)) from function 'main'
 >ONE Homo sapiens alu
 CGGAGTCTCGCTCTGTCGCCCAGGCTGGAGTGCAGTGGCGCGATCTCGGCTCACTGCAAC
 CTCCGCCTCCCGGGTTCAAGCGATTCTCCTGCCTCAGCCTCCCGAGTAGCTGGGATTACA
diff --git a/test/studies/shootout/submitted/revcomp5.perfkeys b/test/studies/shootout/submitted/revcomp5.perfkeys
@@ -1,2 +1,2 @@
 real
-verify:16666685:AACATTACAGGTAATGATAA
+verify:16666681:AACATTACAGGTAATGATAA
diff --git a/test/studies/shootout/submitted/revcomp8.good b/test/studies/shootout/submitted/revcomp8.good
@@ -17,9 +17,6 @@ revcomp8.chpl:80: warning: inferring a default intent to be 'ref' is deprecated
 revcomp8.chpl:146: warning: inferring a default intent to be 'ref' is deprecated - please use an explicit 'ref' intent for the argument 'buff'
 revcomp8.chpl:146: warning: inferring a default intent to be 'ref' is deprecated - please use an explicit 'ref' intent for the argument 'seq'
 revcomp8.chpl:96: warning: inferring a default intent to be 'ref' is deprecated - please add an explicit 'ref' task intent for 'seq'
-revcomp8.chpl:33: In function 'main':
-revcomp8.chpl:36: warning: inferring a default intent to be 'ref' is deprecated - please add an explicit 'ref' forall intent for 'pairCmpl'
-revcomp8.chpl:62: warning: inferring a default intent to be 'ref' is deprecated - please add an explicit 'ref' forall intent for 'buff'
 >ONE Homo sapiens alu
 CGGAGTCTCGCTCTGTCGCCCAGGCTGGAGTGCAGTGGCGCGATCTCGGCTCACTGCAAC
 CTCCGCCTCCCGGGTTCAAGCGATTCTCCTGCCTCAGCCTCCCGAGTAGCTGGGATTACA
diff --git a/test/studies/shootout/submitted/spectralnorm.good b/test/studies/shootout/submitted/spectralnorm.good
@@ -2,12 +2,4 @@ spectralnorm.chpl:35: warning: inferring a default intent to be 'ref' is depreca
 spectralnorm.chpl:27: warning: inferring a default intent to be 'ref' is deprecated - please use an explicit 'ref' intent for the argument 'tmp'
 spectralnorm.chpl:43: warning: inferring a default intent to be 'ref' is deprecated - please use an explicit 'ref' intent for the argument 'Atv'
 spectralnorm.chpl:27: warning: inferring a default intent to be 'ref' is deprecated - please use an explicit 'ref' intent for the argument 'AtAv'
-spectralnorm.chpl:35: In function 'multiplyAv':
-spectralnorm.chpl:36: warning: inferring a default intent to be 'ref' is deprecated - please add an explicit 'ref' forall intent for 'Av'
-  spectralnorm.chpl:28: called as multiplyAv(v: [domain(1,int(64),one)] real(64), Av: [domain(1,int(64),one)] real(64)) from function 'multiplyAtAv'
-  spectralnorm.chpl:17: called as multiplyAtAv(v: [domain(1,int(64),one)] real(64), tmp: [domain(1,int(64),one)] real(64), AtAv: [domain(1,int(64),one)] real(64))
-spectralnorm.chpl:43: In function 'multiplyAtv':
-spectralnorm.chpl:44: warning: inferring a default intent to be 'ref' is deprecated - please add an explicit 'ref' forall intent for 'Atv'
-  spectralnorm.chpl:29: called as multiplyAtv(v: [domain(1,int(64),one)] real(64), Atv: [domain(1,int(64),one)] real(64)) from function 'multiplyAtAv'
-  spectralnorm.chpl:17: called as multiplyAtAv(v: [domain(1,int(64),one)] real(64), tmp: [domain(1,int(64),one)] real(64), AtAv: [domain(1,int(64),one)] real(64))
 1.274224116
diff --git a/test/studies/shootout/submitted/spectralnorm.perfkeys b/test/studies/shootout/submitted/spectralnorm.perfkeys
@@ -1,3 +1,3 @@
 # file: spectralnorm-submitted.dat
 real
-verify:13: 1.274224153
+verify:5: 1.274224153
diff --git a/test/studies/shootout/submitted/spectralnorm2.good b/test/studies/shootout/submitted/spectralnorm2.good
@@ -2,12 +2,4 @@ spectralnorm2.chpl:35: warning: inferring a default intent to be 'ref' is deprec
 spectralnorm2.chpl:27: warning: inferring a default intent to be 'ref' is deprecated - please use an explicit 'ref' intent for the argument 'tmp'
 spectralnorm2.chpl:43: warning: inferring a default intent to be 'ref' is deprecated - please use an explicit 'ref' intent for the argument 'Atv'
 spectralnorm2.chpl:27: warning: inferring a default intent to be 'ref' is deprecated - please use an explicit 'ref' intent for the argument 'AtAv'
-spectralnorm2.chpl:35: In function 'multiplyAv':
-spectralnorm2.chpl:36: warning: inferring a default intent to be 'ref' is deprecated - please add an explicit 'ref' forall intent for 'Av'
-  spectralnorm2.chpl:28: called as multiplyAv(v: [domain(1,int(64),one)] real(64), Av: [domain(1,int(64),one)] real(64)) from function 'multiplyAtAv'
-  spectralnorm2.chpl:17: called as multiplyAtAv(v: [domain(1,int(64),one)] real(64), tmp: [domain(1,int(64),one)] real(64), AtAv: [domain(1,int(64),one)] real(64))
-spectralnorm2.chpl:43: In function 'multiplyAtv':
-spectralnorm2.chpl:44: warning: inferring a default intent to be 'ref' is deprecated - please add an explicit 'ref' forall intent for 'Atv'
-  spectralnorm2.chpl:29: called as multiplyAtv(v: [domain(1,int(64),one)] real(64), Atv: [domain(1,int(64),one)] real(64)) from function 'multiplyAtAv'
-  spectralnorm2.chpl:17: called as multiplyAtAv(v: [domain(1,int(64),one)] real(64), tmp: [domain(1,int(64),one)] real(64), AtAv: [domain(1,int(64),one)] real(64))
 1.274224116
diff --git a/test/studies/shootout/submitted/spectralnorm2.perfkeys b/test/studies/shootout/submitted/spectralnorm2.perfkeys
@@ -1,2 +1,2 @@
 real
-verify:13: 1.274224153
+verify:5: 1.274224153
diff --git a/test/unstable/ref-maybe-const-forall-intent.chpl b/test/unstable/ref-maybe-const-forall-intent.chpl
diff --git a/test/unstable/ref-maybe-const-forall-intent.good b/test/unstable/ref-maybe-const-forall-intent.good
@@ -0,0 +1,6 @@
+ref-maybe-const-forall-intent.chpl:31: warning: inferring a 'ref' intent on an array in a forall is unstable - in the future this may require an explicit 'ref' forall intent for 'B'
+ref-maybe-const-forall-intent.chpl:70: warning: inferring a 'ref' intent on an array in a forall is unstable - in the future this may require an explicit 'ref' forall intent for 'A'
+ref-maybe-const-forall-intent.chpl:2: warning: inferring a 'ref' intent on an array in a forall is unstable - in the future this may require an explicit 'ref' forall intent for 'myArray1'
+ref-maybe-const-forall-intent.chpl:31: warning: inferring a 'ref' intent on an array in a forall is unstable - in the future this may require an explicit 'ref' forall intent for 'B'
+ref-maybe-const-forall-intent.chpl:50: warning: inferring a 'ref' intent on an array in a forall is unstable - in the future this may require an explicit 'ref' forall intent for 'myArrayD'
+0,0,0,0,0,0,0,0,0,0,

Original file line number	Diff line number	Diff line change
`@@ -1,5 +1,3 @@`
`1`		`-binarytrees3.chpl:13: In function 'main':`
`2`		`-binarytrees3.chpl:38: warning: inferring a default intent to be 'ref' is deprecated - please add an explicit 'ref' forall intent for 'stats'`
`3`	`1`	`stretch tree of depth 11 check: 4095`
`4`	`2`	`1024 trees of depth 4 check: 31744`
`5`	`3`	`256 trees of depth 6 check: 32512`