Further clarify array element error messages w.r.t. generic class management

[bradcray]

Summary of Problem

As a Chapel user, it can sometimes be confusing when a class is generic by nature vs. by omission of any explicit memory management indicator. For example, this weekend, a user was stymied by this example:

class column {
    var x:int;
}

record table {
    var columns: [0..<10] column;
}

var data = new table();

whose error message was:

code.chpl:6: warning: creating an array with element type column
code.chpl:6: warning: which now means class type with generic management
code.chpl:6: error: array element type cannot currently be generic

Their interpretation was that something about column itself must be generic, and that having a generic memory management declaration was acceptable but simply frowned upon. Is there something more we could be doing in this error message's wording to make it clearer where the problem lies?

Configuration Information

• Output of chpl --version: 1.29.0

[mppf]

I think we could remove the "which now means" part and suggest "perhaps you meant to use 'owned column' or 'shared column'" or something like that.

[mppf]

also the title of this issue is pretty broad but the example is specifically about the array-element-type-is-generic error. Did you mean for the title to cover just the array-element-type case?

[bradcray]

I think we could remove the "which now means" part and suggest "perhaps you meant to use 'owned column' or 'shared column'" or something like that.

Good point about "now means" which feels dated at this point (though it hadn't occurred to me). I like the idea of pointing the user in a potential productive direction, which also matches the philosophy of the new error message framework. "Perhaps you meant" sounds passive-aggressive or presumptuous to me, but something like "For instance, declaring your array elements as owned C would help with this."

I also wondered whether we could (easily) distinguish between whether the class was generic and/or whether no management type was specified in order to say something like:

error:arrays cannot be created with generic elements
note: this array's elements are generic because ... [and then one of:]
though `C` is a concrete class, it has no specified memory management type
C is a generic class
C is a generic class and it has no declared memory management type

Did you mean for the title to cover just the array-element-type case?

It wouldn't surprise me if there were other similar cases, but to make this issue close-able, I've reworded it to try and focus in on this case.

[kwaters4]

@bradcray @mppf I was going to file another issue, however I think this belongs here as well. Let me know if you think this should be a separate issue. I think I am fuzzing your library API.

Summary of Problem

In my quest to understand classes and records I stumbled onto this series of messages. I have been hitting a few internal library errors, here is one example. I think I understand the issue now and have found a correct (param x:int -> var x:int) solution around the problem. I am pretty sure if this should not be allowed allowed and caught at the library API.

Steps to Reproduce

Source Code:

param entries:int = 4;

record table {
    param entries;
    forwarding var columns: [0..entries-1] shared column;

    proc init(param entries:int) {
        this.entries = entries;
    this.columns = [i in 0..entries-1] new shared column(entries);
    }
}

class column {
    param x:int;
}

var data = new table(entries);
writeln(data);

Error Messages:

/ATO/code.chpl:7: In initializer:
/ATO/code.chpl:5: warning: creating an array with element type shared column
  /ATO/code.chpl:17: called as table.init(param entries = 4)
$CHPL_HOME/modules/internal/ChapelDomain.chpl:1325: In method 'buildArray':
$CHPL_HOME/modules/internal/ChapelDomain.chpl:1332: error: duplicate decorators - borrowed shared column
  $CHPL_HOME/modules/internal/ChapelArray.chpl:223: called as (domain(1,int(64),false)).buildArray(type eltType = shared column, param initElts = false)
  within internal functions (use --print-callstack-on-error to see)
  /ATO/code.chpl:17: called as table.init(param entries = 4)
/ATO/code.chpl:7: In initializer:
/ATO/code.chpl:5: error: array element type cannot currently be generic
  /ATO/code.chpl:17: called as table.init(param entries = 4)

Compile command: chpl foo.chpl

[bradcray]

@kwaters4 : This one both is, and is not, related to the discussion we were having on slack. The part that is related is that column is a generic class by virtue of its param x: int; field: Since params must be known at compile-time, C(1) is a different type from C(2). So to get this to compile, you'd need to make the array's element type concrete by writing:

...
forwarding var columns: [0..entries-1] shared column(entries);
...

at which point things work, as shown here, on ATO.

The part that's not related is that that error about:

$CHPL_HOME/modules/internal/ChapelDomain.chpl:1332: error: duplicate decorators - borrowed shared column

, which is a complete bug on our part. Fortunately, I believe it has a 5-minute fix (TM). With that fix, I get the error:

testit.chpl:5: warning: creating an array with element type shared C
testit.chpl:5: error: array element type cannot currently be generic

If this error still does not seem clear enough (where I think that argument could be made), then I'd suggest firing off a separate issue proposing a new wording (it doesn't quite fit on this one because in this case the class is generic due to the class declaration itself and not the generic class management aspect).

Finally, note that there's (currently, at least), no benefit in making an array's size a param rather than a const or var, so if I were reviewing your code for Chapel style, I'd probably suggest downgrading the param to a const, which has the benefits of making the code less generic, so also simpler.

[bradcray]

Proposed fix for the error that was ours here: https://github.com/chapel-lang/chapel/pull/21428

[bradcray]

And that fix is now merged thanks to a quick review from @mppf. I'm going to collapse the last few messages to indicate that, though @kwaters4, I want to reiterate that if you have thoughts about how the rest of the error message could be clarified, I think that's still a completely valid issue to file.

[bradcray]

@mppf: Since my last PR bumped me up against where those "which now means..." error wordings occur, I was thinking about doing the simple update of changing them to "which means" unless you're already running with that?

[edit: or maybe "which indicates a class type with..."]

[mppf]

@bradcray - no I'm not working on that & I'm happy to see the improvement

[bradcray]

Looking at this again for the first time in a few weeks, I'm thinking that there's overlap between what's being asked for here and what @dlongnecke-cray has started on in https://github.com/chapel-lang/chapel/pull/21720 and staked out future ideas for in https://github.com/Cray/chapel-private/issues/4443

[bradcray]

21776 did a modest rewording to help here, though more could still be done.