Rank-change type checking is stricter than array indexing

[e-kayrakli]

Consider:

var A: [1:int(32)..10:int(32), 1:int(32)..10:int(32)] int = 2;

writeln(A[2:int(32), ..]);  // I had to cast here

writeln(A[3,4]);  // but not here

I noticed this while looking at the implementation. I want to say this is a bug where the integral index(es) of a rank-change must be coercible to the idxType of the base domain. Or maybe this is just defensive programming against overflow. I think we can tighten up the bounds checking code to protect against that. In any case, if that's a concern, then there must be another bug with bounds checking for array indexing. For now, I am assuming that this is a bug.

[e-kayrakli]

Starting from where the checking occurs (_validRankChangeArgs, a helper that I wanted to use in a different context without idxType), I tracked it down to chpl_intToIdx in ranges. It is a function used in bounds checking. Looks like it can accept the types I'd like it to work with, but it will allow overflow.

I first thought that this can't be correct as it could cause issues for things like:

var myRange = 1:uint(8)..max(uint(8));

writeln(myRange);    // 1..255
writeln(257:uint(8)); // overflows back to 1
writeln(myRange.contains(257));  // I was pretty sure this was going to give me true
                                 // ... but it doesn't resolve

I think we should allow different integral types to be passed to contains, and in order to make that work, we'll have to change its implementation to not use chpl_intToIdx (or fix that, too). In any case, starting from there will probably make what the OP asks to just fall out.

[bradcray]

In the OP, I feel pretty confident that this is a bug/shortcoming/oversight in the rank change code rather than an intentional / defensive choice in the implementation.

I think we should allow different integral types to be passed to contains

In doing so, are you thinking your .contains(257) should return the true you were expecting due to wraparound, or false due to 257 being out of the int(8)'s representational power? (my answer would be the latter)

[e-kayrakli]

In doing so, are you thinking your .contains(257) should return the true you were expecting due to wraparound, or false due to 257 being out of the int(8)'s representational power? (my answer would be the latter)

Same here. Very strongly so.

[bradcray]

Would implementing potentially be able to be done by doing:

• implement routine for idxType that works more or less as I imagine it does today (maybe inaccurately)
• add a second overload for integral that does a check to see whether the value fits into idxType, does the cast and calls the version above if it does; returns false otherwise?

[e-kayrakli]

That sounds like a reasonable thing to try. Adjusting the wiring around this could be a bit more challenging than your description, though (still not too bad). The helper in question, _validRankChangeArgs is a param returning function used in where clauses. So it is more like,

• make _validRankChangeArgs ignore idxType (this is the kind of helper I was looking for, too)
• implement the "does it fit" logic in the rank-changing this, which would result in an additional point for bounds checking as you can no longer rely on ranges' contains.

From a procedural standpoint, I am not sure if it'd worth the effort as opposed to just fixing/improving the range interface, where only the first bullet above would be sufficient to fix this issue. It'll certainly be more challenging, but I think the delta in effort worth considering the delta in interface quality.