Fix compiler crash when accessing previous parameter in default expression with lambda expression #41816

chiranSachintha · 2023-12-03T09:23:28Z

Purpose

$title.

Fixes #41802

Check List

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

...lerina-unit-test/src/test/resources/test-src/functions/functions_with_default_parameters.bal

…ina-lang into fix-41802 # Conflicts: # compiler/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/desugar/ClosureGenerator.java

codecov · 2023-12-21T10:08:28Z

Codecov Report

Attention: Patch coverage is 95.00000% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 76.56%. Comparing base (48a9e69) to head (3824057).
Report is 17 commits behind head on master.

Files	Patch %	Lines
...llerinalang/compiler/desugar/ClosureGenerator.java	90.90%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@              Coverage Diff              @@
##             master   #41816       +/-   ##
=============================================
+ Coverage      0.00%   76.56%   +76.56%     
- Complexity        0    53525    +53525     
=============================================
  Files             9     2905     +2896     
  Lines            35   202267   +202232     
  Branches          0    26336    +26336     
=============================================
+ Hits              0   154858   +154858     
- Misses           35    38924    +38889     
- Partials          0     8485     +8485

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

MaryamZi · 2023-12-22T06:08:23Z

.../ballerina-lang/src/main/java/io/ballerina/compiler/api/impl/symbols/AbstractTypeSymbol.java

+            if (function instanceof BallerinaFunctionSymbol &&
+                    ((BallerinaFunctionSymbol) function).getInternalSymbol().getOrigin() == SymbolOrigin.VIRTUAL) {
+                continue;
+            }


cc @dulajdilshan

How come we didn't come across this previously?

Ping on this @chiranSachintha @dulajdilshan

Previously we haven't got any values such as this here to do the filtering. Let's better check why this is happening with the new changes.

However, it would be better to do the filtering in https://github.com/ballerina-platform/ballerina-lang/blob/master/compiler/ballerina-lang/src/main/java/io/ballerina/compiler/api/impl/LangLibrary.java

The issue arises when using a parameter as the default value for another parameter without creating a new symbol for it. That is not correct. With this I have fixed that, and now we create a new symbol for that. So we need to skip those function as they are created in the desugar level.

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

Line 606 in 4d53eb7

BVarSymbol varSymbol = new BVarSymbol(Flags.REQUIRED_PARAM, symbolName, symbol.pkgID, type, funcSymbol, pos,

@dulajdilshan We need to skip them within the filterLangLibMethods. Isn't it?

How about we filter them in here:

ballerina-lang/compiler/ballerina-lang/src/main/java/io/ballerina/compiler/api/impl/LangLibrary.java

Line 179 in 9215217

private static void addLangLibMethods(String basicType, BPackageSymbol langLibModule,

...lerina-unit-test/src/test/resources/test-src/functions/functions_with_default_parameters.bal

compiler/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/util/CompilerUtils.java

MaryamZi · 2023-12-22T06:55:11Z

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

-                    varSymbol.owner == resolvedSymbolEnv.scope.owner) {
+            if (entry != NOT_FOUND_ENTRY && varSymbol == entry.symbol) {


Do we need this because we don't shadow the symbol like we discussed offline?

Yes, when looking for the resolved level of a symbol, we don't need to check for the symbol owner. If there's the same symbol with two different owners, it becomes a compile-time error. I believe we introduced this logic before addressing the shadowing issue(that is the reason to add this check).

The reason for keeping the varSymbol == entry.symbol logic is because there are instances in the desugar where we use the same symbol generate variable. However, this is also not correct, and we need to address it separately.

Have we created an issue for this?

github-actions · 2024-01-06T19:33:08Z

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.

github-actions · 2024-01-23T19:33:07Z

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.

github-actions · 2024-01-27T19:34:13Z

Closed PR due to inactivity for more than 18 days.

github-actions · 2024-03-08T19:33:26Z

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.

…ina-lang into fix-41802

MaryamZi · 2024-03-20T04:41:31Z

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

-                    varSymbol.owner == resolvedSymbolEnv.scope.owner) {
+            if (entry != NOT_FOUND_ENTRY && varSymbol == entry.symbol) {


Have we created an issue for this?

MaryamZi · 2024-03-20T04:42:59Z

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

-            varSymbol.flags = 0;
-            funcSymbol.scope.define(varSymbol.name, varSymbol);
+            BType type = symbol.type;
+            Location pos = funcSymbol.pos;


This is fixed, so extract to a variable outside the loop?

Only the position right? Type is extract from the parameter

...lerina-unit-test/src/test/resources/test-src/functions/functions_with_default_parameters.bal

MaryamZi · 2024-03-21T05:40:52Z

...lerina-unit-test/src/test/resources/test-src/functions/functions_with_default_parameters.bal

+    return a + b();
+}
+
+function testParamUseAsValueInAnonFuncWithDefaultForNextParam() {


Suggested change

function testParamUseAsValueInAnonFuncWithDefaultForNextParam() {

function testUsingParamInAnonFuncDefaultValueOfSubsequentParam() {

MaryamZi · 2024-03-21T05:41:52Z

...lerina-unit-test/src/test/resources/test-src/functions/functions_with_default_parameters.bal

+                                                                        }) returns int {
+    return a + b();
+}
+


Add at least one test

where it's not the next immediate param that accesses a previous param like this

with an expression-bodied function as the default value

Add tests with ce41d41

…ina-lang into fix-41802

CLAassistant · 2024-03-29T05:22:43Z

All committers have signed the CLA.

MaryamZi · 2024-03-29T08:02:20Z

@chiranSachintha, please check the level 6 build failure.

...lerina-unit-test/src/test/resources/test-src/functions/functions_with_default_parameters.bal

chiranSachintha added 4 commits December 3, 2023 13:56

Resolve closure variable level using its symbol

238f6d3

Exclude marking function parameters as closure variables

dbda070

Use the same symbol of parameter for closure generation

3a8aa82

Add unit tests

f6ab63d

chiranSachintha requested review from sameerajayasoma and hasithaa as code owners December 3, 2023 09:23

Update owner of the symbol

3b0ae6d

chiranSachintha requested review from MaryamZi, heshanpadmasiri and dulajdilshan December 3, 2023 16:39

MaryamZi reviewed Dec 4, 2023

View reviewed changes

heshanpadmasiri reviewed Dec 5, 2023

View reviewed changes

...lerina-unit-test/src/test/resources/test-src/functions/functions_with_default_parameters.bal Show resolved Hide resolved

MaryamZi added the Team/CompilerFE All issues related to Language implementation and Compiler, this exclude run times. label Dec 8, 2023

chiranSachintha added 3 commits December 12, 2023 08:58

Generate new symbols for closure parameters

6d13cbc

Add unit tests

c2bd648

Merge branch 'master' of https://github.com/ballerina-platform/baller…

2d79df8

…ina-lang into fix-41802 # Conflicts: # compiler/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/desugar/ClosureGenerator.java

chiranSachintha requested review from MaryamZi and heshanpadmasiri December 12, 2023 09:42

Skip desugared methods

ca72df0

MaryamZi reviewed Dec 22, 2023

View reviewed changes

github-actions bot added the Stale label Jan 6, 2024

chiranSachintha removed the Stale label Jan 8, 2024

github-actions bot added the Stale label Jan 23, 2024

github-actions bot closed this Jan 27, 2024

chiranSachintha removed the Stale label Jan 30, 2024

Fix review suggestions

d9604bb

github-actions bot added the Stale label Mar 8, 2024

chiranSachintha removed the Stale label Mar 11, 2024

Merge branch 'master' of https://github.com/ballerina-platform/baller…

4d53eb7

…ina-lang into fix-41802

chiranSachintha dismissed heshanpadmasiri’s stale review via 457647e March 18, 2024 10:43

chiranSachintha force-pushed the fix-41802 branch from 457647e to 4d53eb7 Compare March 19, 2024 05:35

Fix review suggestions

834b86d

dulajdilshan previously approved these changes Mar 19, 2024

View reviewed changes

MaryamZi reviewed Mar 20, 2024

View reviewed changes

chiranSachintha dismissed dulajdilshan’s stale review via 00d515d March 20, 2024 11:39

Add unit tests

b863355

chiranSachintha force-pushed the fix-41802 branch from 00d515d to b863355 Compare March 20, 2024 11:40

Fix review suggestions

ba86c4e

chiranSachintha added this to the 2201.9.0 milestone Mar 21, 2024

MaryamZi reviewed Mar 21, 2024

View reviewed changes

chiranSachintha added 2 commits March 29, 2024 10:35

Merge branch 'master' of https://github.com/ballerina-platform/baller…

d925766

…ina-lang into fix-41802

Fix review suggestions

ce41d41

chiranSachintha requested review from MaryamZi, dulajdilshan and heshanpadmasiri March 29, 2024 06:14

MaryamZi previously approved these changes Mar 29, 2024

View reviewed changes

poorna2152 reviewed Apr 1, 2024

View reviewed changes

...lerina-unit-test/src/test/resources/test-src/functions/functions_with_default_parameters.bal Outdated Show resolved Hide resolved

Fix review suggestions

3824057

chiranSachintha dismissed MaryamZi’s stale review via 3824057 April 2, 2024 04:52

MaryamZi approved these changes Apr 2, 2024

View reviewed changes

MaryamZi requested a review from poorna2152 April 2, 2024 08:47

poorna2152 approved these changes Apr 2, 2024

View reviewed changes

MaryamZi merged commit 5e01f3c into ballerina-platform:master Apr 2, 2024
18 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix compiler crash when accessing previous parameter in default expression with lambda expression #41816

Fix compiler crash when accessing previous parameter in default expression with lambda expression #41816

chiranSachintha commented Dec 3, 2023

codecov bot commented Dec 21, 2023 •

edited

Loading

MaryamZi Dec 22, 2023

MaryamZi Dec 22, 2023

MaryamZi Mar 15, 2024

dulajdilshan Mar 15, 2024

chiranSachintha Mar 19, 2024

dulajdilshan Mar 19, 2024

chiranSachintha Mar 19, 2024

MaryamZi Dec 22, 2023

chiranSachintha Mar 15, 2024

MaryamZi Mar 20, 2024

chiranSachintha Mar 29, 2024

github-actions bot commented Jan 6, 2024

github-actions bot commented Jan 23, 2024

github-actions bot commented Jan 27, 2024

github-actions bot commented Mar 8, 2024

MaryamZi Mar 20, 2024

MaryamZi Mar 20, 2024

chiranSachintha Mar 20, 2024

MaryamZi Mar 21, 2024

MaryamZi Mar 21, 2024

MaryamZi Mar 21, 2024

chiranSachintha Mar 29, 2024

CLAassistant commented Mar 29, 2024 •

edited

Loading

MaryamZi commented Mar 29, 2024

		varSymbol.owner == resolvedSymbolEnv.scope.owner) {
		if (entry != NOT_FOUND_ENTRY && varSymbol == entry.symbol) {

	function testParamUseAsValueInAnonFuncWithDefaultForNextParam() {
	function testUsingParamInAnonFuncDefaultValueOfSubsequentParam() {

Fix compiler crash when accessing previous parameter in default expression with lambda expression #41816

Fix compiler crash when accessing previous parameter in default expression with lambda expression #41816

Conversation

chiranSachintha commented Dec 3, 2023

Purpose

Check List

codecov bot commented Dec 21, 2023 • edited Loading

Codecov Report

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

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 Jan 6, 2024

github-actions bot commented Jan 23, 2024

github-actions bot commented Jan 27, 2024

github-actions bot commented Mar 8, 2024

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

CLAassistant commented Mar 29, 2024 • edited Loading

MaryamZi commented Mar 29, 2024

codecov bot commented Dec 21, 2023 •

edited

Loading

CLAassistant commented Mar 29, 2024 •

edited

Loading