[Master] Fix issues when there is union type in the select clause #40835

LakshanWeerasinghe · 2023-06-26T06:13:58Z

Purpose

$subject

Fixes #40013
Fixes #40012

Related PRs #40271, #40726

Approach

Describe how you are implementing the solutions along with the design details.

Samples

Provide high-level details about the samples related to this feature.

Remarks

List any other known issues, related PRs, TODO items, or any other notes related to the PR.

Check List

SasinduDilshara · 2023-07-10T11:54:45Z

Found the issue #40974, which is related to the focused task in this PR, but this issue not happen with this PR, it is already happen in update 6 as well.

codecov · 2023-07-11T05:10:29Z

Codecov Report

Patch coverage: 95.38% and no project coverage change.

Comparison is base (c55bb5f) 76.44% compared to head (6185bb3) 76.45%.
Report is 11 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff            @@
##             master   #40835   +/-   ##
=========================================
  Coverage     76.44%   76.45%           
- Complexity    52244    52273   +29     
=========================================
  Files          2863     2863           
  Lines        197207   197280   +73     
  Branches      25628    25648   +20     
=========================================
+ Hits         150761   150836   +75     
+ Misses        38140    38136    -4     
- Partials       8306     8308    +2

Files Changed	Coverage Δ
.../compiler/semantics/analyzer/QueryTypeChecker.java	`88.74% <94.17%> (+0.69%)`	⬆️
...compiler/semantics/analyzer/IsolationAnalyzer.java	`85.62% <100.00%> (+0.12%)`	⬆️
...alang/compiler/semantics/analyzer/TypeChecker.java	`92.20% <100.00%> (+<0.01%)`	⬆️

... and 5 files with indirect coverage changes

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

.../jballerina-unit-test/src/test/java/org/ballerinalang/test/query/XMLQueryExpressionTest.java

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

LakshanWeerasinghe · 2023-07-11T07:41:15Z

Found the issue #40974, which is related to the focused task in this PR, but this issue not happen with this PR, it is already happen in update 6 as well.

Since this has occurred previously will be addressed in another PR.

tests/jballerina-unit-test/src/test/resources/test-src/query/query_action_or_expr.bal

MaryamZi · 2023-07-11T11:36:59Z

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

@@ -322,89 +323,149 @@ public BType resolveQueryType(SymbolEnv env, BLangExpression selectExp, BType ta
        }
    }



For

type Foo record { Baz[] a; }; type Bar record { boolean[]|float[] b; }; type Baz record { string c; }; type Quux record { string c; int d?; }; type Qux record { string c; int d; }; function transform(Foo foo) returns Bar => { b: from var _ in foo.a select { c: "str" } };

there seems to be an extra error now.

a type compatible with mapping constructor expressions not found in type 'boolean'

Can we avoid this?

This error is there even before the fix. This error comes when we try to type check the select clause againsts the expected member type of the query result.

Will try to find a proper solution to remove this misleading diagnostic message.

Since this error is expected logging is correct. The missing part was the error should be logged for all expected types. In this case a type compatible with mapping constructor expressions not found in type 'float' was missing. Added it in #6a7b01.

What are all the errors for this sample now?

ambiguous type '(Baz|Qux)' will be the only error.

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

github-actions · 2023-08-05T19:33:10Z

This PR has been open for more than 15 days with no activity. This will be closed in 3 days unless the stale label is removed or commented.

pcnfernando · 2023-08-10T16:24:37Z

...s/jballerina-unit-test/src/test/java/org/ballerinalang/test/query/QueryActionOrExprTest.java

@@ -141,6 +145,65 @@ public void testQueryActionOrExprSemanticsNegative() {
        validateError(negativeResult, i++, "action invocation as an expression not allowed here", 315, 23);
        validateError(negativeResult, i++, "incompatible types: expected 'int', found 'other'", 316, 21);
        validateError(negativeResult, i++, "action invocation as an expression not allowed here", 320, 50);
+        validateError(negativeResult, i++, "incompatible types: expected '(T3[]|T4[])', found '(T3|T4)[]'",
+                339, 13);
+        validateError(negativeResult, i++, "incompatible types: expected 'T3', " +


Why we should expect this error?
If we consider a normal ternary expression this is allowed

public function foo(string str) { T3|T4 _ = str == "" ? <T3>{ str: "" } : <T4>{ foo: false }; }

In a normal expression this is correct. But In here we log the error inside the select clause. In the L.H.S it is expecting a T3[]|T4[]. So select clause type being a T3|T4 is incorrect.

Initially we agreed to log a single error for the whole query. But when reviewing this PR @MaryamZi pointed out a #case where we needed type check the select clause as well.

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

MaryamZi · 2023-08-14T04:20:22Z

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

@@ -322,89 +323,149 @@ public BType resolveQueryType(SymbolEnv env, BLangExpression selectExp, BType ta
        }
    }



What are all the errors for this sample now?

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

MaryamZi · 2023-08-14T04:22:51Z

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

+                            checkExpr(selectExp, env, expType, data);
+                            selectExp.typeChecked = false;


Can't we clone and do this instead? Are we sure there's nothing else that needs to be reset?

To avoid such issues and side-effects we usually clone and type check (when checking for compatibility). Check mapping constructor type checking against unions.

Updated in commit.

.../jballerina-unit-test/src/test/java/org/ballerinalang/test/query/XMLQueryExpressionTest.java

tests/jballerina-unit-test/src/test/resources/test-src/query/query_ambiguous_type_negative.bal

pcnfernando · 2023-08-15T14:45:10Z

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

+                            checkExpr(nodeCloner.cloneNode(selectExp), env, expType, data);
+                        }
+                    });
+                    selectExp.typeChecked = true;


At this level selectExp hasn't been typeChecked isn't it? Is it correct to mark it as type-checked?

Yes. There are case that can be where selectExpr not being type checked.

Updated in 30e430.

pcnfernando · 2023-08-16T06:03:47Z

...erina-unit-test/src/test/resources/test-src/query/query_action_or_expr_semantic_negative.bal

+
+function transform(T1 t1) returns T2 => {
+    t3OrT4: from var t3sItem in t1.t3s
+        select t3sItem.str == ""


This specific ternary expression is not correct. Just try running just that sample alone

…esolving selectTypes"" This reverts commit 3231685.

LakshanWeerasinghe requested review from sameerajayasoma and hasithaa as code owners June 26, 2023 06:13

LakshanWeerasinghe requested review from pcnfernando and SasinduDilshara June 26, 2023 06:14

LakshanWeerasinghe force-pushed the fix-#40013 branch from 44c5106 to 0ff6bd7 Compare June 30, 2023 06:14

LakshanWeerasinghe requested review from ushirask, KavinduZoysa, gimantha and MaryamZi July 5, 2023 03:57

LakshanWeerasinghe mentioned this pull request Jul 6, 2023

[Master] Improve diagnostic message when there is a union type in the select clause #40642

Closed

13 tasks

LakshanWeerasinghe force-pushed the fix-#40013 branch from 0ff6bd7 to c21c647 Compare July 10, 2023 09:25

MaryamZi reviewed Jul 11, 2023

View reviewed changes

LakshanWeerasinghe force-pushed the fix-#40013 branch from d329d0d to f74ab40 Compare July 11, 2023 11:01

MaryamZi reviewed Jul 11, 2023

View reviewed changes

LakshanWeerasinghe force-pushed the fix-#40013 branch from f74ab40 to 1849219 Compare July 12, 2023 03:55

LakshanWeerasinghe requested a review from MaryamZi July 12, 2023 18:16

LakshanWeerasinghe force-pushed the fix-#40013 branch from 6a7b016 to b631b2d Compare July 21, 2023 04:19

github-actions bot added the Stale label Aug 5, 2023

gimantha added Team/CompilerFE All issues related to Language implementation and Compiler, this exclude run times. and removed Stale labels Aug 6, 2023

pcnfernando reviewed Aug 10, 2023

View reviewed changes

LakshanWeerasinghe force-pushed the fix-#40013 branch from b631b2d to 3358292 Compare August 13, 2023 16:28

LakshanWeerasinghe requested a review from pcnfernando August 13, 2023 16:44

LakshanWeerasinghe force-pushed the fix-#40013 branch 2 times, most recently from 4522d59 to 6ba4b19 Compare August 14, 2023 01:20

MaryamZi reviewed Aug 14, 2023

View reviewed changes

LakshanWeerasinghe requested a review from MaryamZi August 15, 2023 03:09

SasinduDilshara previously approved these changes Aug 15, 2023

View reviewed changes

pcnfernando reviewed Aug 15, 2023

View reviewed changes

LakshanWeerasinghe dismissed SasinduDilshara’s stale review via 69cfb39 August 15, 2023 17:10

LakshanWeerasinghe requested review from SasinduDilshara and pcnfernando August 16, 2023 01:05

pcnfernando reviewed Aug 16, 2023

View reviewed changes

LakshanWeerasinghe force-pushed the fix-#40013 branch from 69cfb39 to 30e4309 Compare August 16, 2023 08:43

LakshanWeerasinghe requested a review from pcnfernando August 16, 2023 08:44

LakshanWeerasinghe force-pushed the fix-#40013 branch from 30e4309 to ced7bf7 Compare August 16, 2023 10:28

LakshanWeerasinghe added 16 commits August 16, 2023 22:29

Revert "Revert "Perform silent expr checks in QueryTypeChecker when r…

7e8f6e1

…esolving selectTypes"" This reverts commit 3231685.

Fix type checking for union types in select clause

ad96956

Add tests for select clause with unions

d7ee7c1

Add ambigous error type test

e43fec6

Add type checking for select expr

f8bf731

Add negative tests for query exprs

128592e

Fix type checking for xml subtypes

88901b1

Update failing test

0ce9988

Fix select clause being not type checked

a599f4f

Add negative tests for query exprs

57e8bcc

Address review comments

b816a71

Fix selectClause not type checked for all expctedTypes

0733510

Fix selectClause type checking for non-union types

c130343

Address review comments

1218f18

Improve type-checking for xml and string in select

b017693

Address review comments

6185bb3

LakshanWeerasinghe force-pushed the fix-#40013 branch from ced7bf7 to 6185bb3 Compare August 16, 2023 17:07

pcnfernando approved these changes Aug 17, 2023

View reviewed changes

pcnfernando merged commit a1902ad into ballerina-platform:master Aug 17, 2023
17 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Master] Fix issues when there is union type in the select clause #40835

[Master] Fix issues when there is union type in the select clause #40835

LakshanWeerasinghe commented Jun 26, 2023

SasinduDilshara commented Jul 10, 2023

codecov bot commented Jul 11, 2023 •

edited

Loading

LakshanWeerasinghe commented Jul 11, 2023

MaryamZi Jul 11, 2023

LakshanWeerasinghe Jul 11, 2023

LakshanWeerasinghe Jul 11, 2023

LakshanWeerasinghe Jul 12, 2023

MaryamZi Aug 14, 2023

LakshanWeerasinghe Aug 14, 2023

github-actions bot commented Aug 5, 2023

pcnfernando Aug 10, 2023

LakshanWeerasinghe Aug 11, 2023

MaryamZi Aug 14, 2023

MaryamZi Aug 14, 2023

LakshanWeerasinghe Aug 14, 2023

pcnfernando Aug 15, 2023

LakshanWeerasinghe Aug 15, 2023

LakshanWeerasinghe Aug 15, 2023 •

edited

Loading

pcnfernando Aug 16, 2023

		@@ -322,89 +323,149 @@ public BType resolveQueryType(SymbolEnv env, BLangExpression selectExp, BType ta
		}
		}

		checkExpr(selectExp, env, expType, data);
		selectExp.typeChecked = false;

[Master] Fix issues when there is union type in the select clause #40835

[Master] Fix issues when there is union type in the select clause #40835

Conversation

LakshanWeerasinghe commented Jun 26, 2023

Purpose

Approach

Samples

Remarks

Check List

SasinduDilshara commented Jul 10, 2023

codecov bot commented Jul 11, 2023 • edited Loading

Codecov Report

LakshanWeerasinghe commented Jul 11, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Aug 5, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LakshanWeerasinghe Aug 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jul 11, 2023 •

edited

Loading

LakshanWeerasinghe Aug 15, 2023 •

edited

Loading