Rewrite BUnionType's `getMemberTypes()` usages using SemTypes #43341

lochana-chathura · 2024-08-30T19:58:10Z

Purpose

$subject

Fixes #43344
Fixes #43437

Approach

n/a

Samples

n/a

Remarks

n/a

Check List

codecov · 2024-08-30T21:55:45Z

Codecov Report

Attention: Patch coverage is 91.78082% with 12 lines in your changes missing coverage. Please review.

Project coverage is 77.55%. Comparing base (ce35671) to head (ff06641).
Report is 1 commits behind head on nutcracker.

Files with missing lines	Patch %	Lines
...alang/compiler/semantics/analyzer/TypeChecker.java	79.31%	2 Missing and 4 partials ⚠️
...ballerinalang/compiler/desugar/ASTBuilderUtil.java	0.00%	0 Missing and 1 partial ⚠️
...2/ballerinalang/compiler/desugar/QueryDesugar.java	83.33%	0 Missing and 1 partial ⚠️
...mpiler/semantics/analyzer/ConstantTypeChecker.java	50.00%	0 Missing and 1 partial ⚠️
.../compiler/semantics/analyzer/SemanticAnalyzer.java	87.50%	0 Missing and 1 partial ⚠️
...alang/compiler/semantics/analyzer/SymbolEnter.java	91.66%	1 Missing ⚠️
...compiler/semantics/analyzer/TypeParamAnalyzer.java	0.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@               Coverage Diff                @@
##             nutcracker   #43341      +/-   ##
================================================
+ Coverage         77.47%   77.55%   +0.08%     
+ Complexity        58442    58128     -314     
================================================
  Files              3449     3449              
  Lines            219783   218626    -1157     
  Branches          28815    28377     -438     
================================================
- Hits             170269   169549     -720     
+ Misses            40047    39690     -357     
+ Partials           9467     9387      -80

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

heshanpadmasiri · 2024-11-10T08:38:52Z

...ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/bir/codegen/JvmTypeTestGen.java

+        SemType otherTy = null;
+        for (SemType s : ((BUnionType) sourceType).getMemberSemTypes()) {
+            if (PredefinedType.NIL.equals(s)) {


Is there a reason why we can't do containsBasicType here? (since nil don't have subtypes)

Fixed in e2ce145

compiler/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/desugar/Desugar.java

...ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/bir/codegen/JvmTypeTestGen.java

heshanpadmasiri · 2024-11-10T08:58:34Z

...ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/bir/codegen/JvmTypeTestGen.java

        int foundError = 0;
-        for (BType bType : ((BUnionType) sourceType).getMemberTypes()) {
-            if (bType.tag == TypeTags.ERROR) {
+        for (SemType s : ((BUnionType) sourceType).getMemberSemTypes()) {


If I understand this correctly what we are actually trying to check is if we have a is E where E is a error subtype and error part of a is a subtype of E. (In which case if a is a BError above type check passes so we just do instanceof check). If that is the case wouldn't it be easier to,

Check target is isSubTypeSimple(error)

Check SemType errorPart = intersect(sourceType, error); -> errorPart is not never

Check if errorPart is a subtype of target

Thanks. Fixed in 445ecc9

...ler/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/desugar/ServiceDesugar.java

heshanpadmasiri · 2024-11-10T09:06:25Z

...rina-lang/src/main/java/org/wso2/ballerinalang/compiler/semantics/analyzer/CodeAnalyzer.java

-        }
-
-        return true;
+        return !Core.isEmpty(types.typeCtx(), Core.diff(returnType.semType(), PredefinedType.ERROR));


Isn't this same as !Core.isSubtyp(returnType.semType(), PredefinedType.ERROR))? Also in the previous code we had handled the case where returnType could be null don't we need to handle it here a well?

Fixed the former. I think the null check we had previously is redundant/obsolete. No test for that. Hence, removed.

...rina-lang/src/main/java/org/wso2/ballerinalang/compiler/semantics/analyzer/CodeAnalyzer.java

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

heshanpadmasiri · 2024-11-10T09:33:06Z

...ina-lang/src/main/java/org/wso2/ballerinalang/compiler/semantics/model/types/BUnionType.java

+     * When the type is mutated we need to reset resolved semType.
+     */
+    public void resetSemType() {
+        this.semType = null;


Shouldn't we also clear the memberSemTypes as well?

We repopulate the memberSemTypes here, whenever the semtype is null.

ballerina-lang/compiler/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/semantics/model/types/BUnionType.java

Lines 485 to 492 in efc16e5

public void populateMemberSemTypes(boolean ignoreTypeIds) {

LinkedHashSet<SemType> memberSemTypes = new LinkedHashSet<>();

for (BType memberType : this.memberTypes) {

populateMemberSemTypes(memberType, memberSemTypes, ignoreTypeIds);

}

this.memberSemTypes = memberSemTypes;

Since ballerina-platform#35886 has been addressed, we should be able to remove the temporary shape check workaround. Please refer to ballerina-platform#35872 (comment)

Previously ballerina `readonly` return type allowed for java void methods. However, `error?` was not allowed with java void. This inconsistency is due to type checking not recognizing the fact that readonly type does include the error type. The error given in the latter case too is wrong because error is optional.

Previously this was implemented with a temporary workaround which was inefficient as we recalculated the semtypes.

Please refer to ballerina-platform#43344 (comment) for the reason

lochana-chathura added Team/CompilerFE All issues related to Language implementation and Compiler, this exclude run times. Area/SemtypeIntegration Issue related to integrating semtype engine labels Aug 30, 2024

lochana-chathura force-pushed the semIntegration/cleanup3 branch 2 times, most recently from 6d652a5 to dfb9b06 Compare September 4, 2024 16:44

lochana-chathura force-pushed the semIntegration/cleanup3 branch 5 times, most recently from 9af942a to f8d8a47 Compare October 3, 2024 06:35

lochana-chathura mentioned this pull request Oct 15, 2024

[Bug]: No error when distinct is used for an object and error type union #43437

Open

lochana-chathura force-pushed the semIntegration/cleanup3 branch 4 times, most recently from ad2e562 to f503052 Compare October 23, 2024 06:08

lochana-chathura force-pushed the semIntegration/cleanup3 branch 3 times, most recently from 81e9418 to 6b38208 Compare November 7, 2024 04:20

lochana-chathura changed the title ~~Eliminate BUnionType's getMemberTypes() usages~~ Rewrite BUnionType's getMemberTypes() usages using SemTypes Nov 7, 2024

lochana-chathura marked this pull request as ready for review November 7, 2024 07:04

lochana-chathura requested review from gimantha, MaryamZi, KavinduZoysa, warunalakshitha, azinneera, sameerajayasoma and hasithaa as code owners November 7, 2024 07:04

lochana-chathura requested a review from heshanpadmasiri November 7, 2024 07:04

heshanpadmasiri reviewed Nov 10, 2024

View reviewed changes

lochana-chathura force-pushed the semIntegration/cleanup3 branch from d1ee4ee to b54109b Compare November 11, 2024 07:47

lochana-chathura added 25 commits November 13, 2024 07:31

Rewrite isLaxType() check using semtypes

3916457

Avoid BUnionType's getMemberTypes() usages part 2

4ba7f21

Avoid BUnionType's getMemberTypes() usages part 3

bb6332c

Get rid of old BSameTypeVisitor part 1

ae53230

Get rid of old BSameTypeVisitor part 2

f952608

Get rid of BIRSameShapeVisitor

aca8f5a

Since ballerina-platform#35886 has been addressed, we should be able to remove the temporary shape check workaround. Please refer to ballerina-platform#35872 (comment)

Get rid of BSameTypeVisitor part 3

562e9f6

Get rid of BOrderedTypeVisitor

46b4355

Refactor calls to semType

351c09f

Fix spotbugs issues

366ae2c

Rewrite JvmTypeTestGen, JvmTerminatorGen getMemberTypes calls

fc00fca

Implement isAssignableIgnoreObjectTypeIds() properly

9404493

Previously this was implemented with a temporary workaround which was inefficient as we recalculated the semtypes.

Fix :semtypes:javadoc failure

deddfab

Simplify equality-expr operand intersection check using semtypes

99bcfdf

Fix EscapedIdentifiersValidationTest failure due to BType mutation

cb06d16

Refactor code to use semtypes

02a9afa

Fix testFunctionalProgramming shell test failure

7bcd50c

Please refer to ballerina-platform#43344 (comment) for the reason

Rewrite isTypeCastable() using semtypes

41463dd

Rewrite intersectionExists() in CodeAnalyzer using semtypes

9b34bb6

Rewrite isNumericConversionPossible(), getErrorTypes() using semtypes

e67221a

Fix :ballerina-langlib:test:test failure

001cdd5

Refactor code based on the review suggestions

244de44

Simplify JvmTypeTestGen canOptimizeErrorCheck using semtypes

445ecc9

Simplify JvmTypeTestGen canOptimizeNilCheck using semtypes

e2ce145

lochana-chathura force-pushed the semIntegration/cleanup3 branch from 7a8795f to 27a377a Compare November 13, 2024 02:15

lochana-chathura added 2 commits November 13, 2024 07:49

Rewrite annot type desc validation using semtypes

eac0942

Clean up unused methods in SymbolEnter

ff06641

lochana-chathura force-pushed the semIntegration/cleanup3 branch from 27a377a to ff06641 Compare November 13, 2024 02:19

lochana-chathura requested a review from heshanpadmasiri November 13, 2024 04:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rewrite BUnionType's `getMemberTypes()` usages using SemTypes #43341

Rewrite BUnionType's `getMemberTypes()` usages using SemTypes #43341

lochana-chathura commented Aug 30, 2024 •

edited

Loading

codecov bot commented Aug 30, 2024 •

edited

Loading

heshanpadmasiri Nov 10, 2024

lochana-chathura Nov 11, 2024 •

edited

Loading

heshanpadmasiri Nov 10, 2024

lochana-chathura Nov 11, 2024 •

edited

Loading

heshanpadmasiri Nov 10, 2024

lochana-chathura Nov 11, 2024

heshanpadmasiri Nov 10, 2024

lochana-chathura Nov 12, 2024

	public void populateMemberSemTypes(boolean ignoreTypeIds) {
	LinkedHashSet<SemType> memberSemTypes = new LinkedHashSet<>();

	for (BType memberType : this.memberTypes) {
	populateMemberSemTypes(memberType, memberSemTypes, ignoreTypeIds);
	}

	this.memberSemTypes = memberSemTypes;

Rewrite BUnionType's getMemberTypes() usages using SemTypes #43341

Are you sure you want to change the base?

Rewrite BUnionType's getMemberTypes() usages using SemTypes #43341

Conversation

lochana-chathura commented Aug 30, 2024 • edited Loading

Purpose

Approach

Samples

Remarks

Check List

codecov bot commented Aug 30, 2024 • edited Loading

Codecov Report

heshanpadmasiri Nov 10, 2024

Choose a reason for hiding this comment

lochana-chathura Nov 11, 2024 • edited Loading

Choose a reason for hiding this comment

heshanpadmasiri Nov 10, 2024

Choose a reason for hiding this comment

lochana-chathura Nov 11, 2024 • edited Loading

Choose a reason for hiding this comment

heshanpadmasiri Nov 10, 2024

Choose a reason for hiding this comment

lochana-chathura Nov 11, 2024

Choose a reason for hiding this comment

heshanpadmasiri Nov 10, 2024

Choose a reason for hiding this comment

lochana-chathura Nov 12, 2024

Choose a reason for hiding this comment

Rewrite BUnionType's `getMemberTypes()` usages using SemTypes #43341

Rewrite BUnionType's `getMemberTypes()` usages using SemTypes #43341

lochana-chathura commented Aug 30, 2024 •

edited

Loading

codecov bot commented Aug 30, 2024 •

edited

Loading

lochana-chathura Nov 11, 2024 •

edited

Loading

lochana-chathura Nov 11, 2024 •

edited

Loading