Chapel: Disallow arrays with nonnilable elements and non-constant domains

[vasslitvinov]

This is a spin-off from a discussion sparkled by https://github.com/chapel-lang/chapel/issues/13602#issuecomment-541802380 .

This code is unsound because resizing the array's domain allocates nil elements:

var D = {1..2};
var A: [D] shared object = .....;
D = {1..3};

This issue proposes to allow arrays with nonnilable elements only over constant domains, including domain literals. This was suggested in https://github.com/chapel-lang/chapel/issues/13602#issuecomment-580546364 .

Cf. #14936 proposes disallowing nonnilable arrays altogether, or just rectangular arrays, for the time being.

[vasslitvinov]

Here is a challenge:

  proc reshape(A: [], D: domain) {
    var B: [D] A.eltType = for (i,a) in zip(D, A) do a;
    return B;
  }

We do not know whether the actual for D is modifiable. So to be honest with ourselves we would disallow calling this on an array of non-nilables.

So far I am seeing this only on arrays of locales.

@bradcray @mppf @benharsh suggestions?

[mppf]

I think the compiler/type system needs to know the difference between array-over-const-domain and array-over-modifiable-domain. However disallowing reshape for now on an array of non-nilables seems acceptable, if annoying.

[bradcray]

Thinking about this general challenge this morning (and tagging @ben-albrecht who picked up a related issue this week and may be able to put some time into this as well as @dlongnecke-cray and @daviditen since they've worked on related issues):

• outlawing arrays of non-nilable feels more severe than we'd like
• this issue proposes outlawing arrays of non-nilable over non-const arrays (to avoid resizing the domain and adding new elements that don't have an obvious initial value other than nil); being able to reason about which arrays have non-const domains is a long-term desire (see #10911, which was filed years after the desire originally appeared)
• so, what if instead, we either:
  
  
  
  • outlawed (at execution time) resizing arrays of non-nilable type in ways that added new indices (restricting indices would be legal); or
  
  
  • outlawed (at compilation time, via a compilerError() in the relevant code) resizing arrays of non-nilable type
  
  

I think either of these last two approaches would be fairly similar in difficulty and quite easy; the second seems preferable to the first if it doesn't introduce a ton of false positives (i.e., do we resolve the "resize array" code path for all arrays even if the user's code doesn't ever resize them).

A result of either would be that the following code would not generate an error because D is never resized:

class C {}
var D = {1..n};
var A: [D] C = [i in D] new C();

The former would have the benefit of permitting arrays to be made smaller, but not larger:

class C {}
var D = {1..2*n};
var A: [D] C = [i in D] new C();
D = {1..n};

but also has the downside of relying on an execution time error rather than a compile-time one. And frankly, I don't know that the compile-time approach is all that over-restrictive (again, assuming it doesn't just prevent everything from compiling).

[vasslitvinov]

The runtime-check-based approach appeals by the simplicity of implementing it.

Compile-time checking otoh feels more Chapel-spirited - in line with how we do things elsewhere, like other nonnilable checks.

For reshape-like situations, as well as var MyArray: (a type formal, type expression, ...), I can see us distinguish mutable vs. non-mutable domains in the type system as Michael suggests or with an interprocedural analysis.

[vasslitvinov]

BTW we should expand the scope of this check, as well as the one in #13602, to all non-default-initializable types. Ex. ones that do not pass isDefaultInitializable() in Types.chpl.

[bradcray]

Shoot, the compile-time approach didn't work due to dynamic dispatch resolution tripping over the compilerError().

However, for a simple test case at least, the execution-time approach does work.

[bradcray]

Only 85 failures for the execution-time approach, many (most?) of which seem to be due to isDefaultInitializable() not resolving for certain types (domains maybe?).

[vasslitvinov]

BTW we should expand the scope of this check, as well as the one in #13602, to all non-default-initializable types. Ex. ones that do not pass isDefaultInitializable() in Types.chpl.

This will not work at the moment because:

• locale is considered non-default-initializable ==> chpl_TableEntry(idxType) is not-default-initializable ==> can't create a DefaultAssociativeDom because its initializer does not set DefaultAssociativeDom.table, which is an array of chpl_TableEntry(idxType).

• isDefaultInitializable() also has an issue with Chapel domain types. #14854 may be relevant.

Both issues are resolvable, however.

[vasslitvinov]

@bradcray I am looking to see if I can make workarounds for isDefaultInitializable().

[bradcray]

Down to 38 failures if I filter for domain types before calling isDefaultInitializable(), all of which are triggering the error I added.

[bradcray]

(though some of those are due to records whose fields contain domains, so maybe I should've moved the test for domains _into_ the isDefaultInitializable() definition rather than my call to it? Domains are default initializable, right? Just maybe not using real initializers?)

[mppf]

AFAIK it's just a bug that isDefaultInitializable is not working for domains - issue #14854

[bradcray]

If I change my test to a simple check for isNonNilableClass(eltType) I only get 2 failures, both of them on cases that reshape arrays of locales which should be fixed with PR #15149 which is encouraging. This isn't a perfect test of course (a record with a non-nilable class field and no default initializer should also trigger the error, but won't be usable until #14854 is resolved presumably.

[vasslitvinov]

I am following Brad's lead and going from isDefaultInitializable() back to isNonNilableClass() in #13602. None of my workarounds allowed me to accept this test:

test/types/records/intents/out-intent-error-array2.chpl

because it has a default-initialized array with this element type:

record R {
  var x: int = 0;
  var ptr: shared C = new shared C(0);
}

which at present is not isDefaultInitializable.

[bradcray]

Somewhat related: In PR #15187, I added an execution-time halt in the event that an array of non-nilable classes is resized. I think of this as a stopgap until we have a compile-time solution.

[ben-albrecht]

Thanks @bradcray!

I think of this as a stopgap until we have a compile-time solution.

Do you think we should keep this issue open until then?

[bradcray]

Yeah. I think of this issue as essentially being blocked by that oldy-but-goody, issue #10911.