Array module review --- error on multidim array print

[stonea]

This PR satisfies this issue: #18090 (Array module review followup: Investigate if we throw an exception if you try and do a "Chapel Style" print out of a multidimensional array)

Basically, we decided that since Chapel doesn't have a syntax for defining a multi-dimensional array literal we want to error out if you try and do a "Chapel style" print

In other words the following should error:

var A:[1..2, 1..2] string = "hi";
writef("%ht\n", A);

And with this PR, it now will with the following message:

uncaught IllegalArgumentError: Can not perform Chapel write of multidimensional array.
  foo.chpl:2: thrown here
  foo.chpl:2: uncaught here

@dlongnecke-cray: @bradcray suggested you might be a good person to reach out for feedback on this change, particularly in regards to how it might interact with arrays of different distributions.

Experimentally, I tried the following (with my PR changes):

use BlockDist;
var A : [newBlockDom({1..2, 1..2})] string = "hi";
writef("%ht\n", A);

And it will error out as:

uncaught IllegalArgumentError: Can not perform Chapel write of multidimensional array.
  foo.chpl:3: thrown here
  foo.chpl:3: uncaught here

As is, I think the change I've made is an improvement, I'm still a little unsatisfied with it. Specifically, because even though I'm throwing an exception it isn't propagating up to the user code. The error message might lead users to thinking that they should update their code to catch an exception, but in fact.

In fact, if I set in my environment: export CHPL_DEVELOPER=1 and rerun the example I'll get the following:

uncaught IllegalArgumentError: Can not perform Chapel write of multidimensional array.
  $CHPL_HOME/modules/internal/DefaultRectangular.chpl:1670: thrown here
  $CHPL_HOME/modules/standard/ChapelIO.chpl:789: uncaught here

With the block distributed array I'll get this:

uncaught IllegalArgumentError: Can not perform Chapel write of multidimensional array.
  $CHPL_HOME/modules/internal/DefaultRectangular.chpl:1670: thrown here
  $CHPL_HOME/modules/dists/BlockDist.chpl:1261: uncaught here

Should we update things so this exception can work its way to the user code? If so am I going to have to update code for all of distributions (or is there some place farther up on the callstack I could do the check before we start getting into distribution specific code).

If not it seems like I'd be better off making this an error() rather than an exception.

Let me know your thoughts.

[bradcray]

@dlongnecke-cray: @bradcray suggested you might be a good person to reach out for feedback on this change, particularly in regards to how it might interact with arrays of different distributions.

David: What really made me suggest you might be good to look at this was all the work you did in the last year or two making sure that throw/catch was piped through our I/O routines appropriately. The errors Andy showed made me worry that either we or he didn't have something quite right, but I don't feel like I understand the architecture well enough to know why offhand, and I was looking for other experts to help out rather than looking into it more myself.

W.r.t. the "different distributions" concern, I originally had this concern, but once I understood better where Andy's code was (in a helper used by many / most rectangular arrays), I feel less concerned about it.

[dlongnecke-cray]

Should we update things so this exception can work its way to the user code?

I think that ultimately the error should be propagated out of the readf/writef call and sent to the user. The line numbers when CHPL_DEVELOPER=0 are indeed a bit deceptive in this case, so a halt is probably better for now if it's too much effort to get all distributions to throw consistently.

If so am I going to have to update code for all of distributions (or is there some place farther up on the callstack I could do the check before we start getting into distribution specific code).

I would try to modify array.writeThis() and array.readThis() directly before touching code in any specific distribution. I think you should have the tools you need to check for the rank of the array. My hope is that this will cover all the bases, and it also just seems like the appropriate place to put the checks considering that we're taking about something that affects every multidim array. I would write a test that runs through multidim arrays for a bunch of distributions just to make sure, though.

[bradcray]

I think that ultimately the error should be propagated out of the readf/writef call and sent to the user.

Is the implication that readf/writef don't throw today? For some reason, I thought we'd gotten all I/O into throwing form as part of your recent effort. [checks...] Oh wait, I'm suddenly remembering: Is it the case that:

• channel based readf/writef/writeln/etc. throw
• the standalone readf/writef/writeln don't (by virtue of being automatically available, not wanting to drag the world down with wrapping everything in try / catch, etc.?)

If so, something for Andy to try to see whether I'm remembering correctly would be to try stdout.writef(...) instead and see whether that throws the error all the way back to the user?

[dlongnecke-cray]

Thanks Brad, I was confused but just checked and it looks like the free function versions of readf/writef don't throw. It would be a good idea to do some stdout.writef just to make sure the errors propagate back up.

And because this internal machinery is also used within read/write as well, should we also check to make sure that write(ln) displays an error?

[bradcray]

And because this internal machinery is also used within read/write as well, should we also check to make sure that write(ln) displays an error?

The error only triggers for "print me out using Chapel syntax" formats on multidimensional arrays, and since writeln() doesn't use that format (and there's no way to cause it to that I'm aware of), I don't think it applies. Or at least I'm not expert enough in I/O to know how it would.

[stonea]

If so, something for Andy to try to see whether I'm remembering correctly would be to try stdout.writef(...) instead and see whether that throws the error all the way back to the user?

I just tried this and yes, if do stdout.writef(%ht\n", A) I'm able to catch the exception.

[bradcray]

That's a little bit reassuring. But then, @dlongnecke-cray , can you remind me: If an exception occurs within a writeln(), what do we do? Generate a similarly confusing "uncaught exception" error to the one Andy got above? Or does writeln() have some sort of catch that drops such cases on the ground? (in which case, should writef() have that as well?)

[dlongnecke-cray]

That's a little bit reassuring. But then, @dlongnecke-cray , can you remind me: If an exception occurs within a writeln(), what do we do?

I think we literally just do a:

  /* Equivalent to ``try! stdout.writeln``. See :proc:`IO.channel.writeln` */
  proc writeln(const args ...?n) {
    try! stdout.writeln((...args));
  }

I suggested to @stonea that we just leave things as is for now, but open up a followup issue to discuss the error strategy for the standalone functions. Write now (unintentional! :D) they are arguably somewhat confusing, since using try! presents an error message like:

uncaught IllegalArgumentError: Can not perform Chapel write of multidimensional array.
  foo.chpl:2: thrown here
  foo.chpl:2: uncaught here

Which suggests that they can be caught and handled, when they can't.

So maybe the answer to this is to have writeln and friends do a halt with a nice error message, or maybe the answer is to revisit the error message we print out when we hit a try!.

[dlongnecke-cray]

Looks good to me assuming tests pass!

[dlongnecke-cray]

If we haven't already, let's make a followup issue to discuss:

As is, I think the change I've made is an improvement, I'm still a little unsatisfied with it. Specifically, because even though I'm throwing an exception it isn't propagating up to the user code. The error message might lead users to thinking that they should update their code to catch an exception, but in fact.

[stonea]

@dlongnecke-cray , I've made an issue to follow up on the issue you mention:

#18296

I'm spending the next few days moving across the country and even though I think this is a pretty low-risk change re: making tests fail unexpectedly, I think I'll wait until I'm "back" to merge (which is before the end of the sprint \ code freeze).

[dlongnecke-cray]

Thanks for making that issue @stonea! I think it's fine to wait if you don't feel comfortable merging this before your move. Up to you.

[bradcray]

I think we literally just do a:

  /* Equivalent to ``try! stdout.writeln``. See :proc:`IO.channel.writeln` */
  proc writeln(const args ...?n) {
    try! stdout.writeln((...args));
  }

Ah, thanks. For some reason, I didn't interpret the error message as coming from a try! (and apparently was in too much of a tizzy that week to open up the code and see that there was one). I agree with your thoughts that maybe writeln() should be catching and printing its own error or that we should customize the try! error messages for cases that come from code outside of the user's control (particularly the internal modules).

Thanks for opening the new issue @stonea (which I saw before this response).