Chapel: const Domain mutable through array.push_back()

[BryantLam]

Somewhat similar to being able to mutate c's class through const c = new Class(), this code currently mutates D. I don't have an opinion one way or another on whether it should, but it seems interesting, especially if D is used as the domain for multiple arrays. If this behavior is expected, feel free to close.

const D = {1..1}; // const?
var arr: [D] int;

for i in 1..5 {
  arr.push_back(i);
}
writeln(arr.domain);
writeln(D == {1..1});

outputs

{1..6}
false

chpl version 1.19.0 pre-release (997eee0)

Edit: Fixed an error that @lydia-duncan caught in my last-line check.

[lydia-duncan]

Did you mean writeln(D == {1..5}); (compare against the new bounds of the array) or writeln(D == {1..1}); (compare against the original bounds of D)? Note that both return false today, but they describe different expectations about the code

[mppf]

I definitely view it to be a bug that the above compiles. In fact a very similar problem was reported in #10911.

I see two TODOs here:

1. Move away from push_back on arrays and towards different types. There are several reasons for this. See #9452
2. Adjust the compiler / array implementation to track when arrays have a const domain or when the domain for an array is never modified after the arrays exist (which is equivalent for optimization purposes). Arguably #10911 talks about this. I think we've also seen cases where the compiler could optimize better if it knew the domain were const. (e.g. hoisting the array header data / element pointer).

[daviditen]

const D = {1..1}; // const?

I agree that this const domain should not be modified by push_back() because it is const.

If it were not const, it starts with one index, so after doing push_back() 5 times it should have 6 indices, so the output would be correct. To make it empty to start you can declare it as: const D = {1..0};.

[bradcray]

@benharsh: Tagging you on this since you're arguably working on a fix to it by moving the push_back() capabilities to a distinct type.

[bradcray]

This will arguably be resolved as soon as issue #13999 is merged. Unless we want to wait for the deprecated feature to be removed. @BryantLam, @dlongnecke-cray, what do you think?

[dlongnecke-cray]

I think this depends on how reliant @BryantLam is on push_back right now. I would consider this issue resolved with deprecation warnings enabled, but there is an argument to be made that it isn't truly resolved until the methods are completely removed.

[BryantLam]

I'm okay with either. Of course, I prefer the deprecated routines to be removed sooner than later since this issue's behavior is still exhibited until such time.

[bradcray]

Let's go ahead and close this issue then, for simplicity. We need to go out with the deprecation warning for one release (1.20) to give users the chance to shift their weight to List or something else, and we have tests in place in the test/deprecated/ directory to help us remember to turn them off before the next release (so don't need this issue to do so... and it would be a less effective reminder anyway).