From be6452b24486415d423bae9a352db02712945000 Mon Sep 17 00:00:00 2001 From: Heshan Padmasiri Date: Fri, 22 Dec 2023 07:30:50 +0530 Subject: [PATCH 1/6] Remove unnecessary casting for record field set --- .../compiler/desugar/Desugar.java | 21 ++++- .../test/bir/RecordDesugarTest.java | 93 +++++++++++++++++++ .../test-src/bir/bir-dump/setNillableField | 32 +++++++ .../test-src/bir/bir-dump/setOptionalField | 24 +++++ .../test-src/bir/bir-dump/setRequiredField | 24 +++++ .../resources/test-src/bir/record_desugar.bal | 27 ++++++ 6 files changed, 219 insertions(+), 2 deletions(-) create mode 100644 tests/jballerina-unit-test/src/test/java/org/ballerinalang/test/bir/RecordDesugarTest.java create mode 100644 tests/jballerina-unit-test/src/test/resources/test-src/bir/bir-dump/setNillableField create mode 100644 tests/jballerina-unit-test/src/test/resources/test-src/bir/bir-dump/setOptionalField create mode 100644 tests/jballerina-unit-test/src/test/resources/test-src/bir/bir-dump/setRequiredField create mode 100644 tests/jballerina-unit-test/src/test/resources/test-src/bir/record_desugar.bal diff --git a/compiler/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/desugar/Desugar.java b/compiler/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/desugar/Desugar.java index de2afc4612a7..0b33f5ef92f6 100644 --- a/compiler/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/desugar/Desugar.java +++ b/compiler/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/desugar/Desugar.java @@ -2475,17 +2475,34 @@ private void createSimpleVarDefStmt(BLangSimpleVariable simpleVariable, BLangBlo @Override public void visit(BLangAssignment assignNode) { - boolean fieldAccessLVExpr = assignNode.varRef.getKind() == NodeKind.FIELD_BASED_ACCESS_EXPR; + boolean isOptionalFieldAssignment = isOptionalFieldAssignment(assignNode); assignNode.varRef = rewriteExpr(assignNode.varRef); assignNode.expr = rewriteExpr(assignNode.expr); BType castingType = assignNode.varRef.getBType(); - if (fieldAccessLVExpr) { + if (isOptionalFieldAssignment) { castingType = types.addNilForNillableAccessType(castingType); } assignNode.expr = types.addConversionExprIfRequired(rewriteExpr(assignNode.expr), castingType); result = assignNode; } + private boolean isOptionalFieldAssignment(BLangAssignment assignNode) { + if (assignNode.varRef.getKind() != NodeKind.FIELD_BASED_ACCESS_EXPR) { + return false; + } + BLangFieldBasedAccess fieldAccessNode = (BLangFieldBasedAccess) assignNode.varRef; + BType targetType = Types.getImpliedType(fieldAccessNode.expr.getBType()); + if (targetType.getKind() != TypeKind.RECORD) { + return false; + } + BRecordType recordType = (BRecordType) targetType; + BField field = recordType.fields.get(fieldAccessNode.field.value); + if (field == null) { + return false; + } + return (field.symbol.flags & Flags.OPTIONAL) == Flags.OPTIONAL; + } + @Override public void visit(BLangTupleDestructure tupleDestructure) { // case 1: diff --git a/tests/jballerina-unit-test/src/test/java/org/ballerinalang/test/bir/RecordDesugarTest.java b/tests/jballerina-unit-test/src/test/java/org/ballerinalang/test/bir/RecordDesugarTest.java new file mode 100644 index 000000000000..a291a3c84648 --- /dev/null +++ b/tests/jballerina-unit-test/src/test/java/org/ballerinalang/test/bir/RecordDesugarTest.java @@ -0,0 +1,93 @@ +/* + * Copyright (c) 2023, WSO2 LLC. (http://www.wso2.org). + * + * WSO2 LLC. licenses this file to you under the Apache License, + * Version 2.0 (the "License"); you may not use this file except + * in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.ballerinalang.test.bir; + +import org.ballerinalang.test.BCompileUtil; +import org.testng.Assert; +import org.testng.annotations.AfterClass; +import org.testng.annotations.BeforeClass; +import org.testng.annotations.Test; +import org.wso2.ballerinalang.compiler.bir.emit.BIREmitter; +import org.wso2.ballerinalang.compiler.bir.model.BIRNode; +import org.wso2.ballerinalang.compiler.util.CompilerContext; + +import java.io.IOException; +import java.nio.charset.StandardCharsets; +import java.nio.file.Files; +import java.nio.file.Path; +import java.nio.file.Paths; +import java.util.Arrays; +import java.util.List; +import java.util.stream.Stream; + +/** + * This class contains unit tests to validate various desugaring/optimizations on records produce the expected BIR. + * + * @since 2201.9.0 + */ +public class RecordDesugarTest { + + private BIREmitter birEmitter; + private BCompileUtil.BIRCompileResult result; + + @BeforeClass + public void setup() { + birEmitter = BIREmitter.getInstance(new CompilerContext()); + result = BCompileUtil.generateBIR("test-src/bir/record_desugar.bal"); + } + + @Test(description = "Test record field set") + public void testRecordFieldSet() { + List functions = Arrays.asList("setRequiredField", "setNillableField", "setOptionalField"); + result.getExpectedBIR().functions.stream().filter(function -> functions.contains(function.name.value)) + .forEach(this::assertFunctions); + } + + private void assertFunctions(BIRNode.BIRFunction function) { + String actual = BIREmitter.emitFunction(function, 0); + String expected = null; + try { + expected = readFile(function.name.value); + } catch (IOException e) { + Assert.fail("Failed to read the expected BIR file for function: " + function.name.value, e); + } + Assert.assertEquals(actual, expected); + } + + private String readFile(String name) throws IOException { + // The files in the bir-dump folder are named with the function name and contain the expected bir dump for + // the function + Path filePath = Paths.get("src", "test", "resources", "test-src", "bir", "bir-dump", name).toAbsolutePath(); + if (Files.exists(filePath)) { + StringBuilder contentBuilder = new StringBuilder(); + + Stream stream = Files.lines(filePath, StandardCharsets.UTF_8); + stream.forEach(s -> contentBuilder.append(s).append("\n")); + + return contentBuilder.toString().trim(); + } + Assert.fail("Expected BIR file not found for function: " + name); + return null; + } + + @AfterClass + public void tearDown() { + result = null; + } +} diff --git a/tests/jballerina-unit-test/src/test/resources/test-src/bir/bir-dump/setNillableField b/tests/jballerina-unit-test/src/test/resources/test-src/bir/bir-dump/setNillableField new file mode 100644 index 000000000000..d216c36953cb --- /dev/null +++ b/tests/jballerina-unit-test/src/test/resources/test-src/bir/bir-dump/setNillableField @@ -0,0 +1,32 @@ +setNillableField function() -> () { + %0(RETURN) (); + %1(LOCAL) R2; + %2(TEMP) typeDesc; + %4(TEMP) string; + %5(TEMP) int|(); + %6(TEMP) int; + %12(TEMP) (); + + bb0 { + %2 = newType R2; + %4 = ConstLoad x; + %6 = ConstLoad 1; + %5 = %6; + %1 = NewMap %2{%4:%5}; + %6 = ConstLoad 2; + %5 = %6; + %4 = ConstLoad x; + %1[%4] = %5; + %12 = ConstLoad 0; + %5 = %12; + %4 = ConstLoad x; + %1[%4] = %5; + %0 = ConstLoad 0; + GOTO bb1; + } + bb1 { + return; + } + + +} diff --git a/tests/jballerina-unit-test/src/test/resources/test-src/bir/bir-dump/setOptionalField b/tests/jballerina-unit-test/src/test/resources/test-src/bir/bir-dump/setOptionalField new file mode 100644 index 000000000000..ea4903e70b42 --- /dev/null +++ b/tests/jballerina-unit-test/src/test/resources/test-src/bir/bir-dump/setOptionalField @@ -0,0 +1,24 @@ +setOptionalField function() -> () { + %0(RETURN) (); + %1(LOCAL) R3; + %2(TEMP) typeDesc; + %4(TEMP) int|(); + %5(TEMP) int; + %7(TEMP) string; + + bb0 { + %2 = newType R3; + %1 = NewMap %2{}; + %5 = ConstLoad 2; + %4 = %5; + %7 = ConstLoad x; + %1[%7] = %4; + %0 = ConstLoad 0; + GOTO bb1; + } + bb1 { + return; + } + + +} diff --git a/tests/jballerina-unit-test/src/test/resources/test-src/bir/bir-dump/setRequiredField b/tests/jballerina-unit-test/src/test/resources/test-src/bir/bir-dump/setRequiredField new file mode 100644 index 000000000000..24101ec4fd7a --- /dev/null +++ b/tests/jballerina-unit-test/src/test/resources/test-src/bir/bir-dump/setRequiredField @@ -0,0 +1,24 @@ +setRequiredField function() -> () { + %0(RETURN) (); + %1(LOCAL) R1; + %2(TEMP) typeDesc}>; + %4(TEMP) string; + %5(TEMP) int; + + bb0 { + %2 = newType R1; + %4 = ConstLoad x; + %5 = ConstLoad 1; + %1 = NewMap %2{%4:%5}; + %5 = ConstLoad 2; + %4 = ConstLoad x; + %1[%4] = %5; + %0 = ConstLoad 0; + GOTO bb1; + } + bb1 { + return; + } + + +} diff --git a/tests/jballerina-unit-test/src/test/resources/test-src/bir/record_desugar.bal b/tests/jballerina-unit-test/src/test/resources/test-src/bir/record_desugar.bal new file mode 100644 index 000000000000..534e74b26a8b --- /dev/null +++ b/tests/jballerina-unit-test/src/test/resources/test-src/bir/record_desugar.bal @@ -0,0 +1,27 @@ +type R1 record {| + int x; +|}; + +type R2 record {| + int? x; +|}; + +type R3 record {| + int x?; +|}; + +function setRequiredField() { + R1 r1 = { x: 1 }; + r1.x = 2; +} + +function setNillableField() { + R2 r2 = { x: 1 }; + r2.x = 2; + r2.x = (); +} + +function setOptionalField() { + R3 r3 = { }; + r3.x = 2; +} From 2e0f804773073f77ccf4856a9b6c6bee013e3044 Mon Sep 17 00:00:00 2001 From: Heshan Padmasiri Date: Fri, 22 Dec 2023 11:21:40 +0530 Subject: [PATCH 2/6] Limit casting to only basic types --- .../compiler/desugar/Desugar.java | 12 ++++-- .../access/OptionalFieldAccessTest.java | 15 +++++++ .../access/optional_field_access.bal | 41 +++++++++++++++++++ 3 files changed, 64 insertions(+), 4 deletions(-) diff --git a/compiler/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/desugar/Desugar.java b/compiler/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/desugar/Desugar.java index 0b33f5ef92f6..b3df16a92601 100644 --- a/compiler/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/desugar/Desugar.java +++ b/compiler/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/desugar/Desugar.java @@ -2475,7 +2475,7 @@ private void createSimpleVarDefStmt(BLangSimpleVariable simpleVariable, BLangBlo @Override public void visit(BLangAssignment assignNode) { - boolean isOptionalFieldAssignment = isOptionalFieldAssignment(assignNode); + boolean isOptionalFieldAssignment = isOptionalBasicTypeFieldAssignment(assignNode); assignNode.varRef = rewriteExpr(assignNode.varRef); assignNode.expr = rewriteExpr(assignNode.expr); BType castingType = assignNode.varRef.getBType(); @@ -2486,7 +2486,7 @@ public void visit(BLangAssignment assignNode) { result = assignNode; } - private boolean isOptionalFieldAssignment(BLangAssignment assignNode) { + private boolean isOptionalBasicTypeFieldAssignment(BLangAssignment assignNode) { if (assignNode.varRef.getKind() != NodeKind.FIELD_BASED_ACCESS_EXPR) { return false; } @@ -2497,10 +2497,14 @@ private boolean isOptionalFieldAssignment(BLangAssignment assignNode) { } BRecordType recordType = (BRecordType) targetType; BField field = recordType.fields.get(fieldAccessNode.field.value); - if (field == null) { + if (field == null || (field.symbol.flags & Flags.OPTIONAL) != Flags.OPTIONAL) { return false; } - return (field.symbol.flags & Flags.OPTIONAL) == Flags.OPTIONAL; + BType fieldType = field.getType(); + return switch (fieldType.getKind()) { + case BOOLEAN, FLOAT, STRING, DECIMAL -> true; + default -> TypeTags.isIntegerTypeTag(fieldType.tag); + }; } @Override diff --git a/tests/jballerina-unit-test/src/test/java/org/ballerinalang/test/expressions/access/OptionalFieldAccessTest.java b/tests/jballerina-unit-test/src/test/java/org/ballerinalang/test/expressions/access/OptionalFieldAccessTest.java index 5ba1e5aa73b8..8dd76c9e1a69 100644 --- a/tests/jballerina-unit-test/src/test/java/org/ballerinalang/test/expressions/access/OptionalFieldAccessTest.java +++ b/tests/jballerina-unit-test/src/test/java/org/ballerinalang/test/expressions/access/OptionalFieldAccessTest.java @@ -167,6 +167,21 @@ public void testOptionalFieldAccessOnMethodCall() { BRunUtil.invoke(result, "testOptionalFieldAccessOnMethodCall"); } + @Test(dataProvider = "optionalFieldRemovalFunctions") + public void testOptionalFieldRemoval(String function) { + Object returns = BRunUtil.invoke(result, function); + Assert.assertFalse((Boolean) returns); + } + + @DataProvider(name = "optionalFieldRemovalFunctions") + public Object[][] optionalFieldRemovalFunctions() { + return new Object[][]{ + {"testOptionalFieldRemovalBasicType"}, + {"testOptionalFieldRemovalIndirect"}, + {"testOptionalFieldRemovalComplex"} + }; + } + @Test public void testNestedOptionalFieldAccessOnIntersectionTypes() { BRunUtil.invoke(result, "testNestedOptionalFieldAccessOnIntersectionTypes"); diff --git a/tests/jballerina-unit-test/src/test/resources/test-src/expressions/access/optional_field_access.bal b/tests/jballerina-unit-test/src/test/resources/test-src/expressions/access/optional_field_access.bal index bf3daab13bbe..473976a8e523 100644 --- a/tests/jballerina-unit-test/src/test/resources/test-src/expressions/access/optional_field_access.bal +++ b/tests/jballerina-unit-test/src/test/resources/test-src/expressions/access/optional_field_access.bal @@ -40,6 +40,19 @@ type Bar record { decimal c; }; +type R1 record { + int a?; + float b?; + string c?; + boolean d?; + decimal e?; +}; + +type R2 record { + int a; + R1 r1?; +}; + function testOptionalFieldAccessOnRequiredRecordField() returns boolean { string s = "Anne"; Employee e = { name: s, id: 100 }; @@ -47,6 +60,34 @@ function testOptionalFieldAccessOnRequiredRecordField() returns boolean { return name == s; } +function testOptionalFieldRemovalBasicType() returns boolean { + R1 r = { a: 1, b: 2.0, c: "test", d: true, e: 3.0 }; + r.a = (); + r.b = (); + r.c = (); + r.d = (); + r.e = (); + return r.hasKey("a") || r.hasKey("b") || r.hasKey("c") || r.hasKey("d") || r.hasKey("e"); +} + +function testOptionalFieldRemovalIndirect() returns boolean { + R2 r = { a: 1, r1: { a: 1, b: 2.0, c: "test" } }; + r.r1.a = (); + r.r1.b = (); + r.r1.c = (); + R1? r1 = r.r1; + if r1 is () { + return true; + } + return r1.hasKey("a") || r1.hasKey("b") || r1.hasKey("c"); +} + +function testOptionalFieldRemovalComplex() returns boolean { + R2 r = { a: 1, r1: { a: 1, b: 2.0, c: "test" } }; + r.r1 = (); + return r.hasKey("r1"); +} + function testOptionalFieldAccessOnRequiredRecordFieldInRecordUnion() returns boolean { Foo f = { a: 1, b: true }; Foo|Bar fb = f; From 9ce1b3bb03d64ec8449283d9d75b58a41685e6c2 Mon Sep 17 00:00:00 2001 From: Heshan Padmasiri Date: Wed, 3 Jan 2024 16:14:46 +0530 Subject: [PATCH 3/6] Fix unit tests --- .../access/OptionalFieldAccessTest.java | 3 +- .../resources/test-src/bir/record_desugar.bal | 6 ++-- .../access/optional_field_access.bal | 33 +++++++++++-------- 3 files changed, 24 insertions(+), 18 deletions(-) diff --git a/tests/jballerina-unit-test/src/test/java/org/ballerinalang/test/expressions/access/OptionalFieldAccessTest.java b/tests/jballerina-unit-test/src/test/java/org/ballerinalang/test/expressions/access/OptionalFieldAccessTest.java index 8dd76c9e1a69..21dcfe7bf16f 100644 --- a/tests/jballerina-unit-test/src/test/java/org/ballerinalang/test/expressions/access/OptionalFieldAccessTest.java +++ b/tests/jballerina-unit-test/src/test/java/org/ballerinalang/test/expressions/access/OptionalFieldAccessTest.java @@ -169,8 +169,7 @@ public void testOptionalFieldAccessOnMethodCall() { @Test(dataProvider = "optionalFieldRemovalFunctions") public void testOptionalFieldRemoval(String function) { - Object returns = BRunUtil.invoke(result, function); - Assert.assertFalse((Boolean) returns); + BRunUtil.invoke(result, function); } @DataProvider(name = "optionalFieldRemovalFunctions") diff --git a/tests/jballerina-unit-test/src/test/resources/test-src/bir/record_desugar.bal b/tests/jballerina-unit-test/src/test/resources/test-src/bir/record_desugar.bal index 534e74b26a8b..05e6482eb102 100644 --- a/tests/jballerina-unit-test/src/test/resources/test-src/bir/record_desugar.bal +++ b/tests/jballerina-unit-test/src/test/resources/test-src/bir/record_desugar.bal @@ -11,17 +11,17 @@ type R3 record {| |}; function setRequiredField() { - R1 r1 = { x: 1 }; + R1 r1 = {x: 1}; r1.x = 2; } function setNillableField() { - R2 r2 = { x: 1 }; + R2 r2 = {x: 1}; r2.x = 2; r2.x = (); } function setOptionalField() { - R3 r3 = { }; + R3 r3 = {}; r3.x = 2; } diff --git a/tests/jballerina-unit-test/src/test/resources/test-src/expressions/access/optional_field_access.bal b/tests/jballerina-unit-test/src/test/resources/test-src/expressions/access/optional_field_access.bal index 473976a8e523..b11dc182777e 100644 --- a/tests/jballerina-unit-test/src/test/resources/test-src/expressions/access/optional_field_access.bal +++ b/tests/jballerina-unit-test/src/test/resources/test-src/expressions/access/optional_field_access.bal @@ -60,32 +60,35 @@ function testOptionalFieldAccessOnRequiredRecordField() returns boolean { return name == s; } -function testOptionalFieldRemovalBasicType() returns boolean { - R1 r = { a: 1, b: 2.0, c: "test", d: true, e: 3.0 }; +function testOptionalFieldRemovalBasicType() { + R1 r = {a: 1, b: 2.0, c: "test", d: true, e: 3.0}; r.a = (); r.b = (); r.c = (); r.d = (); r.e = (); - return r.hasKey("a") || r.hasKey("b") || r.hasKey("c") || r.hasKey("d") || r.hasKey("e"); + assertFalse(r.hasKey("a")); + assertFalse(r.hasKey("b")); + assertFalse(r.hasKey("c")); + assertFalse(r.hasKey("d")); + assertFalse(r.hasKey("e")); } -function testOptionalFieldRemovalIndirect() returns boolean { - R2 r = { a: 1, r1: { a: 1, b: 2.0, c: "test" } }; +function testOptionalFieldRemovalIndirect() { + R2 r = {a: 1, r1: {a: 1, b: 2.0, c: "test"}}; r.r1.a = (); r.r1.b = (); r.r1.c = (); - R1? r1 = r.r1; - if r1 is () { - return true; - } - return r1.hasKey("a") || r1.hasKey("b") || r1.hasKey("c"); + R1 r1 = r.r1; + assertFalse(r1.hasKey("a")); + assertFalse(r1.hasKey("b")); + assertFalse(r1.hasKey("c")); } -function testOptionalFieldRemovalComplex() returns boolean { - R2 r = { a: 1, r1: { a: 1, b: 2.0, c: "test" } }; +function testOptionalFieldRemovalComplex() { + R2 r = {a: 1, r1: {a: 1, b: 2.0, c: "test"}}; r.r1 = (); - return r.hasKey("r1"); + assertFalse(r.hasKey("r1")); } function testOptionalFieldAccessOnRequiredRecordFieldInRecordUnion() returns boolean { @@ -615,6 +618,10 @@ function assertTrue(anydata actual) { assertEquality(true, actual); } +function assertFalse(anydata actual) { + assertEquality(false, actual); +} + function assertEquality(anydata expected, anydata actual) { if expected == actual { return; From d4d10ae574c3fa01c398e0127ef15ca73f68c754 Mon Sep 17 00:00:00 2001 From: Heshan Padmasiri Date: Thu, 4 Jan 2024 09:42:23 +0530 Subject: [PATCH 4/6] Use tags instead of kind for type checking --- .../ballerinalang/compiler/desugar/Desugar.java | 16 +++++++--------- .../expressions/access/optional_field_access.bal | 10 +++++++++- 2 files changed, 16 insertions(+), 10 deletions(-) diff --git a/compiler/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/desugar/Desugar.java b/compiler/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/desugar/Desugar.java index b3df16a92601..8c4f1dcd50f5 100644 --- a/compiler/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/desugar/Desugar.java +++ b/compiler/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/desugar/Desugar.java @@ -2487,24 +2487,22 @@ public void visit(BLangAssignment assignNode) { } private boolean isOptionalBasicTypeFieldAssignment(BLangAssignment assignNode) { - if (assignNode.varRef.getKind() != NodeKind.FIELD_BASED_ACCESS_EXPR) { + BLangNode varRef = assignNode.varRef; + if (varRef.getKind() != NodeKind.FIELD_BASED_ACCESS_EXPR) { return false; } - BLangFieldBasedAccess fieldAccessNode = (BLangFieldBasedAccess) assignNode.varRef; + BLangFieldBasedAccess fieldAccessNode = (BLangFieldBasedAccess) varRef; BType targetType = Types.getImpliedType(fieldAccessNode.expr.getBType()); - if (targetType.getKind() != TypeKind.RECORD) { + if (targetType.tag != TypeTags.RECORD) { return false; } BRecordType recordType = (BRecordType) targetType; BField field = recordType.fields.get(fieldAccessNode.field.value); - if (field == null || (field.symbol.flags & Flags.OPTIONAL) != Flags.OPTIONAL) { + if (field == null || !Symbols.isOptional(field.symbol)) { return false; } - BType fieldType = field.getType(); - return switch (fieldType.getKind()) { - case BOOLEAN, FLOAT, STRING, DECIMAL -> true; - default -> TypeTags.isIntegerTypeTag(fieldType.tag); - }; + BType fieldType = Types.getImpliedType(field.getType()); + return TypeTags.isSimpleBasicType(fieldType.tag); } @Override diff --git a/tests/jballerina-unit-test/src/test/resources/test-src/expressions/access/optional_field_access.bal b/tests/jballerina-unit-test/src/test/resources/test-src/expressions/access/optional_field_access.bal index b11dc182777e..2ae35c7d0842 100644 --- a/tests/jballerina-unit-test/src/test/resources/test-src/expressions/access/optional_field_access.bal +++ b/tests/jballerina-unit-test/src/test/resources/test-src/expressions/access/optional_field_access.bal @@ -40,12 +40,16 @@ type Bar record { decimal c; }; +type MyString string; + type R1 record { int a?; float b?; string c?; boolean d?; decimal e?; + string:Char f?; + MyString g?; }; type R2 record { @@ -61,17 +65,21 @@ function testOptionalFieldAccessOnRequiredRecordField() returns boolean { } function testOptionalFieldRemovalBasicType() { - R1 r = {a: 1, b: 2.0, c: "test", d: true, e: 3.0}; + R1 r = {a: 1, b: 2.0, c: "test", d: true, e: 3.0, f:"c", g: "test"}; r.a = (); r.b = (); r.c = (); r.d = (); r.e = (); + r.f = (); + r.g = (); assertFalse(r.hasKey("a")); assertFalse(r.hasKey("b")); assertFalse(r.hasKey("c")); assertFalse(r.hasKey("d")); assertFalse(r.hasKey("e")); + assertFalse(r.hasKey("f")); + assertFalse(r.hasKey("g")); } function testOptionalFieldRemovalIndirect() { From 6dfae92e8c3373c4dd75f8c610011e1001bb147a Mon Sep 17 00:00:00 2001 From: Heshan Padmasiri Date: Fri, 5 Jan 2024 13:22:32 +0530 Subject: [PATCH 5/6] Remove casting when assigning non-nil value --- .../compiler/desugar/Desugar.java | 22 ++--- .../semantics/analyzer/SemanticAnalyzer.java | 21 +++-- .../semantics/analyzer/TypeChecker.java | 2 +- .../compiler/util/CompilerUtils.java | 21 +++++ .../access/OptionalFieldAccessTest.java | 5 +- .../test-src/bir/bir-dump/setOptionalField | 18 ++-- .../resources/test-src/bir/record_desugar.bal | 1 + .../access/optional_field_access.bal | 82 +++++++++++++++++++ 8 files changed, 141 insertions(+), 31 deletions(-) diff --git a/compiler/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/desugar/Desugar.java b/compiler/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/desugar/Desugar.java index 8c4f1dcd50f5..c7be1ef41e4b 100644 --- a/compiler/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/desugar/Desugar.java +++ b/compiler/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/desugar/Desugar.java @@ -340,6 +340,7 @@ import static org.wso2.ballerinalang.compiler.desugar.ASTBuilderUtil.createVariable; import static org.wso2.ballerinalang.compiler.desugar.ASTBuilderUtil.createVariableRef; import static org.wso2.ballerinalang.compiler.util.CompilerUtils.getMajorVersion; +import static org.wso2.ballerinalang.compiler.util.CompilerUtils.isAssignmentToOptionalField; import static org.wso2.ballerinalang.compiler.util.Names.GENERATED_INIT_SUFFIX; import static org.wso2.ballerinalang.compiler.util.Names.GEN_VAR_PREFIX; import static org.wso2.ballerinalang.compiler.util.Names.IGNORE; @@ -2475,32 +2476,25 @@ private void createSimpleVarDefStmt(BLangSimpleVariable simpleVariable, BLangBlo @Override public void visit(BLangAssignment assignNode) { - boolean isOptionalFieldAssignment = isOptionalBasicTypeFieldAssignment(assignNode); + boolean addNilToCastingType = shouldWidenExpressionTypeWithNil(assignNode); assignNode.varRef = rewriteExpr(assignNode.varRef); assignNode.expr = rewriteExpr(assignNode.expr); BType castingType = assignNode.varRef.getBType(); - if (isOptionalFieldAssignment) { + if (addNilToCastingType) { castingType = types.addNilForNillableAccessType(castingType); } assignNode.expr = types.addConversionExprIfRequired(rewriteExpr(assignNode.expr), castingType); result = assignNode; } - private boolean isOptionalBasicTypeFieldAssignment(BLangAssignment assignNode) { - BLangNode varRef = assignNode.varRef; - if (varRef.getKind() != NodeKind.FIELD_BASED_ACCESS_EXPR) { + private static boolean shouldWidenExpressionTypeWithNil(BLangAssignment assignNode) { + if (!assignNode.expr.getBType().isNullable() || !isAssignmentToOptionalField(assignNode)) { return false; } - BLangFieldBasedAccess fieldAccessNode = (BLangFieldBasedAccess) varRef; - BType targetType = Types.getImpliedType(fieldAccessNode.expr.getBType()); - if (targetType.tag != TypeTags.RECORD) { - return false; - } - BRecordType recordType = (BRecordType) targetType; + // If we are assigning to an optional field we have a field based access on a record + BLangFieldBasedAccess fieldAccessNode = (BLangFieldBasedAccess) assignNode.varRef; + BRecordType recordType = (BRecordType) Types.getImpliedType(fieldAccessNode.expr.getBType()); BField field = recordType.fields.get(fieldAccessNode.field.value); - if (field == null || !Symbols.isOptional(field.symbol)) { - return false; - } BType fieldType = Types.getImpliedType(field.getType()); return TypeTags.isSimpleBasicType(fieldType.tag); } diff --git a/compiler/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/semantics/analyzer/SemanticAnalyzer.java b/compiler/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/semantics/analyzer/SemanticAnalyzer.java index 7cb3e4f838d4..cf9cd9884090 100644 --- a/compiler/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/semantics/analyzer/SemanticAnalyzer.java +++ b/compiler/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/semantics/analyzer/SemanticAnalyzer.java @@ -118,7 +118,6 @@ import org.wso2.ballerinalang.compiler.tree.expressions.BLangConstant; import org.wso2.ballerinalang.compiler.tree.expressions.BLangErrorVarRef; import org.wso2.ballerinalang.compiler.tree.expressions.BLangExpression; -import org.wso2.ballerinalang.compiler.tree.expressions.BLangFieldBasedAccess; import org.wso2.ballerinalang.compiler.tree.expressions.BLangInvocation; import org.wso2.ballerinalang.compiler.tree.expressions.BLangLambdaFunction; import org.wso2.ballerinalang.compiler.tree.expressions.BLangLetExpression; @@ -246,6 +245,7 @@ import static org.ballerinalang.model.tree.NodeKind.RECORD_LITERAL_EXPR; import static org.ballerinalang.model.tree.NodeKind.REG_EXP_CAPTURING_GROUP; import static org.ballerinalang.model.tree.NodeKind.REG_EXP_CHARACTER_CLASS; +import static org.wso2.ballerinalang.compiler.util.CompilerUtils.isAssignmentToOptionalField; /** * @since 0.94 @@ -2318,15 +2318,20 @@ public void visit(BLangAssignment assignNode, AnalyzerData data) { validateFunctionVarRef(varRef, data); checkInvalidTypeDef(varRef); - if (varRef.getKind() == NodeKind.FIELD_BASED_ACCESS_EXPR && data.expType.tag != TypeTags.SEMANTIC_ERROR) { - BLangFieldBasedAccess fieldBasedAccessVarRef = (BLangFieldBasedAccess) varRef; - int varRefTypeTag = Types.getImpliedType(fieldBasedAccessVarRef.expr.getBType()).tag; - if (varRefTypeTag == TypeTags.RECORD && Symbols.isOptional(fieldBasedAccessVarRef.symbol)) { - data.expType = types.addNilForNillableAccessType(data.expType); - } + BType actualExpectedType = null; + // For optional field assignments we add nil to the expected type before doing type checking in order to get + // the type in error messages correct. But we don't need an implicit conversion since desugar will add a + // cast if needed. + if (data.expType != symTable.semanticError && isAssignmentToOptionalField(assignNode)) { + actualExpectedType = data.expType; + data.expType = types.addNilForNillableAccessType(actualExpectedType); } - data.typeChecker.checkExpr(assignNode.expr, data.env, data.expType, data.prevEnvs, data.commonAnalyzerData); + BLangExpression expr = assignNode.expr; + data.typeChecker.checkExpr(expr, data.env, data.expType, data.prevEnvs, data.commonAnalyzerData); + if (actualExpectedType != null && expr.impConversionExpr != null) { + data.typeChecker.resetImpConversionExpr(expr, expr.getBType(), actualExpectedType); + } validateWorkerAnnAttachments(assignNode.expr, data); diff --git a/compiler/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/semantics/analyzer/TypeChecker.java b/compiler/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/semantics/analyzer/TypeChecker.java index 0c89d01d0684..e029c63930c6 100644 --- a/compiler/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/semantics/analyzer/TypeChecker.java +++ b/compiler/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/semantics/analyzer/TypeChecker.java @@ -6506,7 +6506,7 @@ protected void visitCheckAndCheckPanicExpr(BLangCheckedExpr checkedExpr, Analyze data.resultType = types.checkType(checkedExpr, actualType, data.expType); } - private void resetImpConversionExpr(BLangExpression expr, BType actualType, BType targetType) { + protected void resetImpConversionExpr(BLangExpression expr, BType actualType, BType targetType) { expr.impConversionExpr = null; types.setImplicitCastExpr(expr, actualType, targetType); } diff --git a/compiler/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/util/CompilerUtils.java b/compiler/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/util/CompilerUtils.java index 94bc63139e90..c78ed10ae522 100644 --- a/compiler/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/util/CompilerUtils.java +++ b/compiler/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/util/CompilerUtils.java @@ -19,10 +19,18 @@ import org.ballerinalang.compiler.CompilerOptionName; import org.ballerinalang.model.elements.PackageID; +import org.ballerinalang.model.tree.NodeKind; +import org.wso2.ballerinalang.compiler.semantics.analyzer.Types; import org.wso2.ballerinalang.compiler.semantics.model.symbols.BSymbol; import org.wso2.ballerinalang.compiler.semantics.model.symbols.Symbols; +import org.wso2.ballerinalang.compiler.semantics.model.types.BField; +import org.wso2.ballerinalang.compiler.semantics.model.types.BRecordType; +import org.wso2.ballerinalang.compiler.semantics.model.types.BType; import org.wso2.ballerinalang.compiler.tree.BLangFunction; +import org.wso2.ballerinalang.compiler.tree.BLangNode; import org.wso2.ballerinalang.compiler.tree.BLangSimpleVariable; +import org.wso2.ballerinalang.compiler.tree.expressions.BLangFieldBasedAccess; +import org.wso2.ballerinalang.compiler.tree.statements.BLangAssignment; import java.util.List; @@ -82,4 +90,17 @@ public static boolean isInParameterList(BSymbol symbol, List () { %0(RETURN) (); %1(LOCAL) R3; %2(TEMP) typeDesc; - %4(TEMP) int|(); - %5(TEMP) int; - %7(TEMP) string; + %4(TEMP) int; + %6(TEMP) string; + %7(TEMP) int|(); + %8(TEMP) (); bb0 { %2 = newType R3; %1 = NewMap %2{}; - %5 = ConstLoad 2; - %4 = %5; - %7 = ConstLoad x; - %1[%7] = %4; + %4 = ConstLoad 2; + %6 = ConstLoad x; + %1[%6] = %4; + %8 = ConstLoad 0; + %7 = %8; + %6 = ConstLoad x; + %1[%6] = %7; %0 = ConstLoad 0; GOTO bb1; } diff --git a/tests/jballerina-unit-test/src/test/resources/test-src/bir/record_desugar.bal b/tests/jballerina-unit-test/src/test/resources/test-src/bir/record_desugar.bal index 05e6482eb102..2e0e71087812 100644 --- a/tests/jballerina-unit-test/src/test/resources/test-src/bir/record_desugar.bal +++ b/tests/jballerina-unit-test/src/test/resources/test-src/bir/record_desugar.bal @@ -24,4 +24,5 @@ function setNillableField() { function setOptionalField() { R3 r3 = {}; r3.x = 2; + r3.x = (); } diff --git a/tests/jballerina-unit-test/src/test/resources/test-src/expressions/access/optional_field_access.bal b/tests/jballerina-unit-test/src/test/resources/test-src/expressions/access/optional_field_access.bal index 2ae35c7d0842..ad35feb9ab00 100644 --- a/tests/jballerina-unit-test/src/test/resources/test-src/expressions/access/optional_field_access.bal +++ b/tests/jballerina-unit-test/src/test/resources/test-src/expressions/access/optional_field_access.bal @@ -622,6 +622,88 @@ public function testNestedOptionalFieldAccessOnIntersectionTypes() { assertEquality((), v5); } +type MyInt int; +type Rxx record { + int val; +}; +type Rxy record { + int val; + float otherVal; +}; +type Rx record {| + int a?; + MyInt b?; + Rxx c?; +|}; + +function testSubtypeAssignment() { + int:Signed16 init = 2; + Rx r = {a:init, b:init}; + int:Signed32 val = 5; + r.a = val; + assertEquality(val, r.a); + r.a = (); + assertFalse(r.hasKey("a")); + r.b = val; + assertEquality(val, r.b); + r.b = (); + assertFalse(r.hasKey("b")); + Rxy c = {val: 5, otherVal: 10.0}; + r.c = c; + assertEquality(c, r.c); + r.c = (); + assertFalse(r.hasKey("b")); +} + +function testNullableAssignment() { + Rx r = {}; + int? val = 12; + r.a = val; + assertEquality(val, r.a); + int? b = (); + r.a = b; + assertFalse(r.hasKey("a")); + Rxy? c = {val: 5, otherVal: 10.0}; + r.c = c; + assertEquality(c, r.c); + c = (); + r.c = c; + assertFalse(r.hasKey("c")); +} + +type MyUnion int|float|string; +type Ry record {| + int|float|string a?; + MyUnion b?; + int|Rxx c?; +|}; + +function testUnionAssignment() { + int init = 5; + Ry r = {a:init, b:init}; + int:Signed32 val = 5; + r.a = val; + assertEquality(val, r.a); + int|float val2 = 10; + r.a = val2; + assertEquality(val2, r.a); + r.a = (); + assertFalse(r.hasKey("a")); + + r.b = val; + assertEquality(val, r.b); + r.b = val2; + assertEquality(val2, r.b); + r.b = (); + assertFalse(r.hasKey("b")); + + Rxx c = {val: 5}; + r.c = c; + assertEquality(c, r.c); + r.c = (); + assertFalse(r.hasKey("b")); +} + function assertTrue(anydata actual) { assertEquality(true, actual); } From 6551fb803f4d5ab3f3ab4df6f6c86894f9188c8d Mon Sep 17 00:00:00 2001 From: Heshan Padmasiri Date: Fri, 5 Apr 2024 07:37:08 +0530 Subject: [PATCH 6/6] Add comment explaining workaround --- .../java/org/wso2/ballerinalang/compiler/desugar/Desugar.java | 2 ++ 1 file changed, 2 insertions(+) diff --git a/compiler/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/desugar/Desugar.java b/compiler/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/desugar/Desugar.java index c7be1ef41e4b..43f64cc97689 100644 --- a/compiler/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/desugar/Desugar.java +++ b/compiler/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/desugar/Desugar.java @@ -2476,6 +2476,8 @@ private void createSimpleVarDefStmt(BLangSimpleVariable simpleVariable, BLangBlo @Override public void visit(BLangAssignment assignNode) { + // We rewrite the varRef of the BLangAssignment to a IndexBasedAssignment if it is a FieldBasedAssignment. + // Therefore we must do the shouldWidenExpressionTypeWithNil check before that. boolean addNilToCastingType = shouldWidenExpressionTypeWithNil(assignNode); assignNode.varRef = rewriteExpr(assignNode.varRef); assignNode.expr = rewriteExpr(assignNode.expr);