ballerina-platform / ballerina-spec

Ballerina Language and Platform Specifications
Other
167 stars 53 forks source link

Clarification on mapping constructor with spread field #1240

Closed chiranSachintha closed 7 months ago

chiranSachintha commented 1 year ago

Description: $title. we need to clarify whether the following scenario is correct or not.

import ballerina/io;

type Foo record {|
    int x = 10;
    int y = 10;
|};

public function main() {
    record {| int x; int y?; |} r = {x: 123};
    Foo f = {...r};
    io:println(f);
}

The reason for asking this is that the specification mentions it as,

If the inherent type descriptor is a record type descriptor, a field will be
added to the constructed value using the default value from the type descriptor
for any field that is not specified explicitly in the mapping constructor and
that has a default value.

IMO According to that, if we are going to add default values at compile time to mapping constructor, we won't know whether we have defined a value for y or not. Therefore, adding a default value for y at compile time is not correct. (Should give a compile-time error).

Another thing is that if we add default values at runtime, we can identify whether we need to add the default value for y or not.

jclark commented 1 year ago

If we allow this, then clearly the default for y would be used only when r does not have a y field.

I think in other cases you can always determine at compile time whether a default will be needed, whereas in this case the determination would have to be made at runtime. But is that a reason not to allow it?

I can see it's additional work to implement, but I think the expected behavior is clear, and I can imagine wanting to do this.

chiranSachintha commented 1 year ago

As a solution to this problem, we can desugar the above program in the following manner.

// generate closures

function () returns int foo_x = function () returns int {
    return xFn();
};

function () returns int foo_y = function () returns int {
    return yFn();
};
public function main() {
    record {| int x; int y?; |} r = {x: 123};
    //desugared code
    int? field_y;
    if (r.y is ()) { // To check whether the value given by the user at runtime,
        field_y = foo_y();
    } else {
        field_y = r.y;
    }

    Foo f = {...r, y: field_y};
    io:println(f);
}

any other suggestions or thoughts regarding this work? @jclark @MaryamZi @hasithaa

Desugaring becomes complex when there are multiple spread operators within the mapping constructor and multiple optional fields within those spread operators. The desugaring for these scenarios is described in the following document. https://docs.google.com/document/d/1YoNdn76VBC1tfQ0Qr_kmk74bYMNXknbjZ_XRUUdIu_0/edit?usp=sharing

chiranSachintha commented 1 year ago

As discussed with @MaryamZi , we have updated the solution to desugar the above program in the following manner.

// generate closures

function () returns int foo_x = function () returns int {
    return xFn();
};

function () returns int foo_y = function () returns int {
    return yFn();
};
public function main() {
  record {| int x; int y?; |} r = {x: 123};
  //desugared code
  record {| int x; int y?; |} spread_operator = {x: 123};
  if (r.y is ()) { // validate the presence of a value for field y
       spread_operator["y"] = foo_y();
  }

  Foo f = {...spread_operator};
  io:println(f);
}

More details are discussed in this comment https://docs.google.com/document/d/1YoNdn76VBC1tfQ0Qr_kmk74bYMNXknbjZ_XRUUdIu_0/edit?disco=AAAAz-g7jgY

any other suggestions or thoughts regarding this work?

MaryamZi commented 7 months ago

We've fixed the implementation to use the default only if a value is not provided for the field via a spread field, by introducing a runtime check.