Merge pull request #39951 from pcnfernando/revert-38828-issue_38530

Revert "Consider on-fail as an execution branch when possible failure is reached"

compiler/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/semantics/analyzer/DataflowAnalyzer.java

@@ -262,8 +262,6 @@ public class DataflowAnalyzer extends BLangNodeVisitor {
     private Map<BSymbol, Set<BSymbol>> globalNodeDependsOn;
     private Map<BSymbol, Set<BSymbol>> functionToDependency;
     private boolean flowTerminated = false;
-    private boolean possibleFailureReached = false;
-    private boolean visitingOnFailStmt = false;
 
     private static final CompilerContext.Key<DataflowAnalyzer> DATAFLOW_ANALYZER_KEY = new CompilerContext.Key<>();
     private Deque<BSymbol> currDependentSymbolDeque;
@@ -436,7 +434,6 @@ private void visitFunctionBodyWithDynamicEnv(BLangFunction funcNode, SymbolEnv f
         // updated/marked as initialized.
         this.uninitializedVars = copyUninitializedVars();
         this.flowTerminated = false;
-        this.possibleFailureReached = false;
 
         analyzeNode(funcNode.body, funcEnv);
 
@@ -535,7 +532,6 @@ public void visit(BLangClassDefinition classDef) {
             this.unusedLocalVariables.putAll(prevUnusedLocalVariables);
             this.uninitializedVars = copyUninitializedVars();
             this.flowTerminated = false;
-            this.possibleFailureReached = false;
             visitedOCE = true;
         }
         SymbolEnv objectEnv = SymbolEnv.createClassEnv(classDef, classDef.symbol.scope, env);
@@ -841,7 +837,10 @@ public void visit(BLangForeach foreach) {
         }
 
         analyzeNode(collection, env);
-        analyzeErrorHandlingStatement(foreach.body, foreach.onFailClause);
+        analyzeNode(foreach.body, env);
+        if (foreach.onFailClause != null) {
+            analyzeNode(foreach.onFailClause, env);
+        }
     }
 
     @Override
@@ -854,13 +853,13 @@ public void visit(BLangQueryAction queryAction) {
     @Override
     public void visit(BLangWhile whileNode) {
         Map<BSymbol, InitStatus> prevUninitializedVars = this.uninitializedVars;
-        boolean prevVisitingOnFailStmt = this.visitingOnFailStmt;
-        if (whileNode.onFailClause != null) {
-            this.visitingOnFailStmt = true;
-        }
+
         analyzeNode(whileNode.expr, env);
         BranchResult whileResult = analyzeBranch(whileNode.body, env);
-        analyzeOnFailBranch(whileNode.onFailClause, whileResult);
+
+        if (whileNode.onFailClause != null) {
+            analyzeNode(whileNode.onFailClause, env);
+        }
 
         BType constCondition = ConditionResolver.checkConstCondition(types, symTable, whileNode.expr);
 
@@ -875,56 +874,34 @@ public void visit(BLangWhile whileNode) {
         }
 
         this.uninitializedVars = mergeUninitializedVars(this.uninitializedVars, whileResult.uninitializedVars);
-        this.visitingOnFailStmt = prevVisitingOnFailStmt;
     }
 
     @Override
     public void visit(BLangDo doNode) {
-        analyzeErrorHandlingStatement(doNode.body, doNode.onFailClause);
-    }
-
-    private void analyzeErrorHandlingStatement(BLangBlockStmt blockStmt, BLangOnFailClause onFailClause) {
-        boolean prevVisitingOnFailStmt = this.visitingOnFailStmt;
-        if (onFailClause != null) {
-            this.visitingOnFailStmt = true;
-        }
-        BranchResult doResult = analyzeBranch(blockStmt, env);
-        this.uninitializedVars = doResult.uninitializedVars;
-        analyzeOnFailBranch(onFailClause, doResult);
-        this.visitingOnFailStmt = prevVisitingOnFailStmt;
-    }
-
-    private void analyzeOnFailBranch(BLangOnFailClause onFailClause, BranchResult doResult) {
-        if (this.visitingOnFailStmt) {
-            // loop through the partial init variables from statement block
-            // and remove from `uninitializedVars` map if initialized in on-fail branch
-            BranchResult onFailResult = analyzeBranch(onFailClause, env);
-            Set<BSymbol> symbols = new HashSet<>();
-            for (BSymbol varRef: doResult.uninitializedVars.keySet()) {
-                InitStatus status = onFailResult.uninitializedVars.get(varRef);
-                if (status == null) {
-                    symbols.add(varRef);
-                }
-            }
-            for (BSymbol symbol: symbols) {
-                doResult.uninitializedVars.remove(symbol);
-            }
+        analyzeNode(doNode.body, env);
+        if (doNode.onFailClause != null) {
+            analyzeNode(doNode.onFailClause, env);
         }
     }
 
     public void visit(BLangFail failNode) {
-        this.possibleFailureReached = true;
         analyzeNode(failNode.expr, env);
     }
 
     @Override
     public void visit(BLangLock lockNode) {
-        analyzeErrorHandlingStatement(lockNode.body, lockNode.onFailClause);
+        analyzeNode(lockNode.body, this.env);
+        if (lockNode.onFailClause != null) {
+            analyzeNode(lockNode.onFailClause, env);
+        }
     }
 
     @Override
     public void visit(BLangTransaction transactionNode) {
-        analyzeErrorHandlingStatement(transactionNode.transactionBody, transactionNode.onFailClause);
+        analyzeNode(transactionNode.transactionBody, env);
+        if (transactionNode.onFailClause != null) {
+            analyzeNode(transactionNode.onFailClause, env);
+        }
 
         // marks the injected import as used
         Name transactionPkgName = names.fromString(Names.DOT.value + Names.TRANSACTION_PACKAGE.value);
@@ -1937,9 +1914,6 @@ public void visit(BLangIsAssignableExpr assignableExpr) {
 
     @Override
     public void visit(BLangCheckedExpr checkedExpr) {
-        if (this.visitingOnFailStmt) {
-            this.possibleFailureReached = true;
-        }
         analyzeNode(checkedExpr.expr, env);
     }
 
@@ -1968,7 +1942,10 @@ public void visit(BLangAnnotationAttachment annAttachmentNode) {
 
     @Override
     public void visit(BLangRetry retryNode) {
-        analyzeErrorHandlingStatement(retryNode.retryBody, retryNode.onFailClause);
+        analyzeNode(retryNode.retryBody, env);
+        if (retryNode.onFailClause != null) {
+            analyzeNode(retryNode.onFailClause, env);
+        }
     }
 
     @Override
@@ -2326,28 +2303,25 @@ public void visit(BLangRegExpTemplateLiteral regExpTemplateLiteral) {
     private BranchResult analyzeBranch(BLangNode node, SymbolEnv env) {
         Map<BSymbol, InitStatus> prevUninitializedVars = this.uninitializedVars;
         boolean prevFlowTerminated = this.flowTerminated;
-        boolean prevFailureReached = this.possibleFailureReached;
 
         // Get a snapshot of the current uninitialized vars before visiting the node.
         // This is done so that the original set of uninitialized vars will not be
         // updated/marked as initialized.
         this.uninitializedVars = copyUninitializedVars();
         this.flowTerminated = false;
-        this.possibleFailureReached = false;
 
         analyzeNode(node, env);
-        BranchResult branchResult = new BranchResult(this.uninitializedVars, this.flowTerminated);
+        BranchResult brachResult = new BranchResult(this.uninitializedVars, this.flowTerminated);
 
         // Restore the original set of uninitialized vars
         this.uninitializedVars = prevUninitializedVars;
         this.flowTerminated = prevFlowTerminated;
-        this.possibleFailureReached = prevFailureReached;
 
-        return branchResult;
+        return brachResult;
     }
 
     private Map<BSymbol, InitStatus> copyUninitializedVars() {
-        return new LinkedHashMap<>(this.uninitializedVars);
+        return new HashMap<>(this.uninitializedVars);
     }
 
     private void analyzeNode(BLangNode node, SymbolEnv env) {
@@ -2510,12 +2484,7 @@ private void checkAssignment(BLangExpression varRef) {
             addFunctionToGlobalVarDependency(owner, ((BLangSimpleVarRef) varRef).symbol);
         }
 
-        BSymbol symbol = ((BLangVariableReference) varRef).symbol;
-        if (this.possibleFailureReached && this.uninitializedVars.containsKey(symbol)) {
-            this.uninitializedVars.put(symbol, InitStatus.PARTIAL_INIT);
-        } else {
-            this.uninitializedVars.remove(symbol);
-        }
+        this.uninitializedVars.remove(((BLangVariableReference) varRef).symbol);
     }
 
     private void checkFinalObjectFieldUpdate(BLangFieldBasedAccess fieldAccess) {

tests/jballerina-unit-test/src/test/java/org/ballerinalang/test/statements/onfail/OnFailClauseTest.java

@@ -66,17 +66,6 @@ public void testOnFailClauseNegativeCaseV2() {
         BAssertUtil.validateError(negativeResult, i++, "this function must return a result", 32, 1);
         BAssertUtil.validateError(negativeResult, i++, "this function must return a result", 48, 1);
         BAssertUtil.validateError(negativeResult, i++, "this function must return a result", 66, 1);
-        BAssertUtil.validateError(negativeResult, i++, "variable 'resultInt' may not have been initialized", 92, 5);
-        BAssertUtil.validateError(negativeResult, i++, "variable 'resultInt2' may not have been initialized", 106, 5);
-        BAssertUtil.validateError(negativeResult, i++, "variable 'resultInt3' may not have been initialized", 107, 5);
-        BAssertUtil.validateError(negativeResult, i++, "variable 'resultInt1' may not have been initialized", 121, 5);
-        BAssertUtil.validateError(negativeResult, i++, "variable 'resultInt2' may not have been initialized", 122, 5);
-        BAssertUtil.validateError(negativeResult, i++, "variable 'resultInt3' may not have been initialized", 123, 5);
-        BAssertUtil.validateError(negativeResult, i++, "variable 'resultInt2' may not have been initialized", 140, 5);
-        BAssertUtil.validateError(negativeResult, i++, "variable 'resultInt3' may not have been initialized", 141, 5);
-        BAssertUtil.validateError(negativeResult, i++, "variable 'resultInt3' may not have been initialized", 159, 5);
-        BAssertUtil.validateError(negativeResult, i++, "variable 'k' may not have been initialized", 174, 9);
-        BAssertUtil.validateError(negativeResult, i++, "variable 'str2' may not have been initialized", 212, 5);
         Assert.assertEquals(negativeResult.getErrorCount(), i);
     }
 

tests/jballerina-unit-test/src/test/java/org/ballerinalang/test/types/globalvar/GlobalVarNegativeTest.java

@@ -198,16 +198,4 @@ public void testConfigurableImplicitReadOnly() {
                 28, 22);
         Assert.assertEquals(result.getErrorCount(), i);
     }
-
-    @Test
-    public void testGlobalVariableInitWithOnFailNegative() {
-        CompileResult result = BCompileUtil.compile
-                ("test-src/statements/variabledef/global_variable_init_with_onfail_negative.bal");
-        int i = 0;
-        BAssertUtil.validateError(result, i++, "uninitialized variable 'j'", 18, 1);
-        BAssertUtil.validateError(result, i++, "uninitialized variable 'k'", 19, 1);
-        BAssertUtil.validateError(result, i++, INVALID_FUNC_OR_METHOD_CALL_WITH_UNINITIALIZED_VARS_PREFIX +
-                "variable(s) 'j, k' not initialized", 31, 5);
-        Assert.assertEquals(result.getErrorCount(), i);
-    }
 }

tests/jballerina-unit-test/src/test/resources/test-src/statements/onfail/on-fail-clause-negative-v2.bal

@@ -78,138 +78,3 @@ function getError() returns error {
     error err = error("Custom Error");
     return err;
 }
-
-function getErrorOrInt() returns int|error {
-    return getError();
-}
-
-public function testUnInitVars1() {
-    int resultInt;
-    do {
-        resultInt = check getErrorOrInt();
-    } on fail {
-    }
-    resultInt += 1;
-}
-
-public function testUnInitVars2() {
-    int resultInt1;
-    int resultInt2;
-    int resultInt3;
-    do {
-        resultInt1 = 1;
-        resultInt2 = check getErrorOrInt();
-        resultInt3 = 1;
-    } on fail {
-    }
-    resultInt1 += 1;
-    resultInt2 += 1;
-    resultInt3 += 1;
-}
-
-public function testUnInitVars3() {
-    int resultInt1;
-    int resultInt2;
-    int resultInt3;
-    transaction {
-        check commit;
-        resultInt1 = 1;
-        resultInt2 = check getErrorOrInt();
-        resultInt3 = 1;
-    } on fail {
-    }
-    resultInt1 += 1;
-    resultInt2 += 1;
-    resultInt3 += 1;
-}
-
-public function testUnInitVars4() {
-    int resultInt1;
-    int resultInt2;
-    int resultInt3;
-    transaction {
-        do {
-           resultInt1 = 1;
-           resultInt2 = check getErrorOrInt();
-           resultInt3 = 1;
-        }
-        check commit;
-    } on fail {
-    }
-    resultInt1 += 1;
-    resultInt2 += 1;
-    resultInt3 += 1;
-}
-
-public function testUnInitVars5() {
-    int resultInt1;
-    int resultInt2;
-    int resultInt3;
-    transaction {
-        do {
-           resultInt1 = 1;
-           resultInt2 = 2;
-        }
-        check commit;
-        resultInt3 = 1;
-    } on fail {
-    }
-    resultInt1 += 1;
-    resultInt2 += 1;
-    resultInt3 += 1;
-}
-
-function testUnInitVars6() {
-    int i;
-    int j;
-    int k;
-    do {
-        i = 0;
-        j = 1;
-        check getErrorOrNil();
-        k = 1;
-    } on fail {
-        i += 1;
-        j += 1;
-        k += 1;
-    }
-}
-
-function testUnInitVars7() {
-    int i;
-    int j;
-    int k;
-    do {
-        i = 0;
-        j = 1;
-        check getErrorOrNil();
-        k = 1;
-    } on fail {
-        k = -1;
-    }
-    i += 1;
-    j += 1;
-    k += 1;
-}
-
-function getErrorOrNil() returns error? {
-    return getError();
-}
-
-function testUnInitVars8(int[] data) returns string {
-    string str1 = "";
-    string str2;
-    foreach var i in data {
-        if(i < 0) {
-            check getErrorOrNil();
-            str1 = "partial init";
-            str2 = "partial init";
-        }
-    } on fail {
-        str1 += "-> error caught. Hence value returning";
-        return str1;
-    }
-    str2 += "-> reached end";
-    return str1;
-}
-

tests/jballerina-unit-test/src/test/resources/test-src/statements/onfail/on-fail-clause.bal

@@ -38,7 +38,6 @@ function testOnFailEdgeTestcases() {
     testOnFailWithinInLineServiceObj();
     assertTrue(testOnFailInAnonFunctionExpr() is ());
     testNoPossibleFailureWithOnFail();
-    testVarRefValueUpdate();
 }
 
 function testUnreachableCodeWithIf(){
@@ -589,18 +588,6 @@ function testOnFailInAnonFunctionExpr() returns error? {
     assertEquality((<error>result2).detail().get("message"), "'string' value 'a2' cannot be converted to 'int'");
 }
 
-function testVarRefValueUpdate() {
-    int i = 0;
-    do {
-        i += 1;
-        _ = check getCheckError();
-        i += 1;
-    } on fail {
-        i = i - 1;
-    }
-    assertEquality(0, i);
-}
-
 //-------------------------------------------------------------------------------
 
 type AssertionError error;