Fix isolation analysis for start actions

[LakshanWeerasinghe]

Purpose

$subject

FIxes #41691

Related PR #41772

Approach

Describe how you are implementing the solutions along with the design details.

Samples

Provide high-level details about the samples related to this feature.

Remarks

This PR includes only fixing isolation analysis with start actions. The compiler crashes happens when passing mapping and list constructors will be fixed by #41772.

Check List

• Read the Contributing Guide
• Updated Change Log
• Checked Tooling Support (#)
• Added necessary tests
  • Unit Tests
  • Spec Conformance Tests
  • Integration Tests
  • Ballerina By Example Tests
• Increased Test Coverage
• Added necessary documentation
  • API documentation
  • Module documentation in Module.md files
  • Ballerina By Examples

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 76.73%. Comparing base (09e0471) to head (a22e04b).
Report is 114 commits behind head on master.

Additional details and impacted files

@@              Coverage Diff              @@
##             master   #42366       +/-   ##
=============================================
+ Coverage      0.00%   76.73%   +76.73%     
- Complexity        0    53738    +53738     
=============================================
  Files             9     2907     +2898     
  Lines            35   203156   +203121     
  Branches          0    26484    +26484     
=============================================
+ Hits              0   155883   +155883     
- Misses           35    38754    +38719     
- Partials          0     8519     +8519     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[MaryamZi]

We don't seem to be logging a warning for

import ballerina/http;

type UserRegistration record {
    string name;
    string email;
};

class Obj {
    int x = 1;

    function fn() {

    }
}

function getObj(Obj obj) returns Obj => obj;

service on new http:Listener(8080) {
    resource function get h1(UserRegistration user) {
        Obj obj = new;
        _ = start sendEmailNotification(getObj(obj));
    }
}

isolated function sendEmailNotification(object {} ob) returns error? {
}

[MaryamZi]

We don't seem to be logging a warning for

import ballerina/http;

type UserRegistration record {
    string name;
    string email;
};

class Obj {
    int x = 1;

    function fn() {

    }
}

function getObj(Obj obj) returns Obj => obj;

service on new http:Listener(8080) {
    resource function get h1(UserRegistration user) {
        Obj obj = new;
        _ = start sendEmailNotification(getObj(obj));
    }
}

isolated function sendEmailNotification(object {} ob) returns error? {
}

This isn't fixed yet?

[CLAassistant]

All committers have signed the CLA.

[LakshanWeerasinghe]

Fix issue with infering func isolation

Fixed in commit.

[MaryamZi]

Can't this happen outside the outer check?

[LakshanWeerasinghe]

At the outside of the outer check we analyse symbols with recordFieldDefaultValue and objectFieldDefaultValueRequiringIsolation being true. For those I don't think we need to check its a subtype of readonly or isolated object.

[MaryamZi]

But for those this.inIsolatedStartAction won't be true, right?

[LakshanWeerasinghe]

Yes. But outer was there to handle both true and false scenarios. We have add this new check to validate start actions specifically. Which wasn't being analysed previously.

[MaryamZi]

My point is if we can't do

if (this.inIsolatedStartAction
          && !isSubtypeOfReadOnlyOrIsolatedObjectOrInferableObject(symbol.owner, symbol.getType())) {
      inferredIsolated = false;
      return;
}

outside.

[LakshanWeerasinghe]

Then we need isReferenceToVarDefinedInSameInvokable check as well.

[MaryamZi]

Can you give an example for when we need that?

[LakshanWeerasinghe]

When I am running the test locally previously seems I haven't added the return statement. We can move this to the outside of the outer if. Updated in commit.

[MaryamZi]

Shouldn't we pass unresolvedSymbols here? Shall we add a test too?

[LakshanWeerasinghe]

The existing tests will cover this scenario.

[LakshanWeerasinghe]

Updated in commit.

[MaryamZi]

But that should have resulted in a stack overflow-ish error, right? If we are recursively checking the isolated-ness of the same construct?

[MaryamZi]

Example:

function fn(any x = ()) returns any {
    _ = start fn(fn());
}

[LakshanWeerasinghe]

Thanks!
When we don't pass unresolved symbols, for the provided sample it gives SO.