chapel-lang / chapel

a Productive Parallel Programming Language
https://chapel-lang.org
Other
1.8k stars 421 forks source link

Expand support for type aliases of concrete-ized generic types #5143

Open nspark opened 7 years ago

nspark commented 7 years ago

I've been trying out some ideas with generic records and classes, and ran into a few issues. I saw #3406 and some of the associated examples, and wanted to see whether the compiler could handle what I was trying to do. I thought filing an issue here might help me document what I'm trying to do.

Part 1: A problem with type aliases of generic types

After looking at this example and experimenting with a few variants, I noticed that the generic type specified by the alias is not actually respected.

The following updated test shows that the constructor argument of "1" is what determines the type of A.x, not the type alias:

record A {
  var x;
}

type C = A(real);

var c = new C(1);
if (c.x.type != real) then compilerError("Inner type not respected by alias!");
writeln(c);
writeln(c.x);

Part 2: Generic containers with type fields

Update 11/8/2017: Added Parts 2a and 2b; updated the old "Part 2" to Part 2c.

Part 2a: Records with explicit initializers

To my surprise, this code works!

record Generic {
  type eltType;
  var value: eltType;

  proc init(value: ?eltType) {
    this.eltType = eltType;
    this.value   = value;
    super.init();
  }
}

type Concrete = Generic(string);

var foo = new Concrete("hello");
writeln(foo);

With the output:

$ chpl part2a.chpl && ./part2a
(value = hello)

This Generic record even produces errors as expected (unlike Part 1):

type Concrete = Generic(int);
var foo = new Concrete("hello");

Produces output:

$ chpl part2a.chpl && ./part2a
part2a.chpl:5: In initializer:
part2a.chpl:6: error: cannot assign expression of type string to field 'eltType' of type int(64)

Part 2b: Classes with explicit initializers

Unfortunately, adapting Part 2a from a record to class type does not work:

class Generic {
  type eltType;
  var value: eltType;

  proc init(value: ?eltType) {
    this.eltType = eltType;
    this.value   = value;
    super.init();
  }
}

type Concrete = Generic(string);

var foo = new Concrete("hello");
writeln(foo);
delete foo;

Which produces the output:

$ chpl part2b.chpl && ./part2b
part2b.chpl:14: error: unresolved call '_new(type Generic(string), "hello")'

Part 2c: Records without explicit initializers

Assuming that Part 1 can be resolved, I'm hoping to work with generic types that have a type field. Again comparing to this example, I'm hoping to have something like:

record Generic {
  type eltType;
  var value: eltType;
}

type Concrete = Generic(string);

var foo = new Concrete("hello");
writeln(foo);

which currently produces the following error message:

$ chpl part2c.chpl && ./part2c
part2c.chpl:8: error: unresolved call 'Generic.init("hello")'
part2c.chpl:1: note: candidates are: Generic.init(type eltType, value: eltType)

Part 3: Forwarding constructor arguments

Ultimately, I want a type alias that concrete-izes a generic record, where the "inner" type is a class, and the constructor arguments to the type alias get forwarded to the class's constructor. I think this would look something like:

class B {
  var b1: real;
  var b2: int;
  proc B(v1: real, v2: int) {
    b1 = v1;
    b2 = v2;
  }
}

record A {
  type T;
  var a: T;
  proc A(args...) {
    a = new T((...args));
  }
}

type C = A(B);

var c = new C(99.0, 42);
writeln(c);
writeln(c.a);
writeln(c.T:string); // prints B
writeln(c.a.b1);     // prints 99.0
writeln(c.a.b2);     // prints 42
bradcray commented 7 years ago

@mppf: Tagging you to make sure you see this issue.

mppf commented 7 years ago

I tried making some simple modifications to function resolution to address Part 1 and Part 2, but the result was something that was very fragile - it got Part 1 to work as-written but wouldn't work if there were any user-defined constructors.

I expect that the example in Part 1 should work once we get the compiler-generated initializer using the new style. It is challenging to make it work correctly with the current old-style compiler-generated constructor. (The problem is that the old-style ctor doesn't pass the type to be constructed in - but the new-style will do that as part of the "meme" argument).

For the example in Part 2, if you added

proc A.A(a, T=a.type) { this.a = a; }

we would have that example reduced to the case in Part 1. (And there will be a way to do this with new-style initializers too). The issue there is that the initializer for C takes in two arguments: type T and value a. With the new-style initializers, there won't necessarily be the trivial relationship between type fields and type arguments in the constructor call. Except for the compiler-generated constructor where they would be the same. So I could get exactly what you wrote in Part 2 to work for compiler-generated constructors if we basically had the compiler add the int argument to the new C call. My attempt at implementing that would make new C(int, 1) no longer work, which I think is problematic. I'm thinking it's probably a better idea to just write the 1-argument initializer anyway. Adding the 1-argument initializer would let you write new A(1) as well.

For Part 3, you might be interested in PR #5058 which is a start at adding a forwarding feature. The main thing that effort needs is further design discussion. In particular it needs more agreement on the general direction of the feature and also on the name 'delegate'. I got pretty far in implementing it but have set it aside for now as I no longer view it as necessary for writing things like reference-counting records to avoid needing to write delete.

bradcray commented 7 years ago

Tagging @lydia-duncan on this issue due to the relationship to initializers and particularly generic initializers.

lydia-duncan commented 7 years ago

(lurking, nothing to add just yet)

ben-albrecht commented 7 years ago

For now, this is issue blocked by initializers development. For lack of a better label, I marked this aswork in progress to indicate this.

lydia-duncan commented 7 years ago

Tracking for the initializers implementation lives in #5198

ben-albrecht commented 7 years ago

As an additional datapoint -- this feature would be useful toLayoutCS (#6936) by enabling a less verbose form of CSC:

type CSC = CS(compressRows=false);
nspark commented 7 years ago

Poke @lydia-duncan @bradcray and @mppf due to the update in Part 2.

lydia-duncan commented 7 years ago

Ah, excellent that 2a works! That's interesting about 2b - my suspicion is that the problem is due to our reliance on the generation of the hidden _new function for classes when a relevant initializer is called and that I probably put some code in to circumvent the generation of that function when the type is fully known (which this example proves I should not have done).

(pinging @noakesmichael so that he is aware of this, too)

lydia-duncan commented 7 years ago

I'll add the 2a and 2b variants to our test system.

lydia-duncan commented 6 years ago

8188 resolves case 2b

lydia-duncan commented 6 years ago

Note that it does not change the behavior of case 1

cassella commented 5 years ago

1:

test/classes/initializers/generics/typeAlias/errorNewExpr.chpl declares the failure of this to compile to be errorNewExpr.typeless.good.

2a, 2b:

I believe #11656 locks in these not working as written. For the new expression using the alias to invoke the initializer, the initializer now needs a type formal argument with the same name as the type member.

2b: was test/classes/initializers/generics/typeAlias.chpl

11656 removed it.

2a: was test/classes/initializers/records/generics/assignment/typeAlias.chpl

11656 added a type eltType arg to the initializer.

2a-2: fails for the same reason as 2a, before detecting the mismatch.

2c:

This works,

test/classes/ferguson/record-alias-generic-concrete.chpl test/classes/initializers/compilerGenerated/friendly_generics.chpl

(The latter pending #12693)

3:

Following the example of 2a, I was able to make this work by adding an explicit type T argument to A's initializer,

class B {
  var b1: real;
  var b2: int;
  proc init(v1: real, v2: int) {
    b1 = v1;
    b2 = v2;
  }
}

record A {
  type T;
  var a: owned T;
  proc init(type T, args...) {
    this.T = T;
    a = new owned T((...args));
  }
}

type C = A(B);

var c = new C(99.0, 42);
writeln(c);
writeln(c.a);
writeln(c.T:string); // prints B
writeln(c.a.b1);     // prints 99.0
writeln(c.a.b2);     // prints 42

It feels a little fragile though. I don't know if this is different enough from the existing generic+alias+initializer tests to add a new one.

lydia-duncan commented 5 years ago

Thanks Paul! I need to mentally refresh myself on the issue before determining what the next step is (not sure if I'll get to it right away, but hopefully soon)