Remove unnecessary casting for record field set

[heshanpadmasiri]

Purpose

Remove unnecessary casting for record field set

Fixes #41876
Depends on #41998

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

List any other known issues, related PRs, TODO items, or any other notes related to the PR.

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.82%. Comparing base (09e0471) to head (6551fb8).
Report is 162 commits behind head on master.

Additional details and impacted files

@@              Coverage Diff              @@
##             master   #41917       +/-   ##
=============================================
+ Coverage      0.00%   76.82%   +76.82%     
- Complexity        0    53968    +53968     
=============================================
  Files             9     2924     +2915     
  Lines            35   203890   +203855     
  Branches          0    26589    +26589     
=============================================
+ Hits              0   156631   +156631     
- Misses           35    38720    +38685     
- Partials          0     8539     +8539     

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

[github-actions]

This PR has been open for more than 15 days with no activity. This will be closed in 3 days unless the stale label is removed or commented.

[chiranSachintha]

Still for following kind of scenarios we create the unnecessary cast

type R record {|
    int f;
|};

function foo() {
    R|R r = { f: 5 };
    r.f = 6;
}

[chiranSachintha]

When doing the checkExpr, we set the impConversionExpr at the same time. With this logic, what we did is we set it and then reset it with the use of resetImpConversionExpr. IMO, it is not correct, and we should avoid the setting of impConversionExpr for scenarios which don't need.

[heshanpadmasiri]

The reason why I decided to do this was because data.typeChecker.checkExpr uses the expected type for doing 2 things in the end here,

1. Check if the actual type is a subtype of the expected type and if not give an error
2. Add an implicit cast if needed

When it comes to optional field assignments for 1 it must treat expected type for a field type T as T? and for 2 it must treat it as just T. But since we are type checking the expr part of the assignment, which don't have information about what it is being used for currently, and there for we can't distinguish between these expected types. I think there are 2 ways to actually fix this,

1. Add information directly to BLangExpression node that it is being used in such a manner and in the type check we treat expected type differently for 1 and 2 (this is done in #272284f). While this requires the minimum amount of change this means we are leaking information about the parent node to the child nodes in the AST, which I think is bad
2. Speical case the flow for the type checker when we have an optional field assignement. This would mean we either have to change the public API of the type checker such that we can pass information from here all the way down to checkType, or pass the informatio using AnalyzerData by giving a anther API to be used in case of optional field access similar to checkExpr. This would still means each expression node must do the right thing by either calling an appropriately modified version of checkType or passing on this information to checkType by modifying the public API.

I feel 2 is the "correct" solution but given the way our code is structured this means a very large modification. Option 1 (and setting and resetting the implConversionExpr) both require minimal changes but are "incorrect" in different ways. I am not sure which of these 3 possible options is the best. WDYT?

[chiranSachintha]

Why we need field != null check? As this is a field based access expression field cannot be null.

[heshanpadmasiri]

Added this because I got the field as null when trying to build the Ballerina langlib. Will investigate and create a issue for that.

[MaryamZi]

Can we get rid of this now?

[heshanpadmasiri]

It seems a new regression has been introduced since #41998, while trying to desugar this query expression. Will try to find a simple reproducible case and make a separate issue

[heshanpadmasiri]

This is caused by a preexisting bug in query de-sugar. Recent changes has surfaced it. Created #42479

[heshanpadmasiri]

Still for following kind of scenarios we create the unnecessary cast

type R record {|
    int f;
|};

function foo() {
    R|R r = { f: 5 };
    r.f = 6;
}

I think this is a different issue from #41876 since we are not casting the value being assigned (ie 6) but R. And if I am not mistaken we do this whenever we refer to r irrespective of whether we are dealing with an assignment or not. May I create a separate issue for this because I suspect this has do with current type system not being able to reduce R|R to R (I expect this to fix itself with the semtype introduction)

[chiranSachintha]

@heshanpadmasiri Ubuntu and windows build are failing. Please check