-
Notifications
You must be signed in to change notification settings - Fork 419
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
initializers and typeless or generic fields #16508
Comments
Here is more detail about the "doesn't work yet on master" comment: record R {
var x:numeric = 1;
proc init() {
this.x = 1.0;
}
}
var a:R;
writeln(a);
writeln(isGenericType(a.type));
writeln(a, ": ", a.type:string, " field type ", a.x.type:string);
I ran into this when working on #16030 and the pattern above is inspired by something we have currently on master in Treap.chpl. |
It is appealing to imagine that the user could write this: record GenericTypedFieldWithInit {
var f:numeric = 1;
proc init() {
this.f = 1.0;
}
proc init(f:numeric) {
this.f = 1;
}
} and then the compiler could compute the initialization expression |
Improve compiler's checking for resetting an instantiated field This PR adjusts `resolveInitField` in function resolution to check if a generic field already provided a type is being reset. This provides a good error message in cases such as described in issue #16030. In order to get that to work, I also needed to update `AggregateType::fieldIsGeneric` and `AggregateType::generateType` to better handle the case of a field with a generic type and an initialization expression. This PR adds a better error for the case in which a record field is declared with a generic type and an initialization expression. In that event, there are open language design questions about how to default initialize a variable of that type. See #16508. While there, this PR removes the `decayToBorrow` argument and functionality from several `resolveGenericActuals` functions as it is no longer necessary now that we can pass generic type arguments. It also replaces a strcmp on astrs with a pointer comparison. Resolves #16030. Helps with, but does not fully resolve, #16044. Reviewed by @e-kayrakli - thanks! - [x] primers pass with verify+valgrind and do not leak - [x] full local futures testing
My current thinking is: 1. A BugThere is a bug in that code like this record TypelessField {
var field;
proc init(...) { ... }
}
type TypelessFieldInt = TypelessField(int);
var b: TypelessFieldInit; in that the compiler tries to translate it into var defaultTmp: int;
var b = new TypelessFieldInt(field=defaultTmp); 2. How to handle typeless fieldsThe compiler will continue to use named arguments for Note to self - I made a slide deck about this for the 1.23 release notes that we decided not to include. But it presents the above two points (as well as alternative designs). |
See issue chapel-lang#16508. In the future we would like 'this.type' to convey the type information for a typeless field as in record TypelessField { var field; proc init() { /* this.type.field indicates int in the below case */ } } type TypelessFieldInt = TypelessField(int); var b: TypelessFieldInit; --- Signed-off-by: Michael Ferguson <[email protected]>
See issue chapel-lang#16508. In the future we would like 'this.type' to convey the type information for a typeless field as in record TypelessField { var field; proc init() { /* this.type.field indicates int in the below case */ } } type TypelessFieldInt = TypelessField(int); var b: TypelessFieldInit; --- Signed-off-by: Michael Ferguson <[email protected]>
Prohibit default initialization for records with typeless fields and user initializers This PR adjusts the compiler so that the bug described in #16508 emits a compilation error instead of unexpected behavior. The goal here is to allow further improvements to address #16508 to happen without breaking existing code that compiles. In particular, for this code: ``` chapel record TypelessField { var field; proc init() { field = 1.0; writeln("init()"); } proc init(field) { this.field = field; writeln("init(field)"); } } type TypelessFieldInt = TypelessField(int); var b: TypelessFieldInt; writeln(b.field, ": ", b.field.type:string); ``` Before this PR, the compiler would rewrite the default initialization of `b` as: ``` chapel var defaultTmp: int; var b = new TypelessFieldInt(field=defaultTmp); ``` (As a result, it would compile before this PR and print "init(field)"). However this is surprising (Shouldn't `init()` be called?) and counter to the design of initializers (because the record author doesn't have an opportunity to control the default value of `field` in the default initialization). Instead of calling "init(field)", we expect this program to fail to compile because the default initialization would run `init()` and encounter a type mismatch. To get that working nicely will require using `this.type` to convey the type information in this case (see also #16508). This PR puts off that effort in favor of prohibiting the pattern that would change in behavior. In particular, this PR makes it an error "this default-initialization is not yet supported". This error is demonstrated in the new test issue-16508-user.chpl. This PR allows types that use the compiler-generated default initializer to continue to use the default value for the field type when default-initializing since that does not present stability problems with the change planned in #16508. This is demonstrated in the new test, issue-16508-default-ok.chpl. Besides adding the error, this PR uses a combination of adding workarounds and updating tests to be futures. I added workarounds for tests that seemed to have a goal different from checking this pattern. Also note that reuse-init-instantiation-var.chpl was the same as reuse-init-new-instantiation-var.chpl so this PR removes it. - [x] full local futures testing Reviewed by @lydia-duncan - thanks!
Potentially related: #18664 |
Following up on - #16508 (comment) - I have some discomfort that the design proposed there spans two different approaches (one for |
Regarding the comment #16508 (comment) -- the "bug" is what makes this example compile and run: record R {
var field;
}
var x: R(int); // compiles and runs by effectively calling R.init(0)
writeln(x); But, record R {
var field;
// What signature to write if I want a custom default initializer?
proc init(type field) {
var tmp: field;
this.field = tmp;
}
}
var x: R(int); // fails to compile
writeln(x); This second example gives an error about it not being supported yet:
Here is another example of trying to write a custom default initializer that gives a similar error: record R {
var field;
// What signature to write if I want a custom initializer?
proc init() {
var tmp: this.type.field;
this.field = tmp;
}
}
var x: R(int);
writeln(x); |
I propose that, in the context of the previous comment, we switch from calling The compiler would generate these initializers, or they can be provided by the user:
Why not
|
That specific statement does not make sense to me. If the field is switched from untyped to concretely typed, then the function called for default initialization will change from
I don't think we should do that because it creates busy-work for users who want to make their types as flexible as possible. I also wanted to add that the combinatorial explosion problem also applies when partial instantiation is used. Lastly, I was wondering about the situation with the example record we discussed yesterday, since it appears that the type constructor does use record R {
type T; // ‘type’ field
param p; // ‘param’ field
var v; // untyped ‘var’ or ‘const’ field
var i: integral; // a ‘var’ or ‘const’ field
// whose type is generic
var j: int; // cf. this does not make it generic
}
type t = R(v=bool);
var x = new t(T=int, p=4, i=32, j=5);
// error: A type alias of 'R' may not be used in a new-expression because it contains a typeless field ('v')
// this does work, but the compiler is computing the default values
// and passing them to the initializer, as discussed above
type tt = R(T=int, p=3, v=real, i=uint);
var y: tt; |
In this case, perhaps the approach we have today where the compiler fills in the default values is good? The downside is that it does not allow the initializer to discern whether the actual argument was a compiler-provided default or a user-supplied value. So perhaps we can make this scheme an opt-in?
Good point. I need to think more about it. |
The user should be able to provide the default value by writing their own initializer (with some signature). The approach today doesn't have that property. |
Probably just an aside, but if it matters to this conversation, there's been a long-term desire to add language support for distinguishing between these cases (e.g., "In |
Something I was musing about this week w.r.t. the challenge case in #16508 (comment) was whether when the language sees: record R {
var field;
} it should no longer create a type constructor/initializer with the signature var x: R(int); they would get an error like "R(int) is not a legal type signature" (maybe w/ the potential bonus "only var x: R(?);
x = new R(42); if they wanted to specify a type signature and do split initialization. Basically, record R2 {
type t;
param p: int;
var x;
} type signatures like While the "implicit type field for the type-less variable" idea that we currently have is cute and seemed sensible, it's also subtle. And if it leads to weirdness like we're seeing, it doesn't seem worth preserving. In contrast, this proposal has the benefit of reflecting more directly/simply what the type's fields are, and puts the burden back on the type author if they want to support a complete/concrete type. The only drawback I can think of is whether users would trip over this more often than desired/intended. For example, if I wrote: record R {
var c: MyConcreteClass; // /generic class management
var c2: MyGenericClass(?); // + generic class
var a: [] int; // no idea what kind of domain map this array is
} it seems the presence of such fields, being generic, would also result in my needing to type |
@bradcray - I interpret your proposal as "Some features don't work with typeless fields" which would be fine with me. To me, this not that different in the solution space from issuing a warning for fields like Another thing I appreciate about your proposal is that it does not require us to solve user-defined type constructors to make progress on this specific issue. So, I support your proposal. Of course, we will need to do some testing to understand whether or not we are underestimating the impact on user code / realistic code. A nit:
That is not what we have today. Today, the record genericRecord {
var field;
}
// default-initialize with field=int
var x: genericRecord(int);
writeln(x , " ", x.type:string);
// using named-argument passing works too, with the name 'field':
var y: genericRecord(field=int);
writeln(y , " ", y.type:string); That said:
I think the idea is subtle whether the type constructor is |
@jhh67 brought up the idea that one could use record R {
var x;
}
var c: R(int);
// calls R.init(x.type=int)
// so you could write
proc R.init(type x.type) {
x = 4; // or whatever makes sense
} |
I've been looking at not allowing type construction for records like What could we do instead? I can think of two possible directions. With either of these strategies, I think we could, in the future, add support for Before getting into the two possible directions, I'd like to restate, what problem are we solving here? Restatement of the ProblemsI think that there are two problems. Not possible to write a custom initializer to respond to an instantiated type with typeless fieldFor example, we might try to write it this way: record U {
var genericField;
proc init(type genericField) {
var tmp: genericField; // default initialize
this.genericField = tmp;
}
proc init(in genericField) {
this.genericField = genericField;
}
}
var x: U(int); // error here, currently
writeln(x, " : ", x.type:string); However, this does not compile. It gives
Default initialization for the compiler-generated initializer is worryingGiven that the above does not work, it seems worrying that the same pattern does work with the default initializer: record R { var genericField; }
var x: R(int); // ok
writeln(x, " : ", x.type:string); The compiler generated initializer for this record has the signature Potential PathsDo not allow default initialization of such types for nowCurrently, default initialization is not allowed for such types if there is a user-provided initializer. We could extend this restriction to types with compiler-generated initializers. That would make the two cases consistent. If this is not allowed for now, then in the future, we can allow this only for user-defined initializers that meet certain constraints. In particular, we might require use of Enable the default initialization for user-provided initializatersAs a convenient way to address the combinatorial explosion, we could extend the current behavior to user-provided initializers, under certain circumstances. For example: record U {
var genericField;
proc init(in genericField) {
this.genericField = genericField;
}
}
var x: U(int); // this could default initialize an 'int' and pass it as the value for the initializer's 'genericField' formal
writeln(x, " : ", x.type:string); This could be really convenient (since it's hard to imagine what else a user-provided initializer would do in such a case) but it has two drawbacks:
|
I've created a PR to explore this option: #22836. |
I'm now caught up on this latest comment, but am not sure I fully understand everything about it (and may benefit from an interactive discussion). Thanks for starting by recapping the problems, and I haven't taken the time to re-read the whole thread, so apologize if I'm repeating earlier comments or questions as a result (this issue refuses to stick in my brain). I'll type my questions/thoughts/notes here, though you're welcome to reply in person if it's more efficient. In your first example: record U {
var genericField;
proc init(type genericField) {
var tmp: genericField; // default initialize
this.genericField = tmp;
}
proc init(in genericField) {
this.genericField = genericField;
}
}
var x: U(int); // error here, currently
writeln(x, " : ", x.type:string); what would we expect/hope that this code would do? Guessing at the answer:
So the current behavior seems reasonable to me (and I think you're saying that it's not unreasonable, just that it's not aligned with the all-compiler-generated behavior / doesn't support the user's ability to write similar code to the compiler?). So arguably the problem is that if we try to write the 0-argument initializer, we might try this: proc init(in genericField = defaultOf(this.genericField.type)) {
this.genericField = genericField;
} but this doesn't work because the argument list can't refer to proc init() {
var tmp: this.genericField.type;
this.genericField = tmp;
} then we're trying to refer to a field before it's declared. And if we do this: proc init() {
var tmp: this.type.genericField;
this.genericField = tmp;
} then we have the problem that we can't currently refer to Interestingly, changing the declaration to Actually, maybe that's my only [series of] questions...? |
Right, but I don't think the type construction process is particularly relevant here.
I see it differently. Today, we have this strategy that we use named expressions to pass the type/param components of an instantiated type to the record R {
type T;
var x : T;
}
var x: R(int);
// compiler translates it into var x = new R(T=int) The technote linked above says:
This is what we are facing in this issue. IMO if we turn off the special behavior for the compiler-generated default initializer, we can create an additive / non-breaking change in the future to enable user-defined initializers in such cases to accept the type. I think the most obvious strategy (given our other language design choices) would be for the signature for such an initializer to be what I showed above record U {
var genericField;
proc init(type genericField) { ... } // describe how to default initialize R(int) etc
} I think we should consider other options as well (including having the compiler try to default-initialze and pass to a |
For #16508. This PR changes the behavior of examples like the below, when the type uses a compiler-generated default initializer: ``` chapel record R { var genericField; } var x: R(int); // was OK; now error record U { var genericField: integral; } var x: U(int); // was OK; now error ``` Historically, when faced with a default initialization like `var x: R(int)` above, the compiler would default initialize an `int` to pass as the value for `genericField`. However, it is worrying that this pattern default-initializes something without it being clear that the user has requested it. A further concern is that the default initializer should be possible for users to write, but this is a case where a user cannot write it. Additionally, a similar pattern was already producing a compilation error if the type has user-provided initializers. That makes sense, because it would be reasonable for a type author to assume that they could customize the default value of such a field, when in fact today, they cannot. This PR changes the compiler to emit an error for default initialization of a record using the compiler-generated initializer a typeless or generic value field. The new error does not apply to `type` or `param` fields. Note that such patterns already generated an error when the type did not rely on the compiler-generated initializer. So, this PR makes this error apply also to the case of compiler-generated initializers. Reviewed by @vasslitvinov - thanks! - [x] full comm=none testing
Closing. PR #22836 removed the special behavior for default initialization with |
Follow-on to #12318 and issue #16044.
Issue #12318 discusses a language design direction that can handle typeless fields better.
We still have problems with the language design in this area. The main problems are around type aliases (and/or partial instantiations) and typeless fields.
For example:
According to #12318, initializer authors will use two features to be able to write an
init
that responds to the various possibilities:this.type
to query elements of the type. This is already possible ininit=
functions but not yet ininit
However we currently have implemented a pass-type-alias-arguments-as-named-arguments-to-initializers strategy (see https://chapel-lang.org/docs/master/technotes/initTypeAlias.html). Does a user trying to write a
proc init
forGenericTypedFieldWithInit
above need to create one with signatureproc init(type f)
for the compiler to pass the type off
into? That seems strange since the fieldf
is not a type but rather a value. Should this information be passed exclusively throughthis.type
- and in that event do we need to change something about the pass-type-alias-arguments-as-named-arguments-to-initializers strategy? Or, should the compiler try to callproc init(type fType)
or something along those lines?Update: My current understanding is that we can add
this.type
conveying the types for these typeless fields as a change that preserves compatability (i.e. is not a breaking change).The text was updated successfully, but these errors were encountered: