ballerina-platform / ballerina-spec

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

Invalid update of restricted field within the `init` method of an `isolated` object #1243

Closed MaryamZi closed 1 year ago

MaryamZi commented 1 year ago

Description: The spec says

6.8.1.5 Initialization

If the object-constructor-expr is explicitly isolated, then a field can only be assigned to by an assignment-stmt with a right hand side that is an isolated expression. The requirement to access self within a lock statement does not apply to the init method.

But validating updates of the fields (specifically transferring values in) where it isn't a direct assignment to a field is tied to the lock statement, right?

The jBallerina implementation seem to be allowing updates that can violate the isolated root invariant via langlib functions (but also updates with member-access-lvexpr since we seem to be checking only assignments to the field directly - depends on the interpretation of "a field can only be assigned to") in the init method at the moment.

type Node record {|
    readonly string path;
    map<string[]>? app = ();
|};

isolated class Class {
    private Node node;
    private Node[] arr = [];

    function init(Node node) {
        self.arr.push(node); // allowed at the moment
        self.arr[0] = node; // allowed at the moment

        self.node = node; // error
    }

    function fn(Node node) {
        lock {
            // errors for all three
            self.arr.push(node);
            self.arr[0] = node;
            self.node = node;
        }
    }
}

I'm not sure the spec disallows the invalid update (at least via the langlib function) at the moment?

jclark commented 1 year ago

If the object-constructor-expr is explicitly isolated, then a field can only be assigned to by an assignment-stmt with a right hand side that is an isolated expression. The requirement to access self within a lock statement does not apply to the init method.

My initial thought is that the second sentence is too loose. We only want the second sentence to apply when using self as described in the first sentence. If you are using self in init to access an already initialized field, then we need the same restrictions as usual.

jclark commented 1 year ago

It need to tie into the provision of https://ballerina.io/spec/lang/2022R4/#section_6.8.1.5 that says that it is a compile error if the method uses the self variable other than to access or modify the value of a field at a point where there is any potentially uninitialized field.

jclark commented 1 year ago

The simplest fix would be to change this

The requirement to access self within a lock statement does not apply to the init method.

to say

The requirement to access self within a lock statement does not apply to the init method at any point where there is any potentially uninitialized field.

This intuitively makes sense, since once all the fields are known to be initialized, init works like any other method.

MaryamZi commented 1 year ago

Just to cover the different scenarios, with

The requirement to access self within a lock statement does not apply to the init method at any point where there is any potentially uninitialized field.

for

isolated class Class {

    private int i = 0;

    function init() {
        self.i = 0; // 1

        lock {
            fn(self);
        }

        self.i = 1; // 2
    }
}

isolated function fn(Class cl) {

}

we would require using a lock statement at 2 also, right, even though it isn't really required?

jclark commented 1 year ago

we would require using a lock statement at 2 also

RIght. This keeps things as simple as possible.