Chapel: Unable to update elements of a list within a map

[ben-albrecht]

Summary of Problem

Updates to the elements of a list within a map seem to be dropped. See the example code below.

Source Code:

use Map;
use List;

var m: map(string, list(int));

m['a'] = new list(int);

writeln(m);
// {a: []}

m['a'].append(1);
writeln(m);
// {a: [1]}

m['a'][1] = 2;
writeln(m);
// {a: [1]}
// but I expected:
// {a: [2]}

Taking a peek at the Map.chpl source code, I noticed there are 2 this methods:

proc this(k: keyType) ref { .. }
proc const this(k: keyType) const { .. }

It turns out the const this is getting called in my program above, which explains why the updates are not happening. Assuming this is actually a bug and not a feature, I suspect the issue lies in compiler resolution.

Related to https://github.com/chapel-lang/chapel/issues/14474

Configuration Information

• Output of chpl --version: chpl version 1.21.0 pre-release (4f4e86253a)

[ben-albrecht]

FYI map makers @dlongnecke-cray && @daviditen

@e-kayrakli -- this is the issue we discussed offline.

[mppf]

This patch fixes the issue:

diff --git a/modules/standard/List.chpl b/modules/standard/List.chpl
index 59aa75f4c5..3872d79e1c 100644
--- a/modules/standard/List.chpl
+++ b/modules/standard/List.chpl
@@ -351,7 +351,7 @@ module List {
     // accesses of list elements should go through this function.
     //
     pragma "no doc"
-    inline proc _getRef(idx: int) ref {
+    inline proc const ref _doGetRef(idx: int) ref {
       _sanity(idx >= 1 && idx <= _totalCapacity);
       const zpos = idx - 1;
       const arrayIdx = _getArrayIdx(zpos);
@@ -361,6 +361,12 @@ module List {
       ref result = array[itemIdx];
       return result;
     }
+    inline proc ref _getRef(idx: int) ref {
+      return _doGetRef(idx);
+    }
+    inline proc const ref _getRef(idx: int) const ref {
+      return _doGetRef(idx);
+    }

     pragma "no doc"
     inline proc _enter() {
@@ -576,7 +582,7 @@ module List {
       :arg x: An element to append.
       :type x: `eltType`
     */
-    proc append(pragma "no auto destroy" in x: eltType) lifetime this < x {
+    proc ref append(pragma "no auto destroy" in x: eltType) lifetime this < x {
       _enter();

       //

@dlongnecke-cray , @daviditen - I think we need to make sure that Map/List methods have ref or const ref this-intent as appropriate. In general this should be inferred by the compiler for a record, but if the type is updating something hanging off of a pointer, the compiler won't infer it as a modification to the type itself. This is important for patterns that rely on return-intent-overloading such as the situation described in this issue.

[dlongnecke-cray]

a) So basically, we should prefer explicitly writing decorators for methods to prevent cases where the compiler may not be able to infer them?

b) If a method is working with some pointer type such as _ddata that the compiler cannot analyze, we should provide both ref and const ref overloads (as was done for _getRef)?

[mppf]

@dlongnecke-cray

b first -- Record methods have this intent inferred as ref or const ref depending on if any fields are modified (or methods called with ref). But modifying a field through a pointer (including say an unmanaged MyClass or owned MyClass or _ddata) does not count as modifying the record so doesn't make it const. The adjustment to _getRef allows the compiler to consider modifying the element returned to be equivalent to modifying the record.

a -- I think it's nice in a case like this to write explicit decorators to be clear (also might allow for better error messages in the event something is inconsistent)

[dlongnecke-cray]

Ah, I understand now. If we're returning a mutable reference to an element in the list then ~to be safe~ to force the correct behavior we should mark that method as ref.