Associative domain / array combinations permit nil storage in non-nilable array

[bradcray]

Summary of Problem

At present, given an associative array that stores non-nilable class objects, it is possible to store nil in those objects by simply never assigning the corresponding array element. This is likely an instance of the more general problem in issue #13602.

My current thinking is that there ought to be an overload of the .add() method that takes a varargs list of (array, initial value) pairs and initializes the corresponding array element for the new index to the respective initial value. That is, given a call like MyAssocDom.add(idx, (arr1, init1), (arr2, init2), ...)), idx would be added to MyAssocDom and then the listed arrays would be initialized as arr1[idx] = init1, arr2[idx] = init2, and so forth (any associated arrays that had no corresponding argument would be default initialized as usual).

Steps to Reproduce

Source Code:

class C { var x: int; }

var D: domain(string);
var A: [D] shared C;

D += "hi";
// this should arguably be illegal since A["hi"] has been default initialized to `nil` here until the following assignment, which shouldn't be legal
A["hi"] = new shared C(42);
D += "bye";
writeln(A);  // halts due to hitting a nil object

This issue also relates loosely to deferred initialization (issue #14271), though I think doing deferred initialization correctly / sanely for individual array elements is a challenging topic.

Compile command:
chpl foo.chpl

Execution command:
./foo

Configuration Information

• Output of chpl --version: 1.20.0

[dlongnecke-cray]

It seems like at the very least we could start by emitting some sort of error message to prevent this bug from slipping through the cracks, and then we can work towards a more general solution.

[bradcray]

Agreed. The lack of an error message was the main thing that caused me to file this. The brainstorming about add() methods and the like was thinking about how one would modify the domain in a safe/sane way w.r.t. nilability.

[dlongnecke-cray]

After fiddling with some simple solutions and thinking about it some more, it seems like the easiest correct thing to do here would be to just disallow creating arrays with a value type that is a non-nilable class.

In the future, we could lighten this constraint by saying the following...

If the array's value type is a non-nilable class:

    - Its domain must be const.
    - All of its values must be initialized in its initializer expression.

In the even further future, assuming we have support for it, we could say the following:

If the array's value type is a non-nilable class:

    - Its domain must be const.
    - All of its values must be initialized before its first use (?).

At the very least, we should prohibit this in the near term in order to prevent buggy programs, with a message along the lines of:

Arrays of non-nilable classes are not currently supported.

Whenever an array with a value type that is a non-nilable class is declared.

We can then expand support to allow for initializer expressions.

Thoughts?

[bradcray]

I'm OK with this conservative error message as a means of avoiding having people write incorrect code for the time being. It would also cause people who genuinely need such support to step up and complain.

[dlongnecke-cray]

I've encountered an issue that has blindsided me a little bit, which is what to do about the map class. #14846 (comment)

The map uses associative arrays in a safe manner because it always initializes a value at the same time it adds a key - there is no possibility for nilable values to slip through the cracks.

I would like the map type to use associative arrays as they exist today while having other uses of associative arrays emit an error when the value type is a non-nilable class, but am not sure how best to accomplish this. Any ideas?

[bradcray]

Taking a bit more time to stew on this, I'm remembering that an approach I proposed awhile back is to have the map store nilable C when it's asked to store non-nilable C and then to take care of any casts from nilable to non-nilable in its implementation, knowing that there will never be problems.

[dlongnecke-cray]

Taking a bit more time to stew on this, I'm remembering that an approach I proposed awhile back is to have the map store nilable C when it's asked to store non-nilable C and then to take care of any casts from nilable to non-nilable in its implementation, knowing that there will never be problems.

This is what i was thinking we could do as well. The only question is what order of work it is, and what the code will end up looking like. If there is an elegant pattern for unwrapping/rewrapping I could see this approach working.

[dlongnecke-cray]

Despite merging #14846, I still think we should keep this issue open until a more long term solution is found. What do you think, @bradcray?

[bradcray]

Until we either have support for associative arrays of non-nilable classes or decide for certain that we won't do so? Yeah, I agree.

[dlongnecke-cray]

As of #15747 maps of (int, (shared T, shared T)) compile, but they will emit a runtime error until we adjust the associative array API.

If we want to support building something like map out of an associative domain/array, or handle the more general problem of working with associative arrays of non-nilable classes, we need to resolve this issue.

So I thought I'd chip in and offer my opinions on a few different approaches to get discussion moving along.

---

1) proc DefaultAssociativeDom.add(idx: idxType, ...?t)

This is the original idea proposed by @bradcray.

Pros:

• Plays well with owned classes
• I think this idea is appealing because it offers a way to initialize multiple different arrays with different values (that may be different types) in a single call.
• Can handle arrays of different value types declared over the same domain.
• Only have to modify associative domain interface

Cons:

• It is possible for some array values to be default initialized, which can lead to potentially difficult error messages when the default initialization occurs elsewhere.
• If we want to avoid any possibility of default initialization, this function would have to be total over all arrays, which may not be a feasible expectation (can we expect a user to remember all the arrays declared over a given domain?).
• Flexibility makes the interface more complex.

2) proc DefaultAssociativeArr.addIndexAndValue(idx: idxType, val: eltType)

This idea would add a method to associative arrays that has a constraint similar to array.push_back which is that the domain must only have a single array declared over it.

Pros:

• Strict constraint makes for simple interface
• Plays well with owned classes
• We get totality for free due to chpl__assertSingleArrayDomain

Cons:

• Total over all "arrays" at the cost of inflexibility of use
• Error message is potentially confusing (suffers the same context problem of requiring a user to keep track of all the arrays declared over a domain)
• Ugly runtime assertion (this might be unavoidable no matter which path we choose, though)

3) proc DefaultAssociativeArr.addIndexAndValueToAll(idx: idxType, val: eltType)

This was an idea brought up in discussion with @mppf. It is an alternative implementation of addIndexAndValue which aims to be total without constraining a domain to a single array.

Pros:

• Trivially total over all arrays (avoid any possibility of default initialization)
• Avoids ugly chpl__assertSingleArrayDomain constraint

Cons:

• May not work well when arrays of different value types are declared over the same domain
• We can fix this by accepting a varargs, one default value per value type (?)
• Does not play well with owned classes

4) Do not allow adding to a domain if an array contains a non-default-initializable value type

Pros:

• Elegant!
• Simple!

Cons:

• We can't support maps of non-nilable classes :(

[bradcray]

Spitballing here: Is there an option in which the author of an associative array of non-nilable could register a factory function/method of some sort that would say what default value should be used for that array? For example, maybe I would allocate a sentinel object for the array and have all "uninitialized" elements point to that sentinel object?

[dlongnecke-cray]

That's a cool take.

Initialization from a callback might not be that flexible, but it could feasibly support owned classes as long as the handler returns a distinct owned every time (which could wrap some sort of sentinel value (?)).

By having the default handler be a halt, we could exist in the same world as we do today while offering a workaround.

There might be situations where that doesn't make sense (i.e. for owned we're kind of stretching things). And users may not appreciate having to carry around sentinel values with their arrays or check for them.

[vasslitvinov]

@dlongnecke-cray could this be closed because it is superseded by something else?
See also #15094

[vasslitvinov]

P.S. also #16250 with pointers to some specific proposals.

[dlongnecke-cray]

See the section "Supporting Resized Arrays of Non-Nilable Classes" in https://chapel-lang.org/releaseNotes/1.24/04-ongoing.pdf