Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Improvement]: Refactor BUnionType's getMemberTypes() usages #43344

Closed
lochana-chathura opened this issue Sep 2, 2024 · 16 comments
Closed

[Improvement]: Refactor BUnionType's getMemberTypes() usages #43344

lochana-chathura opened this issue Sep 2, 2024 · 16 comments
Assignees
Labels
Area/SemtypeIntegration Issue related to integrating semtype engine Team/CompilerFE All issues related to Language implementation and Compiler, this exclude run times. Type/Improvement

Comments

@lochana-chathura
Copy link
Member

lochana-chathura commented Sep 2, 2024

Description

$subject. We need to get rid of getMemberTypes() and getOriginalMemberTypes() usages in the current compiler, in order to do #43343.

This issue depends on #43126 which is in PR sent state now.

@lochana-chathura lochana-chathura added Type/Improvement Team/CompilerFE All issues related to Language implementation and Compiler, this exclude run times. Area/SemtypeIntegration Issue related to integrating semtype engine labels Sep 2, 2024
@lochana-chathura lochana-chathura self-assigned this Sep 2, 2024
@lochana-chathura lochana-chathura moved this from On Hold to In Progress in Ballerina Team Main Board Sep 4, 2024
@lochana-chathura
Copy link
Member Author

lochana-chathura commented Oct 1, 2024

  • Found a bug in isSimpleNode() where AllOrNothingSubtype instance checks are carried out instead of BddAllOrNothing instance checks. This resulted in not giving correct values for Core.mappingAtomicType().
  • New API method Optional<List<MappingAtomicType>> mappingAtomicTypesInUnion(Context, SemType) introduced to implement isLaxType() check using semtypes.

Fixed with 4f137ee

@lochana-chathura
Copy link
Member Author

When converting isSameType() usages to semtypes, here at

the same BObjectType instance with one inside a BUnionType wrapper, evaluate not to be the same type, in the current compiler.

Screenshot 2024-10-02 at 07 50 51

We are comparing BUnionType 5262 to BObjectType 5231

With semTypes, they both identified to be the same type, causing NPE at later stage

@lochana-chathura
Copy link
Member Author

When rewriting isDistinctAllowedOnType() using semtypes, it was found that the same method is called twice for an BObjectType causing the wrong resolution of distinct type IDs for the semtype. When calling [1], BTypeIdSet info is not available, resulting in the wrong semtype resolution.

Two places:
[1]

[2]

Removing [1] fixed the issue.

@lochana-chathura
Copy link
Member Author

Found another issue related to isDistinctAllowedOnType(). When calling it at Symbol Resolver, the BObject type's symbol does not have the attachedFuncs values updated, causing a wrong resolution of semtype for the object type.

type Obj0 distinct object {
    function apply() returns int;
};

type Obj2 object {
    function apply() returns int;
};

function foo(Obj0 x) {
    Obj2 y = x; // Should have no error
}

@lochana-chathura
Copy link
Member Author

lochana-chathura commented Oct 3, 2024

After rewriting isAllReadonlyTypes() using semtypes at,

the following test that used compile will now give an error,

class P1 {
    Q1 q;
    string p1;

    function init() {
        self.q = new;
        self.p1 = "P1";
    }
}

class Q1 {
    P1 p;
    string q1;

    function init() {
        self.p = new;
        self.q1 = "Q1";
    }
}

function testArraysOfCyclicDependentTypes3() returns P1[] {
    P1[] arr = [];
    arr[3] = new; // ERROR [test_parser.bal:(23:5,23:11)] cannot update 'readonly' value of type 'P1[]'
    return arr;
}

function testArraysOfCyclicDependentTypes4() returns Q1[] {
    Q1[] arr = [];
    arr[3] = new; // ERROR [test_parser.bal:(29:5,29:11)] cannot update 'readonly' value of type 'Q1[]'
    return arr;
}

This is because in semtype P1 and Q1 are considered to have an empty value space making them equivalent to never

@lochana-chathura
Copy link
Member Author

lochana-chathura commented Oct 4, 2024

const int[2]|[int, int] ERR_TUPLE5 = []; // ambiguous types

With the use of isSameType() semtype based API, the above will no longer give the ambiguous type error.

@lochana-chathura
Copy link
Member Author

Sample:

function testBitWiseOperationsForNullable() {
    byte? x = 251;
    int:Signed8? y = -123;

    var ans7 = x >> y;
}

Replacing the following isSameType() with semtype-based API would crash as function (int:Unsigned8?,int:Signed8?) returns (int:Unsigned8?) considered to be resolved operator symbol rather than function (byte?,int:Signed8?) returns (byte?).

BIR diff:
Screenshot 2024-10-08 at 15 29 03

crash log (click to expand)
org.ballerinalang.compiler.BLangCompilerException: error while generating method 'testBitWiseOperationsForNullable' in class 'test_parser'
	at org.wso2.ballerinalang.compiler.bir.codegen.JvmCodeGenUtil.visitMaxStackForMethod(JvmCodeGenUtil.java:760)
	at org.wso2.ballerinalang.compiler.bir.codegen.methodgen.MethodGen.genJMethodForBFunc(MethodGen.java:390)
	at org.wso2.ballerinalang.compiler.bir.codegen.methodgen.MethodGen.generateMethod(MethodGen.java:186)
	at org.wso2.ballerinalang.compiler.bir.codegen.JvmPackageGen.lambda$generateModuleClasses$0(JvmPackageGen.java:420)
	at java.base/java.util.HashMap.forEach(HashMap.java:1421)
	at org.wso2.ballerinalang.compiler.bir.codegen.JvmPackageGen.generateModuleClasses(JvmPackageGen.java:377)
	at org.wso2.ballerinalang.compiler.bir.codegen.JvmPackageGen.generate(JvmPackageGen.java:765)
	at org.wso2.ballerinalang.compiler.bir.codegen.CodeGenerator.generate(CodeGenerator.java:86)
	at org.wso2.ballerinalang.compiler.bir.codegen.CodeGenerator.generate(CodeGenerator.java:63)
	at io.ballerina.projects.JBallerinaBackend.performCodeGen(JBallerinaBackend.java:377)
	at io.ballerina.projects.ModuleContext.generateCodeInternal(ModuleContext.java:458)
	at io.ballerina.projects.ModuleCompilationState$4.generatePlatformSpecificCode(ModuleCompilationState.java:132)
	at io.ballerina.projects.ModuleContext.generatePlatformSpecificCode(ModuleContext.java:358)
	at io.ballerina.projects.JBallerinaBackend.performCodeGen(JBallerinaBackend.java:178)
	at io.ballerina.projects.JBallerinaBackend.<init>(JBallerinaBackend.java:144)

@lochana-chathura
Copy link
Member Author

When replacing the types.isSameBIRShape() call in BIRPackageSymbolEnter with semtype based isSameType(), encountered with the following crash.

stacktrace (click to expand)
ballerina-new-parser-test-suite > jballerina-test > org.ballerinalang.test.bala.types.TypeNameWithSpecialCharsBalaTest > testTypeNameWithSpecialChars FAILED
    org.ballerinalang.compiler.BLangCompilerException: failed to load the module 'specialchars/pkg:0.0.0' from its BIR due to: Cannot invoke "org.wso2.ballerinalang.compiler.semantics.model.types.BType.semType()" because "this.referredType" is null
        at app//org.wso2.ballerinalang.compiler.BIRPackageSymbolEnter.definePackage(BIRPackageSymbolEnter.java:239)
        at app//org.wso2.ballerinalang.compiler.BIRPackageSymbolEnter.definePackage(BIRPackageSymbolEnter.java:215)
        at app//io.ballerina.projects.ModuleContext.loadPackageSymbolInternal(ModuleContext.java:528)
        at app//io.ballerina.projects.ModuleCompilationState$6.compile(ModuleCompilationState.java:178)
        at app//io.ballerina.projects.ModuleContext.compile(ModuleContext.java:354)
        at app//io.ballerina.projects.PackageCompilation.compileModulesInternal(PackageCompilation.java:211)
        at app//io.ballerina.projects.PackageCompilation.compileModules(PackageCompilation.java:195)
        at app//io.ballerina.projects.PackageCompilation.compile(PackageCompilation.java:102)
        at app//io.ballerina.projects.PackageCompilation.from(PackageCompilation.java:97)
        at app//io.ballerina.projects.PackageContext.getPackageCompilation(PackageContext.java:243)
        at app//io.ballerina.projects.Package.getCompilation(Package.java:154)
        at app//org.ballerinalang.test.BCompileUtil.jBallerinaBackend(BCompileUtil.java:224)
        at app//org.ballerinalang.test.BCompileUtil.compile(BCompileUtil.java:99)
        at app//org.ballerinalang.test.bala.types.TypeNameWithSpecialCharsBalaTest.testTypeNameWithSpecialChars(TypeNameWithSpecialCharsBalaTest.java:43)

        Caused by:
        java.lang.NullPointerException: Cannot invoke "org.wso2.ballerinalang.compiler.semantics.model.types.BType.semType()" because "this.referredType" is null
            at org.wso2.ballerinalang.compiler.semantics.model.types.BTypeReferenceType.semType(BTypeReferenceType.java:44)
            at org.wso2.ballerinalang.compiler.semantics.model.types.BRecordType.semType(BRecordType.java:171)
            at org.wso2.ballerinalang.compiler.semantics.analyzer.SemTypeHelper.semType(SemTypeHelper.java:165)
            at org.wso2.ballerinalang.compiler.semantics.analyzer.SemTypeHelper.semType(SemTypeHelper.java:109)
            at org.wso2.ballerinalang.compiler.semantics.analyzer.Types.isSameTypeWithTags(Types.java:406)
            at org.wso2.ballerinalang.compiler.semantics.analyzer.Types.isSameBIRShape(Types.java:2498)
            at org.wso2.ballerinalang.compiler.BIRPackageSymbolEnter.getType(BIRPackageSymbolEnter.java:2166)
            at org.wso2.ballerinalang.compiler.BIRPackageSymbolEnter$BIRTypeReader.readType(BIRPackageSymbolEnter.java:1364)
            at org.wso2.ballerinalang.compiler.BIRPackageSymbolEnter.readBType(BIRPackageSymbolEnter.java:715)
            at org.wso2.ballerinalang.compiler.BIRPackageSymbolEnter$BIRTypeReader.readTypeFromCp(BIRPackageSymbolEnter.java:1185)
            at org.wso2.ballerinalang.compiler.BIRPackageSymbolEnter$BIRTypeReader.readType(BIRPackageSymbolEnter.java:1551)
            at org.wso2.ballerinalang.compiler.BIRPackageSymbolEnter.readBType(BIRPackageSymbolEnter.java:715)
            at org.wso2.ballerinalang.compiler.BIRPackageSymbolEnter$BIRTypeReader.readTypeFromCp(BIRPackageSymbolEnter.java:1185)
            at org.wso2.ballerinalang.compiler.BIRPackageSymbolEnter$BIRTypeReader.readType(BIRPackageSymbolEnter.java:1385)
public type Bdd Node|boolean;

public type Node readonly & record {|
    int atom;
    Bdd left;
    Bdd middle;
    Bdd right;
|};

The reason is in the Node record type, the referred types for left, middle and right are null when calling at BIRPackageSymbolEnter. However, via the previous API since we don't access the type(referred type's symbol is used instead) it was not a problem.

Failing test: TypeNameWithSpecialCharsBalaTest

@lochana-chathura
Copy link
Member Author

lochana-chathura commented Oct 21, 2024

The reason is in the Node record type, the referred types for left, middle and right are null when calling at BIRPackageSymbolEnter. However, via the previous API since we don't access the type(referred type's symbol is used instead) it was not a problem.

The reason for null was that the type was still resolving. For BReferenceType we update the referredType after creating the instance. It turned out isSameBIRShape() was a temporary workaround(See: #35872 (comment)) and the original issue has been fixed with PR #36168.

Hence, isSameBIRShape() call was removed.

@lochana-chathura
Copy link
Member Author

lochana-chathura commented Oct 21, 2024

  • BSameTypeVisitor and BIRSameShapeVisitor visitors have now been removed. The former was replaced with the isSameType() API from semtype module.
  • BOrderedTypeVisitor has been removed. Used comparable() API instead, from nBallerina. For that needed to port extra few APIs from there. Additionally, in the nBallerina API, there was a missing part, because of that the following is considered comparable in nBallerina. ("There must be an ordered type to which the static type of both operands belong" part from spec was not implemented in nBallerina) We need to check whether the accumulated types belong to the same ordered type if the lengths are different.
type X [(), (), (), (), int, (), (), string];
type Y [5.0];

@lochana-chathura
Copy link
Member Author

lochana-chathura commented Oct 25, 2024

Found an inconsistency in current interop method return type validation.
e.g.
ballerina functions:

function getNilAsReadOnly() returns error? = @java:Method { // <---------------[1]
    'class: "org/ballerinalang/nativeimpl/jvm/tests/StaticMethods"
} external;

function getNilAsReadOnly() returns readonly = @java:Method { // <---------------[2]
    'class: "org/ballerinalang/nativeimpl/jvm/tests/StaticMethods"
} external;

function getBooleanAsReadOnly() returns readonly = @java:Method { // <---------------[3]
    'class: "org/ballerinalang/nativeimpl/jvm/tests/StaticMethods"
} external;

java methods:

    public static void getNilAsReadOnly() {
    }

    public static boolean getBooleanAsReadOnly() {
        return true;
    }

For [1] a compile error is given because error is present in the return type, however java method does give exception or anything.
For [2] and [3] no compiler error. Although it is not explicitly declared readonly does have the error within.

Inconsistency is fixed with e70628b. In the new behavior all 3 cases are allowed. No compile error.

@lochana-chathura
Copy link
Member Author

lochana-chathura commented Oct 25, 2024

  • The following bal code that used to compile, now gives compile-time errors as commented down.
import ballerina/test;

type IntOne 1;
type FloatOne 1.0;
type IntTwo 2;
type FloatTwo 2f;

function checkFiniteTypeEquality() {
    IntOne intOne_1 = 1;
    IntOne intOne_2 = 1;
    IntTwo intTwo = 2;
    FloatOne floatOne = 1f;
    FloatTwo floatTwo = 2.0;

    test:assertTrue((intOne_1 == intOne_2) && !(intOne_1 != intOne_2));
    test:assertTrue((floatOne != floatTwo) && !(floatOne == floatTwo)); // error - line 16
    test:assertFalse((intOne_1 == intTwo) && !(intOne_1 != intTwo)); // error - line 17
}

type Array ["array", 1];
type Mapping ["mapping", 2];

function checkTupleEqualityNegative() {
    Array a = ["array", 1];
    Mapping b = ["mapping", 2];
    test:assertFalse(a == b); // error - line 26
}

//ERROR [test_parser.bal:(16:22,16:42)] operator '!=' not defined for 'FloatOne' and 'FloatTwo'
//ERROR [test_parser.bal:(16:49,16:69)] operator '==' not defined for 'FloatOne' and 'FloatTwo'
//ERROR [test_parser.bal:(17:23,17:41)] operator '==' not defined for 'IntOne' and 'IntTwo'
//ERROR [test_parser.bal:(17:48,17:66)] operator '!=' not defined for 'IntOne' and 'IntTwo'
//ERROR [test_parser.bal:(26:22,26:28)] operator '==' not defined for 'Array' and 'Mapping'
  • The following bal code that used to give compile-time errors, now compiles with no errors.
function checkEqualityOfMapsOfIncompatibleConstraintTypes() returns boolean {
    map<int> a = {};
    map<float> b = {};
    boolean bool1 = a == b && !(a != b); // no error

    map<string|int> c = {};
    map<float> d = {};
    boolean bool2 = c == d && !(c != d); // no error

    return bool1 && bool2;
}

type EmployeeWithOptionalId record {|
    string name;
    float id?;
|};

type PersonWithOptionalId record {|
    string name;
    string id?;
|};

function checkEqualityOfRecordsOfIncompatibleTypes() returns boolean {
    EmployeeWithOptionalId e1 = { name: "Maryam" };
    PersonWithOptionalId p1 = { name: "Maryam" };
    return b && e1 == p1 && !(e1 != p1); // no error
}

function readonlyEquality() returns boolean {
    map<int> & readonly immutableMarks = {
        math: 80,
        physics: 85,
        chemistry: 75
    };
    readonly readonlyMarks = immutableMarks;

    map<int> marks = {
        math: 80,
        physics: 85,
        chemistry: 75
    };
    return readonlyMarks != marks; // no error
}

function cannotDeepEqualReadonly() returns boolean {
    readonly arr = [1, 2 , 3];
    return arr == [1, 2 , 3]; // no Error
}

According to equality-expr section in spec,

An equality-expr tests whether two values are equal. For all four operators, it is a compile time error if the intersection of the static types of the operands is empty.

So the new behavior is correct. We need to improve the compile time error message.

  • Following unreachability error tests now give compile-time errors at an early stage. (Again error message has to be improved here)
function testUnreachabilityWithIfElseStmts6() {
    10|20 e = 10;
    if e == 10 {
        int _ = 10;
    } else if e == 20 {
        int _ = 20;
    } else if e == 10 { // New error: operator '==' not defined for 'never' and 'int'
        never _ = e; // Previous error: unreachable code
    }
}
function testUnreachabilityWithIfStmtWithEqualityExpr15() {
    True t = true;
    False f = false;

    if f == t { // New error: operator '==' not defined for 'false' and 'true'
        return; // Previous error: unreachable code 
    } else if t == t {
        string _ = "Ballerina";
    } else {
        string _ = "Ballerina"; // Previous error: unreachable code 
    }
}

@lochana-chathura
Copy link
Member Author

lochana-chathura commented Oct 31, 2024

In case of inferred isolation, the isolated tag is added at IsolationAnalyzer. This will mutate the semtype. Therefore, 9af49c6 was added to reset semtype in case of a flag mutation.

With that fix EscapedIdentifiersValidationTest failure was resolved. However, the fix uncovered a bug in the existing implementation. The affected test case is FunctionTests.testFunctionalProgramming.

In the previous BSameTypeVisitor, two functions were assumed to be the same if the source has the isolated flag and the target does not have the isolated flag. Since now they are not evaluated to the same, a new ImplicitCastExpr is added here

. In the new cast we ignore the BTypeReferenceType wrapper as per the following logic.
if (lhsType.tag == TypeTags.TYPEREFDESC && rhsType.tag != TypeTags.TYPEREFDESC) {
return addConversionExprIfRequired(expr, Types.getReferredType(lhsType));
}

Therefore, the resultant runtime type will not have the reference type name propagated.
Old type string: IntFilter
New type string: isolated function (int) returns (boolean)

This caused the FunctionTests.testFunctionalProgramming failure.

lochana-chathura added a commit to lochana-chathura/ballerina-lang that referenced this issue Oct 31, 2024
lochana-chathura added a commit to lochana-chathura/ballerina-lang that referenced this issue Nov 1, 2024
@lochana-chathura
Copy link
Member Author

lochana-chathura commented Nov 4, 2024

After rewriting the isTypeCastable() method with seam types, the following samples used give compile-time errors, now compile with no errors.

  1. BasicTupleTest.java
function testTupleToJSONAssignmentNegative() {
    xml A = xml `xml string`;
    [string, int|xml, string...] C = ["text1", 1, A.toString()];
    json jsonTest = <json[]>C; // No compile-time error for the cast.
}
  1. TableCastTest.java
type Teacher record {
    readonly string name;
    readonly int age;
    string school;
};

type Customer record {
    readonly int id;
    readonly string name;
    string lname;
};

type CustomerTable table<Customer|Teacher>;

CustomerTable tab3 = table key(name) [
    {id: 13, name: "Foo", lname: "QWER"},
    {id: 13, name: "Bar", lname: "UYOR"}
];

type Customer2 record {|
    int id;
    string name;
    string lname;
|};

type CustomerEmptyKeyedTbl table<Customer2> key();

function testCastingWithEmptyKeyedKeylessTbl() {
    CustomerEmptyKeyedTbl tbl1 = <CustomerEmptyKeyedTbl>tab3; // No compile-time error for the cast.

    table<record {
        int id;
        string name;
        string lname;
    }> key() tbl = <table<record {
        int id;
        string name;
        string lname;
    }> key()>tab3; // No compile-time error for the cast.
}

In both cases, non-empty intersections exist. Therefore new behavior is acceptable. Need to make sure these casts are not crashing at runtime.

  1. ConstrainedMapTest.testConstrainedMapNegative()
type Student record {
    int index;
    int age;
};

type Person record {
    string name;
    int age;
    string address;
};

function testInvalidStructEquivalentCastCaseTwo() returns (map<Student>) {
    map<Person> testPMap = {};
    map<Student> testSMap = <map<Student>>testPMap; // No compile-time error for the cast.
    return testSMap;
}

This is because map<Person> and map<Student> do have the intersection value {} in common.

  1. TypeCastExpressionsTest.testCastNegatives()
function testTypeCastInConstructorMemberWithUnionCETNegative() {
    string[]|int val1 = [];
    byte[] a = <byte[]> val1; // No compile-time error for the cast.
}

Similarly, this is allowed because byte[] and string[] have [] in common.

  1. ObjectEquivalencyNegativeTest.java
public class person01 {

    public int age = 0;
    public string name = "";
    public string address = "";

}

public class employee01 {

    public int age = 0;
    public string name = "";
    public string zipcode = "95134";

    function init (int age, string name) {
        self.age = age;
        self.name = name;
    }
}

function f5() returns string {
    employee01 e = new (14, "rat");
    person01 p = <person01> e; // No compile-time error for the cast.
    return p.name;
}

Out of the above ones, 1, 3, 4, and 5 give errors at runtime. 2 runs without any errors.

Relevant commit: 3c58404

@lochana-chathura
Copy link
Member Author

lochana-chathura commented Nov 6, 2024

After rewriting intersectionExists() method in CodeAnalyzer.java using semtype intersection,
The following samples that used to go to runtime(NeverTypeTest class), now give errors at compile-time.

function testNeverRuntime10() {
    int x = 100;
    boolean b = x is never; // incompatible types: 'int' will not be matched to 'never'
    assertEquality(false, b);
}

type Record record {|
    int i;
    never[] j;
|};


function testNeverRuntime11() {
    Record x = { i: 1, j: [] };
    boolean b = x is never; // incompatible types: 'Record' will not be matched to 'never'
    assertEquality(false, b);
}
function testNeverFieldTypeCheck() {
    record {int x;} r2 = {x: 2, "color": "blue"};
    assertEquality(false, r2 is record {never x?;}); // incompatible types: 'record {| int x; anydata...; |}' will not be matched to 'record {| never x?; anydata...; |}'

    record {never? x;} r3 = {x: (), "color": "blue"};
    assertEquality(false, r3 is record {never x?;}); // incompatible types: 'record {| never? x; anydata...; |}' will not be matched to 'record {| never x?; anydata...; |}'

    record {int? x;} r4 = {x: 2, "color": "blue"};
    assertEquality(false, r4 is record {never x?;}); // incompatible types: 'record {| int? x; anydata...; |}' will not be matched to 'record {| never x?; anydata...; |}'

    record {int x;} & readonly v3 = {x: 2, "color": "blue"};
    assertEquality(false, v3 is record {never x?;}); // incompatible types: '(record {| int x; anydata...; |} & readonly)' will not be matched to 'record {| never x?; anydata...; |}'

    record {never? x;} & readonly v4 = {x: (), "color": "blue"};
    assertEquality(false, v4 is record {never x?;}); // incompatible types: '(record {| never? x; anydata...; |} & readonly)' will not be matched to 'record {| never x?; anydata...; |}'
}

@lochana-chathura lochana-chathura moved this from In Progress to PR Sent in Ballerina Team Main Board Nov 12, 2024
lochana-chathura added a commit to lochana-chathura/ballerina-lang that referenced this issue Nov 13, 2024
@lochana-chathura
Copy link
Member Author

Closing with #43341

@github-project-automation github-project-automation bot moved this from PR Sent to Done in Ballerina Team Main Board Nov 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area/SemtypeIntegration Issue related to integrating semtype engine Team/CompilerFE All issues related to Language implementation and Compiler, this exclude run times. Type/Improvement
Projects
Status: Done
Development

No branches or pull requests

1 participant