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.

Sign up for GitHub

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

Fix defaults from the applicable contextually-expected type not being used when a mapping constructor has readonly fields #33767

Merged
merged 2 commits into from
Nov 12, 2021
Merged

Fix defaults from the applicable contextually-expected type not being used when a mapping constructor has readonly fields #33767

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a5e4dd9
Fix defaults not getting added with readonly mapping constructor fields
MaryamZi Nov 11, 2021
72dc954
Add tests
MaryamZi Nov 11, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 17 additions & 3 deletions ...na-lang/src/main/java/org/wso2/ballerinalang/compiler/semantics/analyzer/TypeChecker.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@
import org.wso2.ballerinalang.compiler.semantics.model.symbols.BPackageSymbol;
import org.wso2.ballerinalang.compiler.semantics.model.symbols.BRecordTypeSymbol;
import org.wso2.ballerinalang.compiler.semantics.model.symbols.BSymbol;
import org.wso2.ballerinalang.compiler.semantics.model.symbols.BTypeSymbol;
import org.wso2.ballerinalang.compiler.semantics.model.symbols.BVarSymbol;
import org.wso2.ballerinalang.compiler.semantics.model.symbols.BXMLNSSymbol;
import org.wso2.ballerinalang.compiler.semantics.model.symbols.SymTag;
Expand Down Expand Up @@ -1737,7 +1738,8 @@ private BType getEffectiveMappingType(BLangRecordLiteral recordLiteral, BType ap
}

PackageID pkgID = env.enclPkg.symbol.pkgID;
BRecordTypeSymbol recordSymbol = createRecordTypeSymbol(pkgID, recordLiteral.pos, VIRTUAL);
Location pos = recordLiteral.pos;
BRecordTypeSymbol recordSymbol = createRecordTypeSymbol(pkgID, pos, VIRTUAL);

LinkedHashMap<String, BField> newFields = new LinkedHashMap<>();

Expand Down Expand Up @@ -1809,12 +1811,24 @@ private BType getEffectiveMappingType(BLangRecordLiteral recordLiteral, BType ap
recordType.tsymbol = recordSymbol;

BLangRecordTypeNode recordTypeNode = TypeDefBuilderHelper.createRecordTypeNode(recordType, pkgID, symTable,
recordLiteral.pos);
pos);
recordTypeNode.initFunction = TypeDefBuilderHelper.createInitFunctionForRecordType(recordTypeNode, env,
names, symTable);
TypeDefBuilderHelper.addTypeDefinition(recordType, recordSymbol, recordTypeNode, env);

if (applicableMappingType.tag == TypeTags.MAP) {
if (applicableMappingType.tag == TypeTags.RECORD) {
BRecordType applicableRecordType = (BRecordType) applicableMappingType;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since it is not possible to call record field default closures individually atm, the fix done for the time-being is adding the original type as an included type to make its init method (that initiates the fields) get called.

However this will cause issues for something like

type Foo record {
    string[] x = ["foo", "bar"];
};

// Panics now with `invalid value for record field 'x': expected value of type 'string[] & readonly', found 'string[]'`,
// since `x` is initialized using the mutable value from `Foo` before the mapping constructor's value.
Foo _ = {readonly x: [""]};

This does not panic in Beta3/Beta4 but will panic with this fix.

Related to #24077 in a way.

type Foo record {
    int[] x = [];
};

Foo & readonly _ = {};

Also panics at runtime atm.

But if we don't fix this, defaults won't be used in cases like

import ballerina/io;

type Foo record {
    int i;
    string s = "default";
};

public function main() {
    Foo f = {readonly i: 1};
    io:println(f); // `{"i":1,"s":null}` and not `{"i":1,"s":"default"}`
}

@hasithaa, thoughts please?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 for now. will @chiranSachintha fix for closures address this? I guess not.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to introduce similar closures for record default values, right? Created #33771 to track fixing it. #32012 also depends on it.

Created #33773 to track fixing this specific case.

BTypeSymbol applicableRecordTypeSymbol = applicableRecordType.tsymbol;
BLangUserDefinedType origTypeRef = new BLangUserDefinedType(
ASTBuilderUtil.createIdentifier(
pos,
TypeDefBuilderHelper.getPackageAlias(env, pos.lineRange().filePath(),
applicableRecordTypeSymbol.pkgID)),
ASTBuilderUtil.createIdentifier(pos, applicableRecordTypeSymbol.name.value));
origTypeRef.pos = pos;
origTypeRef.setBType(applicableRecordType);
recordTypeNode.typeRefs.add(origTypeRef);
} else if (applicableMappingType.tag == TypeTags.MAP) {
recordLiteral.expectedType = applicableMappingType;
}

Expand Down
20 changes: 3 additions & 17 deletions ...allerina-lang/src/main/java/org/wso2/ballerinalang/compiler/util/ImmutableTypeCloner.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,6 @@
import org.wso2.ballerinalang.compiler.semantics.model.types.BXMLSubType;
import org.wso2.ballerinalang.compiler.semantics.model.types.BXMLType;
import org.wso2.ballerinalang.compiler.tree.BLangClassDefinition;
import org.wso2.ballerinalang.compiler.tree.BLangImportPackage;
import org.wso2.ballerinalang.compiler.tree.BLangSimpleVariable;
import org.wso2.ballerinalang.compiler.tree.BLangTypeDefinition;
import org.wso2.ballerinalang.compiler.tree.types.BLangObjectTypeNode;
Expand Down Expand Up @@ -584,28 +583,15 @@ private static void populateImmutableStructureFields(Types types, SymbolTable sy
}

BLangUserDefinedType origTypeRef = new BLangUserDefinedType(
ASTBuilderUtil.createIdentifier(pos, getPackageAlias(env, pos.lineRange().filePath(),
origStructureType.tsymbol.pkgID)),
ASTBuilderUtil.createIdentifier(pos,
TypeDefBuilderHelper.getPackageAlias(env, pos.lineRange().filePath(),
origStructureType.tsymbol.pkgID)),
ASTBuilderUtil.createIdentifier(pos, origStructureType.tsymbol.name.value));
origTypeRef.pos = pos;
origTypeRef.setBType(origStructureType);
immutableStructureTypeNode.typeRefs.add(origTypeRef);
}

private static String getPackageAlias(SymbolEnv env, String compUnitName, PackageID typePkgId) {
for (BLangImportPackage importStmt : env.enclPkg.imports) {
if (!importStmt.compUnit.value.equals(compUnitName)) {
continue;
}

if (importStmt.symbol != null && typePkgId.equals(importStmt.symbol.pkgID)) {
return importStmt.alias.value;
}
}

return ""; // current module
}

private static void setRestType(Types types, SymbolTable symTable, BLangAnonymousModelHelper anonymousModelHelper,
Names names, BRecordType immutableRecordType, BRecordType origRecordType,
Location pos, SymbolEnv env, Set<BType> unresolvedTypes) {
Expand Down
15 changes: 15 additions & 0 deletions ...llerina-lang/src/main/java/org/wso2/ballerinalang/compiler/util/TypeDefBuilderHelper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@
import org.wso2.ballerinalang.compiler.tree.BLangClassDefinition;
import org.wso2.ballerinalang.compiler.tree.BLangFunction;
import org.wso2.ballerinalang.compiler.tree.BLangIdentifier;
import org.wso2.ballerinalang.compiler.tree.BLangImportPackage;
import org.wso2.ballerinalang.compiler.tree.BLangSimpleVariable;
import org.wso2.ballerinalang.compiler.tree.BLangTypeDefinition;
import org.wso2.ballerinalang.compiler.tree.types.BLangBuiltInRefTypeNode;
Expand Down Expand Up @@ -270,4 +271,18 @@ public static BLangErrorType createBLangErrorType(Location pos, BErrorType type,

return errorType;
}

public static String getPackageAlias(SymbolEnv env, String compUnitName, PackageID typePkgId) {
for (BLangImportPackage importStmt : env.enclPkg.imports) {
if (!importStmt.compUnit.value.equals(compUnitName)) {
continue;
}

if (importStmt.symbol != null && typePkgId.equals(importStmt.symbol.pkgID)) {
return importStmt.alias.value;
}
}

return ""; // current module
}
}
52 changes: 52 additions & 0 deletions ...test/java/org/ballerinalang/test/bala/record/ReadOnlyMappingConstructorFieldBalaTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
/*
* Copyright (c) 2021, WSO2 Inc. (http://www.wso2.org) All Rights Reserved.
*
* WSO2 Inc. 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.bala.record;

import org.ballerinalang.test.BCompileUtil;
import org.ballerinalang.test.BRunUtil;
import org.ballerinalang.test.CompileResult;
import org.testng.annotations.AfterClass;
import org.testng.annotations.BeforeClass;
import org.testng.annotations.Test;

/**
* Bala test for readonly fields in mapping constructor expressions.
*
* @since 2.0.0
*/
public class ReadOnlyMappingConstructorFieldBalaTest {

private CompileResult result;

@BeforeClass
public void setup() {
BCompileUtil.compileAndCacheBala("test-src/bala/test_projects/test_project");
result = BCompileUtil.compile("test-src/record/mapping_constructor_with_readonly_fields_in_bala.bal");
}

@Test
public void testDefaultValueFromCETBeingUsedWithReadOnlyFieldsInTheMappingConstructor() {
BRunUtil.invoke(result, "testDefaultValueFromCETBeingUsedWithReadOnlyFieldsInTheMappingConstructor");
}

@AfterClass
public void tearDown() {
result = null;
}
}
3 changes: 2 additions & 1 deletion ...lerina-unit-test/src/test/java/org/ballerinalang/test/record/ReadonlyRecordFieldTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,8 @@ public Object[][] readonlyRecordFieldTestFunctions() {
{"testReadOnlyFieldsOfClassTypes"},
{"testTypeReadOnlynessNegativeWithNonReadOnlyFieldsViaInclusion"},
{"testTypeReadOnlynessWithReadOnlyFieldsViaInclusion"},
{"testRecordWithFunctionTypeField"}
{"testRecordWithFunctionTypeField"},
{"testDefaultValueFromCETBeingUsedWithReadOnlyFieldsInTheMappingConstructor"}
};
}

Expand Down
21 changes: 21 additions & 0 deletions ...t/src/test/resources/test-src/bala/test_projects/test_project/modules/records/records.bal
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
// Copyright (c) 2021 WSO2 Inc. (http://www.wso2.org) All Rights Reserved.
//
// WSO2 Inc. 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.

public type Corge record {|
string a = "hello";
string b = "world";
string[] c = <readonly> ["x", "y"];
|};
47 changes: 47 additions & 0 deletions ...t/src/test/resources/test-src/record/mapping_constructor_with_readonly_fields_in_bala.bal
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
// Copyright (c) 2021 WSO2 Inc. (http://www.wso2.org) All Rights Reserved.
//
// WSO2 Inc. 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.

import testorg/foo.records;

function testDefaultValueFromCETBeingUsedWithReadOnlyFieldsInTheMappingConstructor() {
records:Corge d = {readonly b: "ballerina"};
string[] & readonly e = ["x", "y"];
assertEquality({a: "hello", b: "ballerina", c: e}, d);
assertTrue(d is record {|string a; readonly string b; string[] c;|});
assertFalse(d is record {|string a; readonly string b; readonly string[] c;|});

records:Corge f = {readonly b: "val", readonly c: ["a", "b", "c"]};
assertEquality({a: "hello", b: "val", c: <readonly> ["a", "b", "c"]}, f);
assertTrue(f is record {|string a; readonly string b; string[] c;|});
assertTrue(f is record {|string a; readonly string b; readonly string[] c;|});
assertFalse(f is record {|readonly string a; readonly string b; readonly string[] c;|});
}

function assertTrue(anydata actual) {
assertEquality(true, actual);
}

function assertFalse(anydata actual) {
assertEquality(false, actual);
}

function assertEquality(anydata expected, anydata actual) {
if expected == actual {
return;
}

panic error("expected '" + expected.toString() + "', found '" + actual.toString() + "'");
}
35 changes: 35 additions & 0 deletions tests/jballerina-unit-test/src/test/resources/test-src/record/readonly_record_fields.bal
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -883,6 +883,41 @@ function getRecord(boolean b) returns IncludingRec1|IncludingRec2? {
return {k: 1, l: 2};
}

type Corge record {|
string a = "hello";
string b = "world";
string[] c = <readonly> ["x", "y"];
|};

function testDefaultValueFromCETBeingUsedWithReadOnlyFieldsInTheMappingConstructor() {
record {int a; string b = "default";} a = {readonly a: 2};
assertEquality(<anydata> {a: 2, b: "default"}, a);
assertTrue(a is record {readonly int a; string b;});
assertFalse(a is record {readonly int a; readonly string b;});

record {int a; string b = "default";} b = {readonly a: 3, b: "val"};
assertEquality(<anydata> {a: 3, b: "val"}, b);
assertTrue(b is record {readonly int a; string b;});
assertFalse(b is record {readonly int a; readonly string b;});

record {int a; string b = "default";} c = {readonly a: 3, readonly b: "val"};
assertEquality(<anydata> {a: 3, b: "val"}, c);
assertTrue(c is record {readonly int a; string b;});
assertTrue(c is record {readonly int a; readonly string b;});

Corge d = {readonly b: "ballerina"};
string[] & readonly e = ["x", "y"];
assertEquality(<anydata> {a: "hello", b: "ballerina", c: e}, d);
assertTrue(d is record {|string a; readonly string b; string[] c;|});
assertFalse(d is record {|string a; readonly string b; readonly string[] c;|});

Corge f = {readonly b: "val", readonly c: ["a", "b", "c"]};
assertEquality(<anydata> {a: "hello", b: "val", c: <readonly> ["a", "b", "c"]}, f);
assertTrue(f is record {|string a; readonly string b; string[] c;|});
assertTrue(f is record {|string a; readonly string b; readonly string[] c;|});
assertFalse(f is record {|readonly string a; readonly string b; readonly string[] c;|});
}

const ASSERTION_ERROR_REASON = "AssertionError";

function assertTrue(any|error actual) {
Expand Down