[Bug]: Use of on fail can lead to use of an uninitilized variable at runtime

[buehlefs]

Description

When a variable is only initialized in a transaction or do block and errors are caught by an on fail block ballerina will assume the variable is always initialized after the on fail block even though the variable may not be initilized in case of an error.

At runtime this will either result in a null pointer exception (the good case actually) or the use of default values (e.g. 0 for integers) which can lead to very hard to detect bugs!

Instead, the compiler should throw a compilation error for possibly undefined variables.

Steps to Reproduce

    int resultInt;
    transaction {
        resultInt = check maybeProvideInt(true);
        check commit;
    } on fail {
        io:println("Failed to initialize resultInt");
    }
    resultInt += 1;
    io:println(resultInt);  // should always print 42 but can output 1!

Full example code to reproduce issue (click to expand)

import ballerina/io;

type TestObject record {int number;};

public isolated function maybeProvideObject(boolean throwError) returns TestObject|error {
    if throwError {
        return error("Demo error.");
    }
    return {number: 42};
}

public isolated function maybeProvideInt(boolean throwError) returns int|error {
    if throwError {
        return error("Demo error.");
    }
    return 42;
}

public isolated function testUndefinedVariable() returns error? {
    int resultInt;
    transaction {
        resultInt = check maybeProvideInt(true);
        check commit;
    } on fail {
        io:println("Failed to initialize resultInt");
    }
    resultInt += 1;
    io:println(resultInt);  // should always print 42 but can output 1!

    TestObject resultObject;
    do {
        resultObject = check maybeProvideObject(true);
    } on fail {
        io:println("Failed to initialize resultObject");
    }
    io:println(resultObject.number); // throws null pointer exception
}

public function main() {
    error? err = testUndefinedVariable();
    io:println(err);
}

Affected Version(s)

Tested on Ballerina 2201.2.2 (Swan Lake Update 2) and Ballerina 2201.2.0 (Swan Lake Update 2)

OS, DB, other environment details and versions

Ubuntu in WSL 2
Language specification 2022R3
Update Tool 1.3.10

Related area

-> Compilation

Related issue(s) (optional)

Possibly related problem: #37854

Suggested label(s) (optional)

No response

Suggested assignee(s) (optional)

No response

[pcnfernando]

We'll be taking this up on the current sprint.

[github-actions]

This issue is NOT closed with a proper Reason/ label. Make sure to add proper reason label before closing. Please add or leave a comment with the proper reason label now.

      - Reason/EngineeringMistake - The issue occurred due to a mistake made in the past.
      - Reason/Regression - The issue has introduced a regression.
      - Reason/MultipleComponentInteraction - Issue occured due to interactions in multiple components.
      - Reason/Complex - Issue occurred due to complex scenario.
      - Reason/Invalid - Issue is invalid.
      - Reason/Other - None of the above cases.

[pcnfernando]

We have found a regression due to #38828. Hence reverting the fix and reopening the issue.

[buehlefs]

Any estimate on when this may be fixed? While it may not appear as a critical bug in the type checking I do think that this is a severe issue. Especially as it can cause bugs that only appear later in the code (or not at all without proper detection).
The first fix that caused a regression was too eager in marking variables as uninitialized, but could probably be repurposed to only throw a warning until a proper fix can be implemented. This would at least alert users of potential errors in their code.

[pcnfernando]

I agree with your comment. We are working on it atm to cover all possibilities in the data flow analysis. Please note this will be fixed on 2201.8.0