From 6c5b25cfa0cb8a1a78180044b09220e9f15780d5 Mon Sep 17 00:00:00 2001 From: MaryamZi Date: Thu, 1 Oct 2020 22:09:16 +0530 Subject: [PATCH 01/19] Add the isolated flag for isolated objects --- .../compiler/parser/BLangNodeTransformer.java | 51 ++++++++++++------- .../semantics/analyzer/SymbolEnter.java | 13 ++++- .../semantics/analyzer/SymbolResolver.java | 12 +++-- 3 files changed, 54 insertions(+), 22 deletions(-) diff --git a/compiler/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/parser/BLangNodeTransformer.java b/compiler/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/parser/BLangNodeTransformer.java index e862a20e8c06..70875e0f8f99 100644 --- a/compiler/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/parser/BLangNodeTransformer.java +++ b/compiler/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/parser/BLangNodeTransformer.java @@ -422,6 +422,7 @@ import java.util.Stack; import java.util.regex.Matcher; +import static org.ballerinalang.model.elements.Flag.ISOLATED; import static org.ballerinalang.model.elements.Flag.SERVICE; import static org.wso2.ballerinalang.compiler.util.Constants.INFERRED_ARRAY_INDICATOR; import static org.wso2.ballerinalang.compiler.util.Constants.OPEN_ARRAY_INDICATOR; @@ -861,13 +862,23 @@ public BLangNode transform(ObjectTypeDescriptorNode objTypeDescNode) { BLangObjectTypeNode objectTypeNode = (BLangObjectTypeNode) TreeBuilder.createObjectTypeNode(); for (Token qualifier : objTypeDescNode.objectTypeQualifiers()) { - if (qualifier.kind() == SyntaxKind.CLIENT_KEYWORD) { + SyntaxKind kind = qualifier.kind(); + if (kind == SyntaxKind.CLIENT_KEYWORD) { objectTypeNode.flagSet.add(Flag.CLIENT); - } else if (qualifier.kind() == SyntaxKind.SERVICE_KEYWORD) { + continue; + } + + if (kind == SyntaxKind.SERVICE_KEYWORD) { objectTypeNode.flagSet.add(SERVICE); - } else { - throw new RuntimeException("Syntax kind is not supported: " + qualifier.kind()); + continue; + } + + if (kind == SyntaxKind.ISOLATED_KEYWORD) { + objectTypeNode.flagSet.add(ISOLATED); + continue; } + + throw new RuntimeException("Syntax kind is not supported: " + kind); } NodeList members = objTypeDescNode.members(); @@ -3616,20 +3627,26 @@ public BLangNode transform(ClassDefinitionNode classDefinitionNode) { }); for (Token qualifier : classDefinitionNode.classTypeQualifiers()) { - if (qualifier.kind() == SyntaxKind.DISTINCT_KEYWORD) { - blangClass.flagSet.add(Flag.DISTINCT); - } + SyntaxKind kind = qualifier.kind(); - if (qualifier.kind() == SyntaxKind.CLIENT_KEYWORD) { - blangClass.flagSet.add(Flag.CLIENT); - } - - if (qualifier.kind() == SyntaxKind.READONLY_KEYWORD) { - blangClass.flagSet.add(Flag.READONLY); - } - - if (qualifier.kind() == SyntaxKind.SERVICE_KEYWORD) { - blangClass.flagSet.add(SERVICE); + switch (kind) { + case DISTINCT_KEYWORD: + blangClass.flagSet.add(Flag.DISTINCT); + break; + case CLIENT_KEYWORD: + blangClass.flagSet.add(Flag.CLIENT); + break; + case READONLY_KEYWORD: + blangClass.flagSet.add(Flag.READONLY); + break; + case SERVICE_KEYWORD: + blangClass.flagSet.add(Flag.SERVICE); + break; + case ISOLATED_KEYWORD: + blangClass.flagSet.add(Flag.ISOLATED); + break; + default: + throw new RuntimeException("Syntax kind is not supported: " + kind); } } diff --git a/compiler/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/semantics/analyzer/SymbolEnter.java b/compiler/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/semantics/analyzer/SymbolEnter.java index dff1a53f695f..e87f8cc69cb0 100644 --- a/compiler/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/semantics/analyzer/SymbolEnter.java +++ b/compiler/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/semantics/analyzer/SymbolEnter.java @@ -593,7 +593,6 @@ private List getClassDefinitions(List topLev @Override public void visit(BLangClassDefinition classDefinition) { EnumSet flags = EnumSet.copyOf(classDefinition.flagSet); - boolean isReadOnly = flags.contains(Flag.READONLY); boolean isPublicType = flags.contains(Flag.PUBLIC); BTypeSymbol tSymbol = Symbols.createClassSymbol(Flags.asMask(flags), @@ -608,7 +607,17 @@ public void visit(BLangClassDefinition classDefinition) { if (flags.contains(Flag.SERVICE)) { objectType = new BServiceType(tSymbol); } else { - objectType = isReadOnly ? new BObjectType(tSymbol, Flags.READONLY) : new BObjectType(tSymbol); + int typeFlags = 0; + + if (flags.contains(Flag.READONLY)) { + typeFlags |= Flags.READONLY; + } + + if (flags.contains(Flag.ISOLATED)) { + typeFlags |= Flags.ISOLATED; + } + + objectType = new BObjectType(tSymbol, typeFlags); } if (flags.contains(Flag.DISTINCT)) { diff --git a/compiler/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/semantics/analyzer/SymbolResolver.java b/compiler/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/semantics/analyzer/SymbolResolver.java index 83f9649a8ee4..0cdad8eb2397 100644 --- a/compiler/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/semantics/analyzer/SymbolResolver.java +++ b/compiler/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/semantics/analyzer/SymbolResolver.java @@ -1011,9 +1011,15 @@ public void visit(BLangObjectTypeNode objectTypeNode) { flags.add(Flag.PUBLIC); } - boolean isReadOnly = objectTypeNode.flagSet.contains(Flag.READONLY); - if (isReadOnly) { + int typeFlags = 0; + if (objectTypeNode.flagSet.contains(Flag.READONLY)) { flags.add(Flag.READONLY); + typeFlags |= Flags.READONLY; + } + + if (objectTypeNode.flagSet.contains(Flag.ISOLATED)) { + flags.add(Flag.ISOLATED); + typeFlags |= Flags.ISOLATED; } BTypeSymbol objectSymbol = Symbols.createObjectSymbol(Flags.asMask(flags), Names.EMPTY, @@ -1022,7 +1028,7 @@ public void visit(BLangObjectTypeNode objectTypeNode) { if (flags.contains(Flag.SERVICE)) { objectType = new BServiceType(objectSymbol); } else { - objectType = isReadOnly ? new BObjectType(objectSymbol, Flags.READONLY) : new BObjectType(objectSymbol); + objectType = new BObjectType(objectSymbol, typeFlags); } objectSymbol.type = objectType; objectTypeNode.symbol = objectSymbol; From b42b70f7e66c79c88a02961fdf4ee148cab2685b Mon Sep 17 00:00:00 2001 From: MaryamZi Date: Fri, 2 Oct 2020 23:48:49 +0530 Subject: [PATCH 02/19] Add mutable field validation for isolated objects --- .../util/diagnostic/DiagnosticCode.java | 4 + .../semantics/analyzer/IsolationAnalyzer.java | 79 ++++++++++++++++++- .../src/main/resources/compiler.properties | 6 ++ 3 files changed, 88 insertions(+), 1 deletion(-) diff --git a/compiler/ballerina-lang/src/main/java/org/ballerinalang/util/diagnostic/DiagnosticCode.java b/compiler/ballerina-lang/src/main/java/org/ballerinalang/util/diagnostic/DiagnosticCode.java index 266704c3a0ff..b48d0a4bf994 100644 --- a/compiler/ballerina-lang/src/main/java/org/ballerinalang/util/diagnostic/DiagnosticCode.java +++ b/compiler/ballerina-lang/src/main/java/org/ballerinalang/util/diagnostic/DiagnosticCode.java @@ -598,6 +598,10 @@ public enum DiagnosticCode { INVALID_WORKER_DECLARATION_IN_ISOLATED_FUNCTION("invalid.worker.declaration.in.isolated.function"), FUNCTION_CAN_BE_MARKED_ISOLATED("function.can.be.marked.isolated"), + INVALID_NON_PRIVATE_MUTABLE_FIELD_IN_ISOLATED_OBJECT("invalid.non.private.mutable.field.in.isolated.object"), + INVALID_MUTABLE_FIELD_ACCESS_IN_ISOLATED_OBJECT_OUTSIDE_LOCK( + "invalid.mutable.field.access.in.isolated.object.outside.lock"), + COMPILER_PLUGIN_ERROR("compiler.plugin.crashed"), ; private String value; diff --git a/compiler/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/semantics/analyzer/IsolationAnalyzer.java b/compiler/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/semantics/analyzer/IsolationAnalyzer.java index fb9d8dac1ff9..9cd8a487de4e 100644 --- a/compiler/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/semantics/analyzer/IsolationAnalyzer.java +++ b/compiler/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/semantics/analyzer/IsolationAnalyzer.java @@ -20,6 +20,7 @@ import org.ballerinalang.compiler.CompilerPhase; import org.ballerinalang.model.clauses.OrderKeyNode; +import org.ballerinalang.model.elements.Flag; import org.ballerinalang.model.symbols.SymbolKind; import org.ballerinalang.model.tree.NodeKind; import org.ballerinalang.model.tree.expressions.RecordLiteralNode; @@ -28,8 +29,11 @@ import org.wso2.ballerinalang.compiler.semantics.model.SymbolEnv; import org.wso2.ballerinalang.compiler.semantics.model.SymbolTable; import org.wso2.ballerinalang.compiler.semantics.model.symbols.BSymbol; +import org.wso2.ballerinalang.compiler.semantics.model.symbols.BVarSymbol; import org.wso2.ballerinalang.compiler.semantics.model.symbols.SymTag; import org.wso2.ballerinalang.compiler.semantics.model.symbols.Symbols; +import org.wso2.ballerinalang.compiler.semantics.model.types.BField; +import org.wso2.ballerinalang.compiler.semantics.model.types.BObjectType; import org.wso2.ballerinalang.compiler.semantics.model.types.BType; import org.wso2.ballerinalang.compiler.tree.BLangAnnotation; import org.wso2.ballerinalang.compiler.tree.BLangAnnotationAttachment; @@ -182,6 +186,8 @@ import org.wso2.ballerinalang.compiler.tree.types.BLangUserDefinedType; import org.wso2.ballerinalang.compiler.tree.types.BLangValueType; import org.wso2.ballerinalang.compiler.util.CompilerContext; +import org.wso2.ballerinalang.compiler.util.Names; +import org.wso2.ballerinalang.compiler.util.TypeTags; import org.wso2.ballerinalang.util.Flags; import java.util.ArrayList; @@ -201,6 +207,7 @@ public class IsolationAnalyzer extends BLangNodeVisitor { private BLangDiagnosticLog dlog; private boolean inferredIsolated = true; + private boolean inLockStatement = false; private IsolationAnalyzer(CompilerContext context) { context.put(ISOLATION_ANALYZER_KEY, this); @@ -332,6 +339,13 @@ public void visit(BLangSimpleVariable varNode) { analyzeNode(typeNode, env); } + int flags = varNode.symbol.flags; + + if (isIsolatedObjectField() && isMutableField(varNode.symbol, varNode.type) && + !Symbols.isFlagOn(flags, Flags.PRIVATE)) { + dlog.error(varNode.pos, DiagnosticCode.INVALID_NON_PRIVATE_MUTABLE_FIELD_IN_ISOLATED_OBJECT); + } + BLangExpression expr = varNode.expr; if (expr == null) { return; @@ -339,7 +353,7 @@ public void visit(BLangSimpleVariable varNode) { analyzeNode(expr, env); - if (Symbols.isFlagOn(varNode.symbol.flags, Flags.WORKER)) { + if (Symbols.isFlagOn(flags, Flags.WORKER)) { inferredIsolated = false; if (isInIsolatedFunction(env.enclInvokable)) { @@ -608,7 +622,12 @@ public void visit(BLangWhile whileNode) { @Override public void visit(BLangLock lockNode) { + boolean prevInLockStatement = this.inLockStatement; + this.inLockStatement = true; + analyzeNode(lockNode.body, env); + + this.inLockStatement = prevInLockStatement; } @Override @@ -801,6 +820,11 @@ public void visit(BLangSimpleVarRef varRefExpr) { @Override public void visit(BLangFieldBasedAccess fieldAccessExpr) { analyzeNode(fieldAccessExpr.expr, env); + + if (!inLockStatement && isIsolatedObjectMutableFieldAccessViaSelf(fieldAccessExpr)) { + dlog.error(fieldAccessExpr.pos, + DiagnosticCode.INVALID_MUTABLE_FIELD_ACCESS_IN_ISOLATED_OBJECT_OUTSIDE_LOCK); + } } @Override @@ -1432,4 +1456,57 @@ private boolean isDefinitionReference(BSymbol symbol) { private boolean isIsolated(BSymbol symbol) { return Symbols.isFlagOn(symbol.flags, Flags.ISOLATED); } + + private boolean isIsolatedObjectField() { + BLangNode node = env.node; + NodeKind kind = node.getKind(); + + if (kind == NodeKind.OBJECT_TYPE) { + return ((BLangObjectTypeNode) node).flagSet.contains(Flag.ISOLATED); + } + + if (kind == NodeKind.CLASS_DEFN) { + return ((BLangClassDefinition) node).flagSet.contains(Flag.ISOLATED); + } + + return false; + } + + private boolean isMutableField(BVarSymbol symbol, BType type) { + return !Symbols.isFlagOn(symbol.flags, Flags.FINAL) || !Symbols.isFlagOn(type.flags, Flags.READONLY); + } + + private boolean isIsolatedObjectMutableFieldAccessViaSelf(BLangFieldBasedAccess fieldAccessExpr) { + BLangExpression expr = fieldAccessExpr.expr; + + if (expr.getKind() != NodeKind.SIMPLE_VARIABLE_REF) { + return false; + } + + if (!Names.SELF.value.equals(((BLangSimpleVarRef) expr).variableName.value)) { + return false; + } + + BLangInvokableNode enclInvokable = env.enclInvokable; + + if (enclInvokable == null || enclInvokable.getKind() != NodeKind.FUNCTION) { + return false; + } + + BLangFunction enclFunction = (BLangFunction) enclInvokable; + + if (enclFunction.objInitFunction || !enclFunction.attachedFunction) { + return false; + } + + BType ownerType = enclInvokable.symbol.owner.type; + + if (ownerType.tag != TypeTags.OBJECT || !Symbols.isFlagOn(ownerType.flags, Flags.ISOLATED)) { + return false; + } + + BField field = ((BObjectType) ownerType).fields.get(fieldAccessExpr.field.value); + + return isMutableField(field.symbol, field.type); + } } diff --git a/compiler/ballerina-lang/src/main/resources/compiler.properties b/compiler/ballerina-lang/src/main/resources/compiler.properties index c54bec5f7215..769fcddd0c8d 100644 --- a/compiler/ballerina-lang/src/main/resources/compiler.properties +++ b/compiler/ballerina-lang/src/main/resources/compiler.properties @@ -1507,3 +1507,9 @@ error.invalid.worker.declaration.in.isolated.function=\ warning.function.can.be.marked.isolated=\ function ''{0}'' can be marked as an ''isolated'' function + +error.invalid.non.private.mutable.field.in.isolated.object=\ + invalid non-private mutable field in an ''isolated'' object + +error.invalid.mutable.field.access.in.isolated.object.outside.lock=\ + invalid access of a mutable field of an ''isolated'' object outside a ''lock'' statement From f460bb69446702c3b75daabb33f87bad55277d98 Mon Sep 17 00:00:00 2001 From: MaryamZi Date: Mon, 5 Oct 2020 19:31:10 +0530 Subject: [PATCH 03/19] Add initial value checks for isolated objects --- .../util/diagnostic/DiagnosticCode.java | 2 + .../semantics/analyzer/IsolationAnalyzer.java | 205 ++++++++++++++++-- .../src/main/resources/compiler.properties | 3 + 3 files changed, 191 insertions(+), 19 deletions(-) diff --git a/compiler/ballerina-lang/src/main/java/org/ballerinalang/util/diagnostic/DiagnosticCode.java b/compiler/ballerina-lang/src/main/java/org/ballerinalang/util/diagnostic/DiagnosticCode.java index b48d0a4bf994..32fbc32a0ca0 100644 --- a/compiler/ballerina-lang/src/main/java/org/ballerinalang/util/diagnostic/DiagnosticCode.java +++ b/compiler/ballerina-lang/src/main/java/org/ballerinalang/util/diagnostic/DiagnosticCode.java @@ -601,6 +601,8 @@ public enum DiagnosticCode { INVALID_NON_PRIVATE_MUTABLE_FIELD_IN_ISOLATED_OBJECT("invalid.non.private.mutable.field.in.isolated.object"), INVALID_MUTABLE_FIELD_ACCESS_IN_ISOLATED_OBJECT_OUTSIDE_LOCK( "invalid.mutable.field.access.in.isolated.object.outside.lock"), + INVALID_NON_UNIQUE_EXPRESSION_AS_INITIAL_VALUE_IN_ISOLATED_OBJECT( + "invalid.non.unique.expression.as.initial.value.in.isolated.object"), COMPILER_PLUGIN_ERROR("compiler.plugin.crashed"), ; diff --git a/compiler/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/semantics/analyzer/IsolationAnalyzer.java b/compiler/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/semantics/analyzer/IsolationAnalyzer.java index 9cd8a487de4e..19d63791357f 100644 --- a/compiler/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/semantics/analyzer/IsolationAnalyzer.java +++ b/compiler/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/semantics/analyzer/IsolationAnalyzer.java @@ -35,6 +35,7 @@ import org.wso2.ballerinalang.compiler.semantics.model.types.BField; import org.wso2.ballerinalang.compiler.semantics.model.types.BObjectType; import org.wso2.ballerinalang.compiler.semantics.model.types.BType; +import org.wso2.ballerinalang.compiler.semantics.model.types.BUnionType; import org.wso2.ballerinalang.compiler.tree.BLangAnnotation; import org.wso2.ballerinalang.compiler.tree.BLangAnnotationAttachment; import org.wso2.ballerinalang.compiler.tree.BLangBlockFunctionBody; @@ -191,7 +192,9 @@ import org.wso2.ballerinalang.util.Flags; import java.util.ArrayList; +import java.util.HashMap; import java.util.List; +import java.util.Map; /** * Responsible for performing isolation analysis. @@ -201,6 +204,10 @@ public class IsolationAnalyzer extends BLangNodeVisitor { private static final CompilerContext.Key ISOLATION_ANALYZER_KEY = new CompilerContext.Key<>(); + private static final String VALUE_LANG_LIB = "lang.value"; + private static final String CLONE_LANG_LIB_METHOD = "clone"; + private static final String CLONE_READONLY_LANG_LIB_METHOD = "cloneReadOnly"; + private SymbolEnv env; private SymbolTable symTable; private Types types; @@ -208,6 +215,7 @@ public class IsolationAnalyzer extends BLangNodeVisitor { private boolean inferredIsolated = true; private boolean inLockStatement = false; + private Map> references = new HashMap<>(); private IsolationAnalyzer(CompilerContext context) { context.put(ISOLATION_ANALYZER_KEY, this); @@ -262,6 +270,17 @@ public void visit(BLangPackage pkgNode) { } pkgNode.completedPhases.add(CompilerPhase.ISOLATION_ANALYZE); + + for (List referenceList : references.values()) { + if (referenceList.size() == 1) { + continue; + } + + for (BLangSimpleVarRef varRef : referenceList) { + dlog.error(varRef.pos, + DiagnosticCode.INVALID_NON_UNIQUE_EXPRESSION_AS_INITIAL_VALUE_IN_ISOLATED_OBJECT); + } + } } @Override @@ -283,7 +302,7 @@ public void visit(BLangFunction funcNode) { SymbolEnv funcEnv = SymbolEnv.createFunctionEnv(funcNode, funcNode.symbol.scope, env); analyzeNode(funcNode.body, funcEnv); - if (isBallerinaModule(env.enclPkg) && !isIsolated(funcNode.symbol) && + if (isBallerinaModule(env.enclPkg) && !isIsolated(funcNode.symbol.flags) && this.inferredIsolated && !Symbols.isFlagOn(funcNode.symbol.flags, Flags.WORKER)) { dlog.warning(funcNode.pos, DiagnosticCode.FUNCTION_CAN_BE_MARKED_ISOLATED, funcNode.name); } @@ -341,12 +360,19 @@ public void visit(BLangSimpleVariable varNode) { int flags = varNode.symbol.flags; - if (isIsolatedObjectField() && isMutableField(varNode.symbol, varNode.type) && - !Symbols.isFlagOn(flags, Flags.PRIVATE)) { - dlog.error(varNode.pos, DiagnosticCode.INVALID_NON_PRIVATE_MUTABLE_FIELD_IN_ISOLATED_OBJECT); + BLangExpression expr = varNode.expr; + + BType fieldType = varNode.type; + if (isIsolatedObjectField() && isMutableField(varNode.symbol, fieldType)) { + if (!Symbols.isFlagOn(flags, Flags.PRIVATE)) { + dlog.error(varNode.pos, DiagnosticCode.INVALID_NON_PRIVATE_MUTABLE_FIELD_IN_ISOLATED_OBJECT); + } + + if (expr != null) { + validateIsolatedObjectFieldInitialValue(fieldType, expr); + } } - BLangExpression expr = varNode.expr; if (expr == null) { return; } @@ -398,8 +424,23 @@ public void visit(BLangSimpleVariableDef varDefNode) { @Override public void visit(BLangAssignment assignNode) { - analyzeNode(assignNode.varRef, env); - analyzeNode(assignNode.expr, env); + BLangExpression varRef = assignNode.varRef; + analyzeNode(varRef, env); + + BLangExpression expr = assignNode.expr; + analyzeNode(expr, env); + + BLangInvokableNode enclInvokable = env.enclInvokable; + if (varRef.getKind() == NodeKind.FIELD_BASED_ACCESS_EXPR) { + BLangFieldBasedAccess fieldAccess = (BLangFieldBasedAccess) varRef; + + if (enclInvokable != null && enclInvokable.getKind() == NodeKind.FUNCTION && + ((BLangFunction) enclInvokable).objInitFunction && + isIsolatedObjectFieldAccessViaSelf(fieldAccess, false)) { + validateIsolatedObjectFieldInitialValue( + ((BObjectType) enclInvokable.symbol.owner.type).fields.get(fieldAccess.field.value).type, expr); + } + } } @Override @@ -821,7 +862,7 @@ public void visit(BLangSimpleVarRef varRefExpr) { public void visit(BLangFieldBasedAccess fieldAccessExpr) { analyzeNode(fieldAccessExpr.expr, env); - if (!inLockStatement && isIsolatedObjectMutableFieldAccessViaSelf(fieldAccessExpr)) { + if (!inLockStatement && isIsolatedObjectMutableFieldAccessViaSelf(fieldAccessExpr, true)) { dlog.error(fieldAccessExpr.pos, DiagnosticCode.INVALID_MUTABLE_FIELD_ACCESS_IN_ISOLATED_OBJECT_OUTSIDE_LOCK); } @@ -859,7 +900,7 @@ public void visit(BLangInvocation.BLangActionInvocation actionInvocationExpr) { @Override public void visit(BLangTypeInit typeInitExpr) { BSymbol initInvocationSymbol = typeInitExpr.initInvocation.symbol; - if (initInvocationSymbol != null && !isIsolated(initInvocationSymbol)) { + if (initInvocationSymbol != null && !isIsolated(initInvocationSymbol.flags)) { inferredIsolated = false; if (isInIsolatedFunction(env.enclInvokable)) { @@ -1375,7 +1416,7 @@ private void analyzeInvocation(BLangInvocation invocationExpr) { } BSymbol symbol = invocationExpr.symbol; - if (symbol == null || symbol.getKind() == SymbolKind.ERROR_CONSTRUCTOR || isIsolated(symbol)) { + if (symbol == null || symbol.getKind() == SymbolKind.ERROR_CONSTRUCTOR || isIsolated(symbol.flags)) { return; } @@ -1414,10 +1455,10 @@ private boolean isInIsolatedFunction(BLangInvokableNode enclInvokable) { env.enclEnv.node.getKind() != NodeKind.ARROW_EXPR) { return false; } - return Symbols.isFlagOn(((BLangArrowFunction) env.enclEnv.node).funcType.flags, Flags.ISOLATED); + return isIsolated(((BLangArrowFunction) env.enclEnv.node).funcType.flags); } - return isIsolated(enclInvokable.symbol); + return isIsolated(enclInvokable.symbol.flags); } private boolean isRecordFieldDefaultValue(BLangType enclType) { @@ -1443,7 +1484,7 @@ private boolean isObjectFieldDefaultValueRequiringIsolation(SymbolEnv env) { return true; } - return isIsolated(initFunction.symbol); + return isIsolated(initFunction.symbol.flags); } private boolean isDefinitionReference(BSymbol symbol) { @@ -1453,8 +1494,8 @@ private boolean isDefinitionReference(BSymbol symbol) { Symbols.isFlagOn(symbol.flags, Flags.LISTENER); } - private boolean isIsolated(BSymbol symbol) { - return Symbols.isFlagOn(symbol.flags, Flags.ISOLATED); + private boolean isIsolated(int flags) { + return Symbols.isFlagOn(flags, Flags.ISOLATED); } private boolean isIsolatedObjectField() { @@ -1476,7 +1517,7 @@ private boolean isMutableField(BVarSymbol symbol, BType type) { return !Symbols.isFlagOn(symbol.flags, Flags.FINAL) || !Symbols.isFlagOn(type.flags, Flags.READONLY); } - private boolean isIsolatedObjectMutableFieldAccessViaSelf(BLangFieldBasedAccess fieldAccessExpr) { + private boolean isIsolatedObjectFieldAccessViaSelf(BLangFieldBasedAccess fieldAccessExpr, boolean ignoreInit) { BLangExpression expr = fieldAccessExpr.expr; if (expr.getKind() != NodeKind.SIMPLE_VARIABLE_REF) { @@ -1495,18 +1536,144 @@ private boolean isIsolatedObjectMutableFieldAccessViaSelf(BLangFieldBasedAccess BLangFunction enclFunction = (BLangFunction) enclInvokable; - if (enclFunction.objInitFunction || !enclFunction.attachedFunction) { + if (!enclFunction.attachedFunction) { + return false; + } + + if (enclFunction.objInitFunction && ignoreInit) { return false; } BType ownerType = enclInvokable.symbol.owner.type; - if (ownerType.tag != TypeTags.OBJECT || !Symbols.isFlagOn(ownerType.flags, Flags.ISOLATED)) { + return ownerType.tag == TypeTags.OBJECT && isIsolated(ownerType.flags); + } + + private boolean isIsolatedObjectMutableFieldAccessViaSelf(BLangFieldBasedAccess fieldAccessExpr, + boolean ignoreInit) { + if (!isIsolatedObjectFieldAccessViaSelf(fieldAccessExpr, ignoreInit)) { return false; } - BField field = ((BObjectType) ownerType).fields.get(fieldAccessExpr.field.value); + BField field = ((BObjectType) env.enclInvokable.symbol.owner.type).fields.get(fieldAccessExpr.field.value); return isMutableField(field.symbol, field.type); } + + private void validateIsolatedObjectFieldInitialValue(BType fieldType, BLangExpression expression) { + if (Symbols.isFlagOn(fieldType.flags, Flags.READONLY) || isIsolatedObjectTypes(fieldType)) { + return; + } + + validateReferenceOrUniqueExpression(expression); + } + + private boolean isIsolatedObjectTypes(BType type) { + int tag = type.tag; + + if (tag == TypeTags.OBJECT) { + return isIsolated(type.flags); + } + + if (tag != TypeTags.UNION) { + return false; + } + + for (BType memberType : ((BUnionType) type).getMemberTypes()) { + if (!isIsolated(memberType.flags)) { + return false; + } + } + return true; + } + + private void validateReferenceOrUniqueExpression(BLangExpression expression) { + if (Symbols.isFlagOn(expression.type.flags, Flags.READONLY)) { + return; + } + + switch (expression.getKind()) { + case SIMPLE_VARIABLE_REF: + BLangSimpleVarRef varRef = (BLangSimpleVarRef) expression; + BSymbol symbol = varRef.symbol; + + List varRefList = references.get(symbol); + + if (varRefList == null) { + references.put(symbol, new ArrayList() {{ add(varRef); }}); + return; + } + varRefList.add(varRef); + return; + case LITERAL: + case NUMERIC_LITERAL: + return; + case LIST_CONSTRUCTOR_EXPR: + for (BLangExpression expr : ((BLangListConstructorExpr) expression).exprs) { + validateReferenceOrUniqueExpression(expr); + } + return; + case RECORD_LITERAL_EXPR: + for (RecordLiteralNode.RecordField field : ((BLangRecordLiteral) expression).fields) { + if (field.isKeyValueField()) { + BLangRecordLiteral.BLangRecordKeyValueField keyValueField = + (BLangRecordLiteral.BLangRecordKeyValueField) field; + + BLangRecordLiteral.BLangRecordKey key = keyValueField.key; + if (key.computedKey) { + validateReferenceOrUniqueExpression(key.expr); + } + + validateReferenceOrUniqueExpression(keyValueField.valueExpr); + continue; + } + + if (field.getKind() == NodeKind.RECORD_LITERAL_SPREAD_OP) { + validateReferenceOrUniqueExpression( + ((BLangRecordLiteral.BLangRecordSpreadOperatorField) field).expr); + continue; + } + + validateReferenceOrUniqueExpression((BLangRecordLiteral.BLangRecordVarNameField) field); + } + return; + case INVOCATION: + BLangInvocation invocation = (BLangInvocation) expression; + + if (Symbols.isFlagOn(invocation.type.flags, Flags.READONLY)) { + return; + } + + if (invocation.langLibInvocation) { + String methodName = invocation.symbol.name.value; + + if (invocation.symbol.pkgID.name.value.equals(VALUE_LANG_LIB) && + (methodName.equals(CLONE_LANG_LIB_METHOD) || + methodName.equals(CLONE_READONLY_LANG_LIB_METHOD))) { + return; + } + } + + if (isIsolated(invocation.symbol.type.flags)) { + for (BLangExpression requiredArg : invocation.requiredArgs) { + if (requiredArg.getKind() == NodeKind.NAMED_ARGS_EXPR) { + validateReferenceOrUniqueExpression(((BLangNamedArgsExpression) requiredArg).expr); + continue; + } + + validateReferenceOrUniqueExpression(requiredArg); + } + + for (BLangExpression restArg : invocation.restArgs) { + if (restArg.getKind() == NodeKind.REST_ARGS_EXPR) { + validateReferenceOrUniqueExpression(((BLangRestArgsExpression) restArg).expr); + continue; + } + + validateReferenceOrUniqueExpression(restArg); + } + } + } + dlog.error(expression.pos, DiagnosticCode.INVALID_NON_UNIQUE_EXPRESSION_AS_INITIAL_VALUE_IN_ISOLATED_OBJECT); + } } diff --git a/compiler/ballerina-lang/src/main/resources/compiler.properties b/compiler/ballerina-lang/src/main/resources/compiler.properties index 769fcddd0c8d..c1ebf579a43f 100644 --- a/compiler/ballerina-lang/src/main/resources/compiler.properties +++ b/compiler/ballerina-lang/src/main/resources/compiler.properties @@ -1513,3 +1513,6 @@ error.invalid.non.private.mutable.field.in.isolated.object=\ error.invalid.mutable.field.access.in.isolated.object.outside.lock=\ invalid access of a mutable field of an ''isolated'' object outside a ''lock'' statement + +error.invalid.non.unique.expression.as.initial.value.in.isolated.object=\ + invalid initial value expression: expected a unique expression From 21c2e7d03d1f60e40810e42bb22dc13a4d6491ae Mon Sep 17 00:00:00 2001 From: MaryamZi Date: Thu, 8 Oct 2020 17:05:03 +0530 Subject: [PATCH 04/19] Add copy in/out analysis for isolated object methods --- .../util/diagnostic/DiagnosticCode.java | 2 + .../semantics/analyzer/IsolationAnalyzer.java | 175 ++++++++++++++---- .../compiler/semantics/model/SymbolEnv.java | 5 +- .../src/main/resources/compiler.properties | 6 + 4 files changed, 156 insertions(+), 32 deletions(-) diff --git a/compiler/ballerina-lang/src/main/java/org/ballerinalang/util/diagnostic/DiagnosticCode.java b/compiler/ballerina-lang/src/main/java/org/ballerinalang/util/diagnostic/DiagnosticCode.java index 53d2d1128d35..85846bd192cf 100644 --- a/compiler/ballerina-lang/src/main/java/org/ballerinalang/util/diagnostic/DiagnosticCode.java +++ b/compiler/ballerina-lang/src/main/java/org/ballerinalang/util/diagnostic/DiagnosticCode.java @@ -605,6 +605,8 @@ public enum DiagnosticCode { "invalid.mutable.field.access.in.isolated.object.outside.lock"), INVALID_NON_UNIQUE_EXPRESSION_AS_INITIAL_VALUE_IN_ISOLATED_OBJECT( "invalid.non.unique.expression.as.initial.value.in.isolated.object"), + INVALID_COPY_OUT_OF_MUTABLE_VALUE_FROM_ISOLATED_OBJECT("invalid.copy.out.of.mutable.value.from.isolated.object"), + INVALID_COPY_IN_OF_MUTABLE_VALUE_INTO_ISOLATED_OBJECT("invalid.copy.in.of.mutable.value.into.isolated.object"), COMPILER_PLUGIN_ERROR("compiler.plugin.crashed"), ; diff --git a/compiler/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/semantics/analyzer/IsolationAnalyzer.java b/compiler/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/semantics/analyzer/IsolationAnalyzer.java index 19d63791357f..493db07b23e6 100644 --- a/compiler/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/semantics/analyzer/IsolationAnalyzer.java +++ b/compiler/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/semantics/analyzer/IsolationAnalyzer.java @@ -187,6 +187,7 @@ import org.wso2.ballerinalang.compiler.tree.types.BLangUserDefinedType; import org.wso2.ballerinalang.compiler.tree.types.BLangValueType; import org.wso2.ballerinalang.compiler.util.CompilerContext; +import org.wso2.ballerinalang.compiler.util.Name; import org.wso2.ballerinalang.compiler.util.Names; import org.wso2.ballerinalang.compiler.util.TypeTags; import org.wso2.ballerinalang.util.Flags; @@ -195,6 +196,7 @@ import java.util.HashMap; import java.util.List; import java.util.Map; +import java.util.Stack; /** * Responsible for performing isolation analysis. @@ -210,16 +212,21 @@ public class IsolationAnalyzer extends BLangNodeVisitor { private SymbolEnv env; private SymbolTable symTable; + private SymbolResolver symResolver; + private Names names; private Types types; private BLangDiagnosticLog dlog; private boolean inferredIsolated = true; private boolean inLockStatement = false; private Map> references = new HashMap<>(); + private Stack copyInInLockInfoStack = new Stack<>(); private IsolationAnalyzer(CompilerContext context) { context.put(ISOLATION_ANALYZER_KEY, this); this.symTable = SymbolTable.getInstance(context); + this.symResolver = SymbolResolver.getInstance(context); + this.names = Names.getInstance(context); this.types = Types.getInstance(context); this.dlog = BLangDiagnosticLog.getInstance(context); } @@ -665,10 +672,36 @@ public void visit(BLangWhile whileNode) { public void visit(BLangLock lockNode) { boolean prevInLockStatement = this.inLockStatement; this.inLockStatement = true; + copyInInLockInfoStack.push(new CopyInInLockInfo(env.enclInvokable)); - analyzeNode(lockNode.body, env); + analyzeNode(lockNode.body, SymbolEnv.createLockEnv(lockNode, env)); + + CopyInInLockInfo copyInInLockInfo = copyInInLockInfoStack.pop(); this.inLockStatement = prevInLockStatement; + + if (copyInInLockInfo.hasMutableAccessInLock) { + for (BLangSimpleVarRef varRef : copyInInLockInfo.varRefs) { + dlog.error(varRef.pos, DiagnosticCode.INVALID_COPY_IN_OF_MUTABLE_VALUE_INTO_ISOLATED_OBJECT); + } + return; + } + + for (int i = copyInInLockInfoStack.size() - 1; i >= 0; i--) { + CopyInInLockInfo prevCopyInInLockInfo = copyInInLockInfoStack.get(i); + + BLangInvokableNode enclInvokableNode = prevCopyInInLockInfo.enclInvokableNode; + if (enclInvokableNode.getKind() != NodeKind.FUNCTION) { + return; + } + + BLangFunction function = (BLangFunction) enclInvokableNode; + if (!function.attachedFunction || function.objInitFunction) { + return; + } + + prevCopyInInLockInfo.varRefs.addAll(copyInInLockInfo.varRefs); + } } @Override @@ -797,11 +830,15 @@ public void visit(BLangSimpleVarRef varRefExpr) { BLangInvokableNode enclInvokable = env.enclInvokable; BLangType enclType = env.enclType; - if (symbol == null) { return; } + if (inLockStatement && isInIsolatedObjectMethod(env, true) && !varRefExpr.lhsVar && + isInvalidCopyIn(varRefExpr, env)) { + copyInInLockInfoStack.peek().varRefs.add(varRefExpr); + } + boolean inIsolatedFunction = isInIsolatedFunction(enclInvokable); boolean recordFieldDefaultValue = isRecordFieldDefaultValue(enclType); boolean objectFieldDefaultValue = !recordFieldDefaultValue && isObjectFieldDefaultValueRequiringIsolation(env); @@ -836,7 +873,6 @@ public void visit(BLangSimpleVarRef varRefExpr) { return; } - inferredIsolated = false; if (inIsolatedFunction) { @@ -862,10 +898,24 @@ public void visit(BLangSimpleVarRef varRefExpr) { public void visit(BLangFieldBasedAccess fieldAccessExpr) { analyzeNode(fieldAccessExpr.expr, env); - if (!inLockStatement && isIsolatedObjectMutableFieldAccessViaSelf(fieldAccessExpr, true)) { + if (!isIsolatedObjectMutableFieldAccessViaSelf(fieldAccessExpr, true)) { + return; + } + + if (inLockStatement) { + copyInInLockInfoStack.peek().hasMutableAccessInLock = true; + } else { dlog.error(fieldAccessExpr.pos, DiagnosticCode.INVALID_MUTABLE_FIELD_ACCESS_IN_ISOLATED_OBJECT_OUTSIDE_LOCK); } + + if (fieldAccessExpr.lhsVar) { + return; + } + + if (isInvalidCopyingOfMutableIsolatedObjectField(fieldAccessExpr)) { + dlog.error(fieldAccessExpr.pos, DiagnosticCode.INVALID_COPY_OUT_OF_MUTABLE_VALUE_FROM_ISOLATED_OBJECT); + } } @Override @@ -1528,25 +1578,7 @@ private boolean isIsolatedObjectFieldAccessViaSelf(BLangFieldBasedAccess fieldAc return false; } - BLangInvokableNode enclInvokable = env.enclInvokable; - - if (enclInvokable == null || enclInvokable.getKind() != NodeKind.FUNCTION) { - return false; - } - - BLangFunction enclFunction = (BLangFunction) enclInvokable; - - if (!enclFunction.attachedFunction) { - return false; - } - - if (enclFunction.objInitFunction && ignoreInit) { - return false; - } - - BType ownerType = enclInvokable.symbol.owner.type; - - return ownerType.tag == TypeTags.OBJECT && isIsolated(ownerType.flags); + return isInIsolatedObjectMethod(env, ignoreInit); } private boolean isIsolatedObjectMutableFieldAccessViaSelf(BLangFieldBasedAccess fieldAccessExpr, @@ -1644,14 +1676,8 @@ private void validateReferenceOrUniqueExpression(BLangExpression expression) { return; } - if (invocation.langLibInvocation) { - String methodName = invocation.symbol.name.value; - - if (invocation.symbol.pkgID.name.value.equals(VALUE_LANG_LIB) && - (methodName.equals(CLONE_LANG_LIB_METHOD) || - methodName.equals(CLONE_READONLY_LANG_LIB_METHOD))) { - return; - } + if (isCloneOrCloneReadOnlyInvocation(invocation)) { + return; } if (isIsolated(invocation.symbol.type.flags)) { @@ -1676,4 +1702,91 @@ private void validateReferenceOrUniqueExpression(BLangExpression expression) { } dlog.error(expression.pos, DiagnosticCode.INVALID_NON_UNIQUE_EXPRESSION_AS_INITIAL_VALUE_IN_ISOLATED_OBJECT); } + + private boolean isCloneOrCloneReadOnlyInvocation(BLangInvocation invocation) { + if (!invocation.langLibInvocation) { + return false; + } + + String methodName = invocation.symbol.name.value; + + return invocation.symbol.pkgID.name.value.equals(VALUE_LANG_LIB) && + (methodName.equals(CLONE_LANG_LIB_METHOD) || methodName.equals(CLONE_READONLY_LANG_LIB_METHOD)); + } + + private boolean isInvalidCopyingOfMutableIsolatedObjectField(BLangExpression expression) { + BType type = expression.type; + if (Symbols.isFlagOn(type.flags, Flags.READONLY)) { + return false; + } + + BLangNode parent = expression.parent; + + if (!(parent instanceof BLangExpression)) { + return true; + } + + BLangExpression parentExpression = (BLangExpression) parent; + + switch (parent.getKind()) { + case FIELD_BASED_ACCESS_EXPR: + case INDEX_BASED_ACCESS_EXPR: + return isInvalidCopyingOfMutableIsolatedObjectField(parentExpression); + case INVOCATION: + return !isCloneOrCloneReadOnlyInvocation((BLangInvocation) parentExpression); + case GROUP_EXPR: + return isInvalidCopyingOfMutableIsolatedObjectField(((BLangGroupExpr) parentExpression).expression); + } + + return !isIsolatedObjectTypes(type); + } + + private boolean isInIsolatedObjectMethod(SymbolEnv env, boolean ignoreInit) { + BLangInvokableNode enclInvokable = env.enclInvokable; + + if (enclInvokable == null || enclInvokable.getKind() != NodeKind.FUNCTION) { + return false; + } + + BLangFunction enclFunction = (BLangFunction) enclInvokable; + + if (!enclFunction.attachedFunction) { + return false; + } + + if (enclFunction.objInitFunction && ignoreInit) { + return false; + } + + BType ownerType = enclInvokable.symbol.owner.type; + + return ownerType.tag == TypeTags.OBJECT && isIsolated(ownerType.flags); + } + + private boolean isInvalidCopyIn(BLangSimpleVarRef varRefExpr, SymbolEnv currentEnv) { + return isInvalidCopyIn(varRefExpr, names.fromIdNode(varRefExpr.variableName), varRefExpr.symbol.tag, + currentEnv); + } + + private boolean isInvalidCopyIn(BLangSimpleVarRef varRefExpr, Name name, int symTag, SymbolEnv currentEnv) { + if (symResolver.lookupSymbolInGivenScope(currentEnv, name, symTag) != symTable.notFoundSymbol) { + return false; + } + + if (currentEnv.node.getKind() == NodeKind.LOCK) { + return isInvalidCopyingOfMutableIsolatedObjectField(varRefExpr); + } + + return isInvalidCopyIn(varRefExpr, name, symTag, env.enclEnv); + } + + private static class CopyInInLockInfo { + BLangInvokableNode enclInvokableNode; + boolean hasMutableAccessInLock = false; + List varRefs = new ArrayList<>(); + + private CopyInInLockInfo(BLangInvokableNode enclInvokableNode) { + this.enclInvokableNode = enclInvokableNode; + } + } } diff --git a/compiler/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/semantics/model/SymbolEnv.java b/compiler/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/semantics/model/SymbolEnv.java index 98a15f59f183..339a2abbf909 100644 --- a/compiler/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/semantics/model/SymbolEnv.java +++ b/compiler/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/semantics/model/SymbolEnv.java @@ -337,11 +337,14 @@ public static SymbolEnv createStreamingInputEnv(BLangNode node, SymbolEnv env) { return createEnv(node, env); } + public static SymbolEnv createLockEnv(BLangNode node, SymbolEnv env) { + return createEnv(node, env); + } + private static SymbolEnv createEnv(BLangNode node, SymbolEnv env) { SymbolEnv symbolEnv = new SymbolEnv(node, new Scope(env.scope.owner)); symbolEnv.envCount = 0; env.copyTo(symbolEnv); - symbolEnv.node = node; return symbolEnv; } diff --git a/compiler/ballerina-lang/src/main/resources/compiler.properties b/compiler/ballerina-lang/src/main/resources/compiler.properties index 70c911cc7cd2..81ef76453712 100644 --- a/compiler/ballerina-lang/src/main/resources/compiler.properties +++ b/compiler/ballerina-lang/src/main/resources/compiler.properties @@ -1519,3 +1519,9 @@ error.invalid.mutable.field.access.in.isolated.object.outside.lock=\ error.invalid.non.unique.expression.as.initial.value.in.isolated.object=\ invalid initial value expression: expected a unique expression + +error.invalid.copy.out.of.mutable.value.from.isolated.object=\ + invalid attempt to copy out a mutable value from an ''isolated'' object + +error.invalid.copy.in.of.mutable.value.into.isolated.object=\ + invalid attempt to copy a mutable value into an ''isolated'' object From e97efa0eb69f04d0de4d2a241de7f2bfce3079dc Mon Sep 17 00:00:00 2001 From: MaryamZi Date: Fri, 9 Oct 2020 12:22:55 +0530 Subject: [PATCH 05/19] Validate invocation isolation in lock accessing mutable isolated object field --- .../util/diagnostic/DiagnosticCode.java | 2 ++ .../semantics/analyzer/IsolationAnalyzer.java | 28 +++++++++++++++---- .../src/main/resources/compiler.properties | 3 ++ 3 files changed, 27 insertions(+), 6 deletions(-) diff --git a/compiler/ballerina-lang/src/main/java/org/ballerinalang/util/diagnostic/DiagnosticCode.java b/compiler/ballerina-lang/src/main/java/org/ballerinalang/util/diagnostic/DiagnosticCode.java index 85846bd192cf..eac4da9fdd9c 100644 --- a/compiler/ballerina-lang/src/main/java/org/ballerinalang/util/diagnostic/DiagnosticCode.java +++ b/compiler/ballerina-lang/src/main/java/org/ballerinalang/util/diagnostic/DiagnosticCode.java @@ -607,6 +607,8 @@ public enum DiagnosticCode { "invalid.non.unique.expression.as.initial.value.in.isolated.object"), INVALID_COPY_OUT_OF_MUTABLE_VALUE_FROM_ISOLATED_OBJECT("invalid.copy.out.of.mutable.value.from.isolated.object"), INVALID_COPY_IN_OF_MUTABLE_VALUE_INTO_ISOLATED_OBJECT("invalid.copy.in.of.mutable.value.into.isolated.object"), + INVALID_NON_ISOLATED_INVOCATION_IN_ISOLATED_OBJECT_METHOD( + "invalid.non.isolated.invocation.in.isolated.object.method"), COMPILER_PLUGIN_ERROR("compiler.plugin.crashed"), ; diff --git a/compiler/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/semantics/analyzer/IsolationAnalyzer.java b/compiler/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/semantics/analyzer/IsolationAnalyzer.java index 493db07b23e6..bd49fb59ba0a 100644 --- a/compiler/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/semantics/analyzer/IsolationAnalyzer.java +++ b/compiler/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/semantics/analyzer/IsolationAnalyzer.java @@ -220,7 +220,7 @@ public class IsolationAnalyzer extends BLangNodeVisitor { private boolean inferredIsolated = true; private boolean inLockStatement = false; private Map> references = new HashMap<>(); - private Stack copyInInLockInfoStack = new Stack<>(); + private Stack copyInInLockInfoStack = new Stack<>(); private IsolationAnalyzer(CompilerContext context) { context.put(ISOLATION_ANALYZER_KEY, this); @@ -672,11 +672,11 @@ public void visit(BLangWhile whileNode) { public void visit(BLangLock lockNode) { boolean prevInLockStatement = this.inLockStatement; this.inLockStatement = true; - copyInInLockInfoStack.push(new CopyInInLockInfo(env.enclInvokable)); + copyInInLockInfoStack.push(new PotentiallyInvalidExpressionInfo(env.enclInvokable)); analyzeNode(lockNode.body, SymbolEnv.createLockEnv(lockNode, env)); - CopyInInLockInfo copyInInLockInfo = copyInInLockInfoStack.pop(); + PotentiallyInvalidExpressionInfo copyInInLockInfo = copyInInLockInfoStack.pop(); this.inLockStatement = prevInLockStatement; @@ -684,11 +684,16 @@ public void visit(BLangLock lockNode) { for (BLangSimpleVarRef varRef : copyInInLockInfo.varRefs) { dlog.error(varRef.pos, DiagnosticCode.INVALID_COPY_IN_OF_MUTABLE_VALUE_INTO_ISOLATED_OBJECT); } + + for (BLangInvocation invocation : copyInInLockInfo.invocations) { + dlog.error(invocation.pos, DiagnosticCode.INVALID_NON_ISOLATED_INVOCATION_IN_ISOLATED_OBJECT_METHOD); + } + return; } for (int i = copyInInLockInfoStack.size() - 1; i >= 0; i--) { - CopyInInLockInfo prevCopyInInLockInfo = copyInInLockInfoStack.get(i); + PotentiallyInvalidExpressionInfo prevCopyInInLockInfo = copyInInLockInfoStack.get(i); BLangInvokableNode enclInvokableNode = prevCopyInInLockInfo.enclInvokableNode; if (enclInvokableNode.getKind() != NodeKind.FUNCTION) { @@ -1470,6 +1475,10 @@ private void analyzeInvocation(BLangInvocation invocationExpr) { return; } + if (inLockStatement && isInIsolatedObjectMethod(env, true)) { + copyInInLockInfoStack.peek().invocations.add(invocationExpr); + } + inferredIsolated = false; if (isInIsolatedFunction(env.enclInvokable)) { @@ -1780,12 +1789,19 @@ private boolean isInvalidCopyIn(BLangSimpleVarRef varRefExpr, Name name, int sym return isInvalidCopyIn(varRefExpr, name, symTag, env.enclEnv); } - private static class CopyInInLockInfo { + /** + * For isolated objects, invalid copy ins and non-isolated invocations should result in compilation errors if + * they happen in a lock statement accessing a mutable field of the object. This class holds potentially + * erroneous expression per lock statement, and whether or not the lock statement accesses a mutable field. + */ + private static class PotentiallyInvalidExpressionInfo { BLangInvokableNode enclInvokableNode; boolean hasMutableAccessInLock = false; + List varRefs = new ArrayList<>(); + List invocations = new ArrayList<>(); - private CopyInInLockInfo(BLangInvokableNode enclInvokableNode) { + private PotentiallyInvalidExpressionInfo(BLangInvokableNode enclInvokableNode) { this.enclInvokableNode = enclInvokableNode; } } diff --git a/compiler/ballerina-lang/src/main/resources/compiler.properties b/compiler/ballerina-lang/src/main/resources/compiler.properties index 81ef76453712..30879576b792 100644 --- a/compiler/ballerina-lang/src/main/resources/compiler.properties +++ b/compiler/ballerina-lang/src/main/resources/compiler.properties @@ -1525,3 +1525,6 @@ error.invalid.copy.out.of.mutable.value.from.isolated.object=\ error.invalid.copy.in.of.mutable.value.into.isolated.object=\ invalid attempt to copy a mutable value into an ''isolated'' object + +error.invalid.non.isolated.invocation.in.isolated.object.method=\ + invalid invocation of a non-isolated function in a method accessing a mutable field of an ''isolated'' object From 7eaa536daf127000e0ab79e963c856631642c4cf Mon Sep 17 00:00:00 2001 From: MaryamZi Date: Fri, 9 Oct 2020 21:10:01 +0530 Subject: [PATCH 06/19] Consider the isolated flag for subtyping --- .../src/main/java/org/ballerinalang/jvm/TypeChecker.java | 6 ++++++ .../compiler/semantics/analyzer/IsolationAnalyzer.java | 8 ++++++++ .../compiler/semantics/analyzer/TypeChecker.java | 5 +++++ .../ballerinalang/compiler/semantics/analyzer/Types.java | 4 ++++ .../compiler/semantics/model/types/BObjectType.java | 8 +++++++- 5 files changed, 30 insertions(+), 1 deletion(-) diff --git a/bvm/ballerina-runtime/src/main/java/org/ballerinalang/jvm/TypeChecker.java b/bvm/ballerina-runtime/src/main/java/org/ballerinalang/jvm/TypeChecker.java index fd7c774a8023..5a841841bdd2 100644 --- a/bvm/ballerina-runtime/src/main/java/org/ballerinalang/jvm/TypeChecker.java +++ b/bvm/ballerina-runtime/src/main/java/org/ballerinalang/jvm/TypeChecker.java @@ -1362,6 +1362,12 @@ private static boolean checkObjectEquivalency(Object sourceVal, BType sourceType unresolvedTypes.add(pair); BObjectType sourceObjectType = (BObjectType) sourceType; + + if (Flags.isFlagOn(targetType.flags, Flags.ISOLATED) && + !Flags.isFlagOn(sourceObjectType.flags, Flags.ISOLATED)) { + return false; + } + Map targetFields = targetType.getFields(); Map sourceFields = sourceObjectType.getFields(); AttachedFunction[] targetFuncs = targetType.getAttachedFunctions(); diff --git a/compiler/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/semantics/analyzer/IsolationAnalyzer.java b/compiler/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/semantics/analyzer/IsolationAnalyzer.java index bd49fb59ba0a..1d555acc3b65 100644 --- a/compiler/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/semantics/analyzer/IsolationAnalyzer.java +++ b/compiler/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/semantics/analyzer/IsolationAnalyzer.java @@ -265,6 +265,14 @@ public void visit(BLangPackage pkgNode) { } for (BLangClassDefinition classDefinition : pkgNode.classDefinitions) { + if (classDefinition.flagSet.contains(Flag.ANONYMOUS) && isIsolated(classDefinition.type.flags)) { + // If this is a class definition for an object constructor expression, and the type is `isolated`, + // that is due to the expected type being an `isolated` object. We now mark the class definition also + // as `isolated`, to enforce the isolation validation. + classDefinition.flagSet.add(Flag.ISOLATED); + classDefinition.symbol.flags |= Flags.ISOLATED; + } + analyzeNode(classDefinition, env); } diff --git a/compiler/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/semantics/analyzer/TypeChecker.java b/compiler/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/semantics/analyzer/TypeChecker.java index 8bee9d5a566c..da97d11b2feb 100644 --- a/compiler/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/semantics/analyzer/TypeChecker.java +++ b/compiler/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/semantics/analyzer/TypeChecker.java @@ -2906,6 +2906,11 @@ public void visit(BLangTypeInit cIExpr) { return; } + if (Symbols.isFlagOn(expType.flags, Flags.ISOLATED)) { + actualType.flags |= Flags.ISOLATED; + actualType.tsymbol.flags |= Flags.ISOLATED; + } + if (((BObjectTypeSymbol) actualType.tsymbol).initializerFunc != null) { cIExpr.initInvocation.symbol = ((BObjectTypeSymbol) actualType.tsymbol).initializerFunc.symbol; checkInvocationParam(cIExpr.initInvocation); diff --git a/compiler/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/semantics/analyzer/Types.java b/compiler/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/semantics/analyzer/Types.java index cd2dabda80cc..185fdd7eb059 100644 --- a/compiler/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/semantics/analyzer/Types.java +++ b/compiler/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/semantics/analyzer/Types.java @@ -1269,6 +1269,10 @@ private boolean checkStructEquivalency(BType rhsType, BType lhsType, Set unresolvedTypes) { + if (Symbols.isFlagOn(lhsType.flags, Flags.ISOLATED) && !Symbols.isFlagOn(rhsType.flags, Flags.ISOLATED)) { + return false; + } + BObjectTypeSymbol lhsStructSymbol = (BObjectTypeSymbol) lhsType.tsymbol; BObjectTypeSymbol rhsStructSymbol = (BObjectTypeSymbol) rhsType.tsymbol; List lhsFuncs = lhsStructSymbol.attachedFuncs; diff --git a/compiler/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/semantics/model/types/BObjectType.java b/compiler/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/semantics/model/types/BObjectType.java index 34736a602e41..1133ceaa0c3a 100644 --- a/compiler/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/semantics/model/types/BObjectType.java +++ b/compiler/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/semantics/model/types/BObjectType.java @@ -76,6 +76,12 @@ public String toString() { if (shouldPrintShape(tsymbol.name)) { StringBuilder sb = new StringBuilder(); + + int symbolFlags = tsymbol.flags; + if (Symbols.isFlagOn(symbolFlags, Flags.ISOLATED)) { + sb.append("isolated "); + } + sb.append(OBJECT).append(SPACE).append(LEFT_CURL); for (BField field : fields.values()) { int flags = field.symbol.flags; @@ -102,7 +108,7 @@ public String toString() { } sb.append(SPACE).append(RIGHT_CURL); - if (Symbols.isFlagOn(tsymbol.flags, Flags.READONLY)) { + if (Symbols.isFlagOn(symbolFlags, Flags.READONLY)) { sb.append(" & readonly"); } From 3f0753b57e2fa74ed7f592ec02438af2a4fd68b2 Mon Sep 17 00:00:00 2001 From: MaryamZi Date: Mon, 12 Oct 2020 17:22:31 +0530 Subject: [PATCH 07/19] Fix copy in/out analysis --- .../semantics/analyzer/IsolationAnalyzer.java | 135 +++++++++++++++--- 1 file changed, 116 insertions(+), 19 deletions(-) diff --git a/compiler/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/semantics/analyzer/IsolationAnalyzer.java b/compiler/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/semantics/analyzer/IsolationAnalyzer.java index 1d555acc3b65..69d887273429 100644 --- a/compiler/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/semantics/analyzer/IsolationAnalyzer.java +++ b/compiler/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/semantics/analyzer/IsolationAnalyzer.java @@ -23,6 +23,7 @@ import org.ballerinalang.model.elements.Flag; import org.ballerinalang.model.symbols.SymbolKind; import org.ballerinalang.model.tree.NodeKind; +import org.ballerinalang.model.tree.expressions.ExpressionNode; import org.ballerinalang.model.tree.expressions.RecordLiteralNode; import org.ballerinalang.util.diagnostic.DiagnosticCode; import org.wso2.ballerinalang.compiler.diagnostic.BLangDiagnosticLog; @@ -847,9 +848,14 @@ public void visit(BLangSimpleVarRef varRefExpr) { return; } - if (inLockStatement && isInIsolatedObjectMethod(env, true) && !varRefExpr.lhsVar && - isInvalidCopyIn(varRefExpr, env)) { - copyInInLockInfoStack.peek().varRefs.add(varRefExpr); + if (inLockStatement && isInIsolatedObjectMethod(env, true)) { + if ((!varRefExpr.lhsVar || varRefExpr.parent.getKind() != NodeKind.ASSIGNMENT) && + !Names.SELF.value.equals(varRefExpr.variableName.value) && isInvalidCopyIn(varRefExpr, env)) { + copyInInLockInfoStack.peek().varRefs.add(varRefExpr); + } else if (!varRefExpr.lhsVar && + isInvalidCopyingOfMutableValueInIsolatedObject(varRefExpr, true)) { + dlog.error(varRefExpr.pos, DiagnosticCode.INVALID_COPY_OUT_OF_MUTABLE_VALUE_FROM_ISOLATED_OBJECT); + } } boolean inIsolatedFunction = isInIsolatedFunction(enclInvokable); @@ -917,18 +923,10 @@ public void visit(BLangFieldBasedAccess fieldAccessExpr) { if (inLockStatement) { copyInInLockInfoStack.peek().hasMutableAccessInLock = true; - } else { - dlog.error(fieldAccessExpr.pos, - DiagnosticCode.INVALID_MUTABLE_FIELD_ACCESS_IN_ISOLATED_OBJECT_OUTSIDE_LOCK); - } - - if (fieldAccessExpr.lhsVar) { return; } - if (isInvalidCopyingOfMutableIsolatedObjectField(fieldAccessExpr)) { - dlog.error(fieldAccessExpr.pos, DiagnosticCode.INVALID_COPY_OUT_OF_MUTABLE_VALUE_FROM_ISOLATED_OBJECT); - } + dlog.error(fieldAccessExpr.pos, DiagnosticCode.INVALID_MUTABLE_FIELD_ACCESS_IN_ISOLATED_OBJECT_OUTSIDE_LOCK); } @Override @@ -1473,7 +1471,14 @@ public void visit(BLangXMLNavigationAccess xmlNavigation) { private void analyzeInvocation(BLangInvocation invocationExpr) { List args = new ArrayList<>(invocationExpr.requiredArgs); + + BLangExpression expr = invocationExpr.expr; + if (expr != null && !args.isEmpty() && args.get(0) != expr) { + args.add(0, expr); + } + args.addAll(invocationExpr.restArgs); + for (BLangExpression argExpr : args) { analyzeNode(argExpr, env); } @@ -1731,7 +1736,7 @@ private boolean isCloneOrCloneReadOnlyInvocation(BLangInvocation invocation) { (methodName.equals(CLONE_LANG_LIB_METHOD) || methodName.equals(CLONE_READONLY_LANG_LIB_METHOD)); } - private boolean isInvalidCopyingOfMutableIsolatedObjectField(BLangExpression expression) { + private boolean isInvalidCopyingOfMutableValueInIsolatedObject(BLangExpression expression, boolean copyOut) { BType type = expression.type; if (Symbols.isFlagOn(type.flags, Flags.READONLY)) { return false; @@ -1739,25 +1744,117 @@ private boolean isInvalidCopyingOfMutableIsolatedObjectField(BLangExpression exp BLangNode parent = expression.parent; + NodeKind kind = parent.getKind(); if (!(parent instanceof BLangExpression)) { - return true; + if (!copyOut) { + return true; + } + + switch (kind) { + case ASSIGNMENT: + BLangExpression varRef = ((BLangAssignment) parent).varRef; + + if (varRef.getKind() != NodeKind.SIMPLE_VARIABLE_REF) { + return false; + } + + BLangSimpleVarRef simpleVarRef = (BLangSimpleVarRef) varRef; + return isDefinedOutsideLock(names.fromIdNode(simpleVarRef.variableName), simpleVarRef.symbol.tag, + env); + case RECORD_DESTRUCTURE: + return hasRefDefinedOutsideLock(((BLangRecordDestructure) parent).varRef); + case TUPLE_DESTRUCTURE: + return hasRefDefinedOutsideLock(((BLangTupleDestructure) parent).varRef); + case ERROR_DESTRUCTURE: + return hasRefDefinedOutsideLock(((BLangErrorDestructure) parent).varRef); + } + return false; } BLangExpression parentExpression = (BLangExpression) parent; - switch (parent.getKind()) { + switch (kind) { case FIELD_BASED_ACCESS_EXPR: case INDEX_BASED_ACCESS_EXPR: - return isInvalidCopyingOfMutableIsolatedObjectField(parentExpression); + return isInvalidCopyingOfMutableValueInIsolatedObject(parentExpression, copyOut); case INVOCATION: - return !isCloneOrCloneReadOnlyInvocation((BLangInvocation) parentExpression); + BLangInvocation invocation = (BLangInvocation) parentExpression; + BLangExpression calledOnExpr = invocation.expr; + + if (calledOnExpr == expression && !isIsolatedObjectTypes(type)) { + return true; + } + + return !isIsolated(invocation.symbol.flags) && !isCloneOrCloneReadOnlyInvocation(invocation); case GROUP_EXPR: - return isInvalidCopyingOfMutableIsolatedObjectField(((BLangGroupExpr) parentExpression).expression); + return isInvalidCopyingOfMutableValueInIsolatedObject(((BLangGroupExpr) parentExpression).expression, + copyOut); } return !isIsolatedObjectTypes(type); } + private boolean hasRefDefinedOutsideLock(BLangExpression variableReference) { + switch (variableReference.getKind()) { + case SIMPLE_VARIABLE_REF: + BLangSimpleVarRef simpleVarRef = (BLangSimpleVarRef) variableReference; + return isDefinedOutsideLock(names.fromIdNode(simpleVarRef.variableName), simpleVarRef.symbol.tag, + env); + case RECORD_VARIABLE_REF: + BLangRecordVarRef recordVarRef = (BLangRecordVarRef) variableReference; + for (BLangRecordVarRef.BLangRecordVarRefKeyValue recordRefField : recordVarRef.recordRefFields) { + if (hasRefDefinedOutsideLock(recordRefField.variableReference)) { + return true; + } + } + ExpressionNode recordRestParam = recordVarRef.restParam; + return recordRestParam != null && hasRefDefinedOutsideLock((BLangExpression) recordRestParam); + case TUPLE_VARIABLE_REF: + BLangTupleVarRef tupleVarRef = (BLangTupleVarRef) variableReference; + for (BLangExpression expression : tupleVarRef.expressions) { + if (hasRefDefinedOutsideLock(expression)) { + return true; + } + } + ExpressionNode tupleRestParam = tupleVarRef.restParam; + return tupleRestParam != null && hasRefDefinedOutsideLock((BLangExpression) tupleRestParam); + case ERROR_VARIABLE_REF: + BLangErrorVarRef errorVarRef = (BLangErrorVarRef) variableReference; + + BLangVariableReference message = errorVarRef.message; + if (message != null && hasRefDefinedOutsideLock(message)) { + return true; + } + + BLangVariableReference cause = errorVarRef.cause; + if (cause != null && hasRefDefinedOutsideLock(cause)) { + return true; + } + + for (BLangNamedArgsExpression namedArgsExpression : errorVarRef.detail) { + if (hasRefDefinedOutsideLock(namedArgsExpression.expr)) { + return true; + } + } + + BLangVariableReference errorRestVar = errorVarRef.restVar; + return errorRestVar != null && hasRefDefinedOutsideLock(errorRestVar); + } + return false; + } + + private boolean isDefinedOutsideLock(Name name, int symTag, SymbolEnv currentEnv) { + if (symResolver.lookupSymbolInGivenScope(currentEnv, name, symTag) != symTable.notFoundSymbol) { + return false; + } + + if (currentEnv.node.getKind() == NodeKind.LOCK) { + return true; + } + + return isDefinedOutsideLock(name, symTag, currentEnv.enclEnv); + } + private boolean isInIsolatedObjectMethod(SymbolEnv env, boolean ignoreInit) { BLangInvokableNode enclInvokable = env.enclInvokable; @@ -1791,7 +1888,7 @@ private boolean isInvalidCopyIn(BLangSimpleVarRef varRefExpr, Name name, int sym } if (currentEnv.node.getKind() == NodeKind.LOCK) { - return isInvalidCopyingOfMutableIsolatedObjectField(varRefExpr); + return isInvalidCopyingOfMutableValueInIsolatedObject(varRefExpr, false); } return isInvalidCopyIn(varRefExpr, name, symTag, env.enclEnv); From 414715816d7efa08b4cbeda9f439f3af6354a633 Mon Sep 17 00:00:00 2001 From: MaryamZi Date: Tue, 13 Oct 2020 12:25:12 +0530 Subject: [PATCH 08/19] Avoid logging not private errors for isolated object typedesc fields --- .../semantics/analyzer/IsolationAnalyzer.java | 16 +++------------- 1 file changed, 3 insertions(+), 13 deletions(-) diff --git a/compiler/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/semantics/analyzer/IsolationAnalyzer.java b/compiler/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/semantics/analyzer/IsolationAnalyzer.java index 69d887273429..ad07f973947d 100644 --- a/compiler/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/semantics/analyzer/IsolationAnalyzer.java +++ b/compiler/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/semantics/analyzer/IsolationAnalyzer.java @@ -379,7 +379,7 @@ public void visit(BLangSimpleVariable varNode) { BLangExpression expr = varNode.expr; BType fieldType = varNode.type; - if (isIsolatedObjectField() && isMutableField(varNode.symbol, fieldType)) { + if (isIsolatedClassField() && isMutableField(varNode.symbol, fieldType)) { if (!Symbols.isFlagOn(flags, Flags.PRIVATE)) { dlog.error(varNode.pos, DiagnosticCode.INVALID_NON_PRIVATE_MUTABLE_FIELD_IN_ISOLATED_OBJECT); } @@ -1570,19 +1570,9 @@ private boolean isIsolated(int flags) { return Symbols.isFlagOn(flags, Flags.ISOLATED); } - private boolean isIsolatedObjectField() { + private boolean isIsolatedClassField() { BLangNode node = env.node; - NodeKind kind = node.getKind(); - - if (kind == NodeKind.OBJECT_TYPE) { - return ((BLangObjectTypeNode) node).flagSet.contains(Flag.ISOLATED); - } - - if (kind == NodeKind.CLASS_DEFN) { - return ((BLangClassDefinition) node).flagSet.contains(Flag.ISOLATED); - } - - return false; + return node.getKind() == NodeKind.CLASS_DEFN && ((BLangClassDefinition) node).flagSet.contains(Flag.ISOLATED); } private boolean isMutableField(BVarSymbol symbol, BType type) { From 38a644e35e7c33c957bcfd44b5c722d75a99ac06 Mon Sep 17 00:00:00 2001 From: MaryamZi Date: Tue, 13 Oct 2020 16:13:19 +0530 Subject: [PATCH 09/19] Infer isolatedness from expType only for object constructor exprs --- .../ballerinalang/compiler/semantics/analyzer/TypeChecker.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/compiler/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/semantics/analyzer/TypeChecker.java b/compiler/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/semantics/analyzer/TypeChecker.java index da97d11b2feb..72c9af7bc0f8 100644 --- a/compiler/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/semantics/analyzer/TypeChecker.java +++ b/compiler/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/semantics/analyzer/TypeChecker.java @@ -2906,7 +2906,8 @@ public void visit(BLangTypeInit cIExpr) { return; } - if (Symbols.isFlagOn(expType.flags, Flags.ISOLATED)) { + if (Symbols.isFlagOn(actualType.tsymbol.flags, Flags.ANONYMOUS) && + Symbols.isFlagOn(expType.flags, Flags.ISOLATED)) { actualType.flags |= Flags.ISOLATED; actualType.tsymbol.flags |= Flags.ISOLATED; } From 6fbed4fe638e17092bf9e5db65de44dc66906568 Mon Sep 17 00:00:00 2001 From: MaryamZi Date: Wed, 14 Oct 2020 23:40:16 +0530 Subject: [PATCH 10/19] Fix isolation analysis for uniqueness and method calls on self --- .../semantics/analyzer/IsolationAnalyzer.java | 211 ++++++++++++++---- 1 file changed, 173 insertions(+), 38 deletions(-) diff --git a/compiler/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/semantics/analyzer/IsolationAnalyzer.java b/compiler/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/semantics/analyzer/IsolationAnalyzer.java index ad07f973947d..d79199ccb220 100644 --- a/compiler/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/semantics/analyzer/IsolationAnalyzer.java +++ b/compiler/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/semantics/analyzer/IsolationAnalyzer.java @@ -220,7 +220,7 @@ public class IsolationAnalyzer extends BLangNodeVisitor { private boolean inferredIsolated = true; private boolean inLockStatement = false; - private Map> references = new HashMap<>(); + private Map uniqueInitAndReferenceInfo = new HashMap<>(); private Stack copyInInLockInfoStack = new Stack<>(); private IsolationAnalyzer(CompilerContext context) { @@ -287,12 +287,40 @@ public void visit(BLangPackage pkgNode) { pkgNode.completedPhases.add(CompilerPhase.ISOLATION_ANALYZE); - for (List referenceList : references.values()) { - if (referenceList.size() == 1) { + for (UniqueInitAndReferenceInfo varInfo : uniqueInitAndReferenceInfo.values()) { + int totalRefCount = varInfo.totalRefCount; + List refsOfVarExpectedToBeUnique = varInfo.refsOfVarExpectedToBeUnique; + + if (totalRefCount == 0 || refsOfVarExpectedToBeUnique.isEmpty()) { + continue; + } + + if (!varInfo.uniqueInitExpr) { + for (BLangSimpleVarRef varRef : refsOfVarExpectedToBeUnique) { + dlog.error(varRef.pos, + DiagnosticCode.INVALID_NON_UNIQUE_EXPRESSION_AS_INITIAL_VALUE_IN_ISOLATED_OBJECT); + } continue; } - for (BLangSimpleVarRef varRef : referenceList) { + boolean nonUniqueInitExprDueToVarRefs = false; + + if (totalRefCount == 1) { + for (BLangSimpleVarRef varRef : varInfo.refsUsedInInitExpr) { + UniqueInitAndReferenceInfo varRefInfo = uniqueInitAndReferenceInfo.get(varRef.symbol); + + if (varRefInfo.totalRefCount > 1) { + nonUniqueInitExprDueToVarRefs = true; + break; + } + } + + if (!nonUniqueInitExprDueToVarRefs) { + continue; + } + } + + for (BLangSimpleVarRef varRef : refsOfVarExpectedToBeUnique) { dlog.error(varRef.pos, DiagnosticCode.INVALID_NON_UNIQUE_EXPRESSION_AS_INITIAL_VALUE_IN_ISOLATED_OBJECT); } @@ -374,12 +402,13 @@ public void visit(BLangSimpleVariable varNode) { analyzeNode(typeNode, env); } - int flags = varNode.symbol.flags; + BVarSymbol symbol = varNode.symbol; + int flags = symbol.flags; BLangExpression expr = varNode.expr; BType fieldType = varNode.type; - if (isIsolatedClassField() && isMutableField(varNode.symbol, fieldType)) { + if (isIsolatedClassField() && isMutableField(symbol, fieldType)) { if (!Symbols.isFlagOn(flags, Flags.PRIVATE)) { dlog.error(varNode.pos, DiagnosticCode.INVALID_NON_PRIVATE_MUTABLE_FIELD_IN_ISOLATED_OBJECT); } @@ -395,6 +424,17 @@ public void visit(BLangSimpleVariable varNode) { analyzeNode(expr, env); + UniqueInitAndReferenceInfo varInfo = this.uniqueInitAndReferenceInfo.get(symbol); + + ArrayList refsUsedInInitExpr = new ArrayList<>(); + boolean uniqueInitExpr = isReferenceOrValidUniqueExpression(expr, refsUsedInInitExpr, false); + if (varInfo == null) { + uniqueInitAndReferenceInfo.put(symbol, new UniqueInitAndReferenceInfo(uniqueInitExpr, refsUsedInInitExpr)); + } else { + varInfo.uniqueInitExpr = uniqueInitExpr; + varInfo.refsUsedInInitExpr = refsUsedInInitExpr; + } + if (Symbols.isFlagOn(flags, Flags.WORKER)) { inferredIsolated = false; @@ -848,6 +888,15 @@ public void visit(BLangSimpleVarRef varRefExpr) { return; } + UniqueInitAndReferenceInfo varInfo = uniqueInitAndReferenceInfo.get(symbol); + + if (varInfo == null) { + uniqueInitAndReferenceInfo.put(symbol, new UniqueInitAndReferenceInfo()); + } else { + varInfo.totalRefCount++; + } + + if (inLockStatement && isInIsolatedObjectMethod(env, true)) { if ((!varRefExpr.lhsVar || varRefExpr.parent.getKind() != NodeKind.ASSIGNMENT) && !Names.SELF.value.equals(varRefExpr.variableName.value) && isInvalidCopyIn(varRefExpr, env)) { @@ -1609,7 +1658,7 @@ private void validateIsolatedObjectFieldInitialValue(BType fieldType, BLangExpre return; } - validateReferenceOrUniqueExpression(expression); + isReferenceOrValidUniqueExpression(expression); } private boolean isIsolatedObjectTypes(BType type) { @@ -1631,9 +1680,15 @@ private boolean isIsolatedObjectTypes(BType type) { return true; } - private void validateReferenceOrUniqueExpression(BLangExpression expression) { + private void isReferenceOrValidUniqueExpression(BLangExpression expression) { + isReferenceOrValidUniqueExpression(expression, null, true); + } + + private boolean isReferenceOrValidUniqueExpression(BLangExpression expression, + List refList, + boolean logErrors) { if (Symbols.isFlagOn(expression.type.flags, Flags.READONLY)) { - return; + return true; } switch (expression.getKind()) { @@ -1641,22 +1696,34 @@ private void validateReferenceOrUniqueExpression(BLangExpression expression) { BLangSimpleVarRef varRef = (BLangSimpleVarRef) expression; BSymbol symbol = varRef.symbol; - List varRefList = references.get(symbol); + if (!logErrors) { + refList.add(varRef); + } + + UniqueInitAndReferenceInfo varInfo = this.uniqueInitAndReferenceInfo.get(symbol); - if (varRefList == null) { - references.put(symbol, new ArrayList() {{ add(varRef); }}); - return; + if (varInfo == null) { + uniqueInitAndReferenceInfo.put(symbol, new UniqueInitAndReferenceInfo(varRef)); + return true; } - varRefList.add(varRef); - return; + + if (logErrors) { + varInfo.refsOfVarExpectedToBeUnique.add(varRef); + } + + return true; case LITERAL: case NUMERIC_LITERAL: - return; + return true; case LIST_CONSTRUCTOR_EXPR: for (BLangExpression expr : ((BLangListConstructorExpr) expression).exprs) { - validateReferenceOrUniqueExpression(expr); + if (isReferenceOrValidUniqueExpression(expr, refList, logErrors) || logErrors) { + continue; + } + + return false; } - return; + return true; case RECORD_LITERAL_EXPR: for (RecordLiteralNode.RecordField field : ((BLangRecordLiteral) expression).fields) { if (field.isKeyValueField()) { @@ -1665,54 +1732,84 @@ private void validateReferenceOrUniqueExpression(BLangExpression expression) { BLangRecordLiteral.BLangRecordKey key = keyValueField.key; if (key.computedKey) { - validateReferenceOrUniqueExpression(key.expr); + if (!isReferenceOrValidUniqueExpression(key.expr, refList, logErrors) && !logErrors) { + return false; + } } - validateReferenceOrUniqueExpression(keyValueField.valueExpr); - continue; + if (isReferenceOrValidUniqueExpression(keyValueField.valueExpr, refList, logErrors) || + logErrors) { + continue; + } + return false; } if (field.getKind() == NodeKind.RECORD_LITERAL_SPREAD_OP) { - validateReferenceOrUniqueExpression( - ((BLangRecordLiteral.BLangRecordSpreadOperatorField) field).expr); - continue; + if (isReferenceOrValidUniqueExpression( + ((BLangRecordLiteral.BLangRecordSpreadOperatorField) field).expr, refList, logErrors) || + logErrors) { + continue; + } + return false; } - validateReferenceOrUniqueExpression((BLangRecordLiteral.BLangRecordVarNameField) field); + if (isReferenceOrValidUniqueExpression((BLangRecordLiteral.BLangRecordVarNameField) field, refList, + logErrors) || logErrors) { + continue; + } + return false; } - return; + return true; case INVOCATION: BLangInvocation invocation = (BLangInvocation) expression; if (Symbols.isFlagOn(invocation.type.flags, Flags.READONLY)) { - return; + return true; } if (isCloneOrCloneReadOnlyInvocation(invocation)) { - return; + return true; } if (isIsolated(invocation.symbol.type.flags)) { for (BLangExpression requiredArg : invocation.requiredArgs) { if (requiredArg.getKind() == NodeKind.NAMED_ARGS_EXPR) { - validateReferenceOrUniqueExpression(((BLangNamedArgsExpression) requiredArg).expr); - continue; + if (isReferenceOrValidUniqueExpression(((BLangNamedArgsExpression) requiredArg).expr, + refList, logErrors) || logErrors) { + continue; + } + return false; } - validateReferenceOrUniqueExpression(requiredArg); + if (isReferenceOrValidUniqueExpression(requiredArg, refList, logErrors) || logErrors) { + continue; + } + return false; } for (BLangExpression restArg : invocation.restArgs) { if (restArg.getKind() == NodeKind.REST_ARGS_EXPR) { - validateReferenceOrUniqueExpression(((BLangRestArgsExpression) restArg).expr); - continue; + if (isReferenceOrValidUniqueExpression(((BLangRestArgsExpression) restArg).expr, refList, + logErrors) || logErrors) { + continue; + } + return false; } - validateReferenceOrUniqueExpression(restArg); + if (isReferenceOrValidUniqueExpression(restArg, refList, logErrors) || logErrors) { + continue; + } + return false; } } } - dlog.error(expression.pos, DiagnosticCode.INVALID_NON_UNIQUE_EXPRESSION_AS_INITIAL_VALUE_IN_ISOLATED_OBJECT); + + if (logErrors) { + dlog.error(expression.pos, + DiagnosticCode.INVALID_NON_UNIQUE_EXPRESSION_AS_INITIAL_VALUE_IN_ISOLATED_OBJECT); + } + + return false; } private boolean isCloneOrCloneReadOnlyInvocation(BLangInvocation invocation) { @@ -1727,6 +1824,13 @@ private boolean isCloneOrCloneReadOnlyInvocation(BLangInvocation invocation) { } private boolean isInvalidCopyingOfMutableValueInIsolatedObject(BLangExpression expression, boolean copyOut) { + return isInvalidCopyingOfMutableValueInIsolatedObject( + expression, copyOut, expression.getKind() == NodeKind.SIMPLE_VARIABLE_REF && + Names.SELF.value.equals(((BLangSimpleVarRef) expression).variableName.value)); + } + + private boolean isInvalidCopyingOfMutableValueInIsolatedObject(BLangExpression expression, boolean copyOut, + boolean invokedOnSelf) { BType type = expression.type; if (Symbols.isFlagOn(type.flags, Flags.READONLY)) { return false; @@ -1766,19 +1870,22 @@ private boolean isInvalidCopyingOfMutableValueInIsolatedObject(BLangExpression e switch (kind) { case FIELD_BASED_ACCESS_EXPR: case INDEX_BASED_ACCESS_EXPR: - return isInvalidCopyingOfMutableValueInIsolatedObject(parentExpression, copyOut); + return isInvalidCopyingOfMutableValueInIsolatedObject(parentExpression, copyOut, invokedOnSelf); case INVOCATION: BLangInvocation invocation = (BLangInvocation) parentExpression; BLangExpression calledOnExpr = invocation.expr; - if (calledOnExpr == expression && !isIsolatedObjectTypes(type)) { + if (calledOnExpr == expression && // if this is a method-call, do additional checks + !Symbols.isFlagOn(calledOnExpr.type.flags, Flags.READONLY) && + (!invokedOnSelf || invocation.type.tag != TypeTags.NIL) && + !isIsolatedObjectTypes(type)) { return true; } return !isIsolated(invocation.symbol.flags) && !isCloneOrCloneReadOnlyInvocation(invocation); case GROUP_EXPR: return isInvalidCopyingOfMutableValueInIsolatedObject(((BLangGroupExpr) parentExpression).expression, - copyOut); + copyOut, invokedOnSelf); } return !isIsolatedObjectTypes(type); @@ -1884,6 +1991,34 @@ private boolean isInvalidCopyIn(BLangSimpleVarRef varRefExpr, Name name, int sym return isInvalidCopyIn(varRefExpr, name, symTag, env.enclEnv); } + private static class UniqueInitAndReferenceInfo { + boolean uniqueInitExpr; + List refsUsedInInitExpr; + List refsOfVarExpectedToBeUnique; + int totalRefCount; + + private UniqueInitAndReferenceInfo(boolean uniqueInitExpr, List refsUsedInInitExpr) { + this.uniqueInitExpr = uniqueInitExpr; + this.refsUsedInInitExpr = refsUsedInInitExpr; + this.refsOfVarExpectedToBeUnique = new ArrayList<>(); + this.totalRefCount = 0; + } + + private UniqueInitAndReferenceInfo(BLangSimpleVarRef reference) { + this.uniqueInitExpr = false; + this.refsUsedInInitExpr = new ArrayList<>(); + this.refsOfVarExpectedToBeUnique = new ArrayList<>() {{ add(reference); }}; + this.totalRefCount = 0; + } + + private UniqueInitAndReferenceInfo() { + this.uniqueInitExpr = false; + this.refsUsedInInitExpr = new ArrayList<>(); + this.refsOfVarExpectedToBeUnique = new ArrayList<>(); + this.totalRefCount = 1; + } + } + /** * For isolated objects, invalid copy ins and non-isolated invocations should result in compilation errors if * they happen in a lock statement accessing a mutable field of the object. This class holds potentially From 24d334fe92f0e9d3392f62172d73d2647f227add Mon Sep 17 00:00:00 2001 From: MaryamZi Date: Wed, 14 Oct 2020 23:52:42 +0530 Subject: [PATCH 11/19] Add initial set of negative tests --- .../test/isolation/IsolatedObjectTest.java | 82 +++++++++++ .../isolated_objects_isolation_negative.bal | 139 ++++++++++++++++++ .../isolated_objects_semantic_negative.bal | 84 +++++++++++ 3 files changed, 305 insertions(+) create mode 100644 tests/jballerina-unit-test/src/test/java/org/ballerinalang/test/isolation/IsolatedObjectTest.java create mode 100644 tests/jballerina-unit-test/src/test/resources/test-src/isolated-objects/isolated_objects_isolation_negative.bal create mode 100644 tests/jballerina-unit-test/src/test/resources/test-src/isolated-objects/isolated_objects_semantic_negative.bal diff --git a/tests/jballerina-unit-test/src/test/java/org/ballerinalang/test/isolation/IsolatedObjectTest.java b/tests/jballerina-unit-test/src/test/java/org/ballerinalang/test/isolation/IsolatedObjectTest.java new file mode 100644 index 000000000000..dd72ac8a4d64 --- /dev/null +++ b/tests/jballerina-unit-test/src/test/java/org/ballerinalang/test/isolation/IsolatedObjectTest.java @@ -0,0 +1,82 @@ +/* + * Copyright (c) 2020, 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.isolation; + +import org.ballerinalang.test.util.BCompileUtil; +import org.ballerinalang.test.util.CompileResult; +import org.testng.Assert; +import org.testng.annotations.Test; + +import static org.ballerinalang.test.util.BAssertUtil.validateError; + +/** + * Test cases related to isolated objects. + * + * @since Swan Lake + */ +public class IsolatedObjectTest { + + @Test + public void testIsolatedObjectSemanticNegative() { + CompileResult result = BCompileUtil.compile( + "test-src/isolated-objects/isolated_objects_semantic_negative.bal"); + int i = 0; + validateError(result, i++, "incompatible types: expected 'Foo', found 'isolated object { }'", 69, 14); + validateError(result, i++, "incompatible types: expected 'Baz', found 'isolated object { }'", 70, 14); + validateError(result, i++, "incompatible types: expected 'Bar', found 'Baz'", 78, 14); + validateError(result, i++, "incompatible types: expected 'Baz', found 'Bar'", 79, 14); + validateError(result, i++, "incompatible types: expected 'Qux', found 'Quux'", 81, 14); + validateError(result, i++, "incompatible types: expected 'Foo', found 'Quux'", 83, 19); + Assert.assertEquals(result.getErrorCount(), i); + } + + @Test + public void testIsolatedObjectIsolationNegative() { + CompileResult result = BCompileUtil.compile( + "test-src/isolated-objects/isolated_objects_isolation_negative.bal"); + int i = 0; + validateError(result, i++, "invalid non-private mutable field in an 'isolated' object", 18, 5); + validateError(result, i++, "invalid non-private mutable field in an 'isolated' object", 19, 5); + validateError(result, i++, "invalid non-private mutable field in an 'isolated' object", 29, 5); + validateError(result, i++, "invalid non-private mutable field in an 'isolated' object", 30, 5); + validateError(result, i++, "invalid non-private mutable field in an 'isolated' object", 45, 6); + validateError(result, i++, "invalid non-private mutable field in an 'isolated' object", 45, 6); + validateError(result, i++, + "invalid access of a mutable field of an 'isolated' object outside a 'lock' statement", 62, 39); + validateError(result, i++, + "invalid access of a mutable field of an 'isolated' object outside a 'lock' statement", 65, 9); + validateError(result, i++, + "invalid access of a mutable field of an 'isolated' object outside a 'lock' statement", 72, 16); + validateError(result, i++, + "invalid access of a mutable field of an 'isolated' object outside a 'lock' statement", 82, 43); + validateError(result, i++, + "invalid access of a mutable field of an 'isolated' object outside a 'lock' statement", 85, 13); + validateError(result, i++, + "invalid access of a mutable field of an 'isolated' object outside a 'lock' statement", 92, 20); + validateError(result, i++, "invalid initial value expression: expected a unique expression", 109, 30); + validateError(result, i++, "invalid initial value expression: expected a unique expression", 115, 22); + validateError(result, i++, "invalid initial value expression: expected a unique expression", 117, 23); + validateError(result, i++, "invalid initial value expression: expected a unique expression", 117, 35); + validateError(result, i++, "invalid initial value expression: expected a unique expression", 122, 18); + validateError(result, i++, "invalid initial value expression: expected a unique expression", 128, 30); + validateError(result, i++, "invalid initial value expression: expected a unique expression", 128, 42); + validateError(result, i++, "invalid initial value expression: expected a unique expression", 129, 34); + validateError(result, i++, "invalid initial value expression: expected a unique expression", 136, 22); + Assert.assertEquals(result.getErrorCount(), i); + } +} diff --git a/tests/jballerina-unit-test/src/test/resources/test-src/isolated-objects/isolated_objects_isolation_negative.bal b/tests/jballerina-unit-test/src/test/resources/test-src/isolated-objects/isolated_objects_isolation_negative.bal new file mode 100644 index 000000000000..ab37a267e04f --- /dev/null +++ b/tests/jballerina-unit-test/src/test/resources/test-src/isolated-objects/isolated_objects_isolation_negative.bal @@ -0,0 +1,139 @@ +// Copyright (c) 2020 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. + +isolated class InvalidIsolatedClassWithNonPrivateMutableFields { + int a; + public map b; + private final string c = "invalid"; + + function init(int a, map b) { + self.a = a; + self.b = b.clone(); + } +} + +isolated object {} invalidIsolatedObjectConstructorWithNonPrivateMutableFields = object { + int a; + public map b; + private final string c = "invalid"; + + function init() { + self.a = 1; + self.b = {}; + } +}; + +type IsolatedObjectType isolated object { + int a; + string[] b; +}; + +isolated class InvalidIsolatedClassNotOverridingMutableFieldsInIncludedIsolatedObject { + *IsolatedObjectType; + + function init() { + self.a = 1; + self.b = []; + } +} + +isolated class InvalidIsolatedClassAccessingMutableFieldsOutsideLock { + final int a = 1; + private string b = "hello"; + private int[] c; + + function init(int[] c) { + self.c = c.clone(); + } + + function getB() returns string => self.b; + + function setB(string s) { + self.b = s; + } + + function updateAndGetC(int i) returns int[] { + lock { + self.c.push(i); // OK + } + return self.c; + } +} + +function testInvalidIsolatedObjectConstructorAccessingMutableFieldsOutsideLock() { + isolated object {} invalidIsolatedObjectConstructorAccessingMutableFieldsOutsideLock = object { + final int a = 1; + private string b = "hello"; + private int[] c = []; + + function getB() returns string => self.b; + + function setB(string s) { + self.b = s; + } + + function updateAndGetC(int i) returns int[] { + lock { + self.c.push(i); // OK + } + return self.c; + } + }; +} + +int[] globIntArr = [1200, 12, 12345]; +int[] globIntArrCopy = globIntArr; +int[] globIntArrCopy2 = globIntArrCopy; + +map globBoolMap = {a: true, b: false}; + +function accessGlobBool() { + _ = globBoolMap; +} + +isolated class InvalidIsolatedClassWithNonUniqueInitializerExprs { + private int[][] a; + private map b = globBoolMap; + private record {} c; + final string d = "str"; + + function init(int[][]? a) { + if a is int[][] { + self.a = a; + } else { + self.a = [globIntArr, globIntArr]; + } + + record {} rec = {"a": 1, "b": 2.0}; + anydata ad = rec; + self.c = rec; + } +} + +function testInvalidIsolatedObjectWithNonUniqueInitializerExprs() { + isolated object {} invalidIsolatedObjectWithNonUniqueInitializerExprs = object { + private int[][] a = [globIntArr, globIntArr]; + private map b = globBoolMap; + private record {} c; + final string d = "str"; + + function init() { + record {} rec = {"a": 1, "b": 2.0}; + anydata ad = rec; + self.c = rec; + } + }; +} diff --git a/tests/jballerina-unit-test/src/test/resources/test-src/isolated-objects/isolated_objects_semantic_negative.bal b/tests/jballerina-unit-test/src/test/resources/test-src/isolated-objects/isolated_objects_semantic_negative.bal new file mode 100644 index 000000000000..afce25a32cbd --- /dev/null +++ b/tests/jballerina-unit-test/src/test/resources/test-src/isolated-objects/isolated_objects_semantic_negative.bal @@ -0,0 +1,84 @@ +// Copyright (c) 2020 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. + +type Foo isolated object { + int a; + string[] b; +}; + +isolated class Bar { + public final int[] & readonly a = [1, 2]; + final Foo & readonly b; + private int c; + private boolean[] d; + + isolated function init(Foo & readonly b, int c, boolean[] d) { + self.b = b; + self.c = c; + self.d = d; + } +} + +isolated class Baz { + public final int[] & readonly a = [1, 2]; + final Foo & readonly b = object { + final int a = 1; + final string[] & readonly b = []; + }; + public final int c = 1; + final readonly & boolean[] d = []; +} + +isolated class Qux { + public final int[] & readonly a = [1, 2, 3, 4]; + final Foo & readonly b = object { + final int a = 1; + final string[] & readonly b = []; + }; +} + +class Quux { + public final int[] & readonly a = [1, 2, 3, 4]; + final Foo & readonly b = object { + final int a = 1; + final string[] & readonly b = []; + }; +} + +type Quuz record { + Foo f; +}; + +function testIsolatedFunctionSemanticsNegative() { + isolated object {} x = object { + final int i = 1; + }; + Foo v1 = x; + Baz v2 = x; + + Foo & readonly readOnlyFoo = object { + final int a = 1; + final string[] & readonly b = []; + }; + Bar bar = new (readOnlyFoo, 1234, [true, false]); + + Bar v3 = new Baz(); + Baz v4 = bar; + + Qux v5 = new Quux(); + + Quuz v6 = {f: new Quux()}; +} From 40f0935774d107a20ef8037c7023ffb0edb179bc Mon Sep 17 00:00:00 2001 From: MaryamZi Date: Thu, 15 Oct 2020 22:36:10 +0530 Subject: [PATCH 12/19] Fix issues in copy in/out analysis for self --- .../semantics/analyzer/IsolationAnalyzer.java | 33 +++++++++---------- 1 file changed, 15 insertions(+), 18 deletions(-) diff --git a/compiler/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/semantics/analyzer/IsolationAnalyzer.java b/compiler/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/semantics/analyzer/IsolationAnalyzer.java index d79199ccb220..f44770e3a241 100644 --- a/compiler/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/semantics/analyzer/IsolationAnalyzer.java +++ b/compiler/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/semantics/analyzer/IsolationAnalyzer.java @@ -1867,28 +1867,25 @@ private boolean isInvalidCopyingOfMutableValueInIsolatedObject(BLangExpression e BLangExpression parentExpression = (BLangExpression) parent; - switch (kind) { - case FIELD_BASED_ACCESS_EXPR: - case INDEX_BASED_ACCESS_EXPR: - return isInvalidCopyingOfMutableValueInIsolatedObject(parentExpression, copyOut, invokedOnSelf); - case INVOCATION: - BLangInvocation invocation = (BLangInvocation) parentExpression; - BLangExpression calledOnExpr = invocation.expr; + if (kind != NodeKind.INVOCATION) { + return !isIsolatedObjectTypes(type) && + isInvalidCopyingOfMutableValueInIsolatedObject(parentExpression, copyOut, invokedOnSelf); + } - if (calledOnExpr == expression && // if this is a method-call, do additional checks - !Symbols.isFlagOn(calledOnExpr.type.flags, Flags.READONLY) && - (!invokedOnSelf || invocation.type.tag != TypeTags.NIL) && - !isIsolatedObjectTypes(type)) { - return true; - } + BLangInvocation invocation = (BLangInvocation) parentExpression; + BLangExpression calledOnExpr = invocation.expr; - return !isIsolated(invocation.symbol.flags) && !isCloneOrCloneReadOnlyInvocation(invocation); - case GROUP_EXPR: - return isInvalidCopyingOfMutableValueInIsolatedObject(((BLangGroupExpr) parentExpression).expression, - copyOut, invokedOnSelf); + if (calledOnExpr == expression) { // if this is a method-call, do additional checks + if (invokedOnSelf && copyOut) { + return invocation.type.tag != TypeTags.NIL; + } + + if (!Symbols.isFlagOn(calledOnExpr.type.flags, Flags.READONLY) && !isIsolatedObjectTypes(type)) { + return true; + } } - return !isIsolatedObjectTypes(type); + return !isIsolated(invocation.symbol.flags) || !isCloneOrCloneReadOnlyInvocation(invocation); } private boolean hasRefDefinedOutsideLock(BLangExpression variableReference) { From d2225dd31d7711b3baa2bcd2ec4805d748cce09c Mon Sep 17 00:00:00 2001 From: MaryamZi Date: Fri, 16 Oct 2020 00:17:41 +0530 Subject: [PATCH 13/19] Add tests for invalid copy-in --- .../test/isolation/IsolatedObjectTest.java | 16 +++++ .../isolated_objects_isolation_negative.bal | 70 +++++++++++++++++++ 2 files changed, 86 insertions(+) diff --git a/tests/jballerina-unit-test/src/test/java/org/ballerinalang/test/isolation/IsolatedObjectTest.java b/tests/jballerina-unit-test/src/test/java/org/ballerinalang/test/isolation/IsolatedObjectTest.java index dd72ac8a4d64..0e5977705ae9 100644 --- a/tests/jballerina-unit-test/src/test/java/org/ballerinalang/test/isolation/IsolatedObjectTest.java +++ b/tests/jballerina-unit-test/src/test/java/org/ballerinalang/test/isolation/IsolatedObjectTest.java @@ -77,6 +77,22 @@ public void testIsolatedObjectIsolationNegative() { validateError(result, i++, "invalid initial value expression: expected a unique expression", 128, 42); validateError(result, i++, "invalid initial value expression: expected a unique expression", 129, 34); validateError(result, i++, "invalid initial value expression: expected a unique expression", 136, 22); + validateError(result, i++, "invalid attempt to copy a mutable value into an 'isolated' object", 157, 25); + validateError(result, i++, "invalid attempt to copy a mutable value into an 'isolated' object", 158, 25); + validateError(result, i++, "invalid attempt to copy a mutable value into an 'isolated' object", 159, 23); + validateError(result, i++, "invalid attempt to copy a mutable value into an 'isolated' object", 168, 36); + validateError(result, i++, "invalid attempt to copy out a mutable value from an 'isolated' object", 169, 23); + validateError(result, i++, "invalid attempt to copy a mutable value into an 'isolated' object", 172, 25); + validateError(result, i++, "invalid attempt to copy a mutable value into an 'isolated' object", 173, 25); + validateError(result, i++, "invalid attempt to copy a mutable value into an 'isolated' object", 174, 23); + validateError(result, i++, "invalid attempt to copy a mutable value into an 'isolated' object", 189, 25); + validateError(result, i++, "invalid attempt to copy a mutable value into an 'isolated' object", 190, 25); + validateError(result, i++, "invalid attempt to copy a mutable value into an 'isolated' object", 191, 23); + validateError(result, i++, "invalid attempt to copy a mutable value into an 'isolated' object", 200, 36); + validateError(result, i++, "invalid attempt to copy out a mutable value from an 'isolated' object", 201, 23); + validateError(result, i++, "invalid attempt to copy a mutable value into an 'isolated' object", 204, 25); + validateError(result, i++, "invalid attempt to copy a mutable value into an 'isolated' object", 205, 25); + validateError(result, i++, "invalid attempt to copy a mutable value into an 'isolated' object", 206, 23); Assert.assertEquals(result.getErrorCount(), i); } } diff --git a/tests/jballerina-unit-test/src/test/resources/test-src/isolated-objects/isolated_objects_isolation_negative.bal b/tests/jballerina-unit-test/src/test/resources/test-src/isolated-objects/isolated_objects_isolation_negative.bal index ab37a267e04f..d69d719dcfa1 100644 --- a/tests/jballerina-unit-test/src/test/resources/test-src/isolated-objects/isolated_objects_isolation_negative.bal +++ b/tests/jballerina-unit-test/src/test/resources/test-src/isolated-objects/isolated_objects_isolation_negative.bal @@ -137,3 +137,73 @@ function testInvalidIsolatedObjectWithNonUniqueInitializerExprs() { } }; } + +isolated class InvalidIsolatedClassWithInvalidCopyIn { + public final record {} & readonly a; + private int b; + private map[] c; + + function init(record {} & readonly a, int b, map[] c) { + self.a = a; + self.b = b; + self.c = c.clone(); + } + + function invalidCopyInOne(map boolMap) { + map bm1 = {}; + lock { + map bm2 = {a: true, b: false}; + + self.c[0] = globBoolMap; + self.c.push(boolMap); + self.c = [bm1, bm2]; + } + } + + isolated function invalidCopyInTwo(map boolMap) { + map bm1 = {}; + lock { + map bm2 = {}; + lock { + map bm3 = boolMap; + bm2 = bm3; + } + + self.c.push(boolMap); + self.c[0] = boolMap; + self.c = [bm1, bm2]; + } + } +} + +isolated object {} invalidIsolatedObjectWithInvalidCopyIn = object { + public final record {} & readonly a = {"type": "final"}; + private int b = 0; + private map[] c = []; + + isolated function invalidCopyInOne(map boolMap) { + map bm1 = {}; + lock { + map bm2 = {a: true, b: false}; + + self.c[0] = boolMap; + self.c.push(boolMap); + self.c = [bm1, bm2]; + } + } + + function invalidCopyInTwo(map boolMap) { + map bm1 = {}; + lock { + map bm2 = {}; + lock { + map bm3 = boolMap; + bm2 = bm3; + } + + self.c.push(boolMap); + self.c[0] = globBoolMap; + self.c = [bm1, bm2]; + } + } +}; From f95afc5e848ab52fd39d636481e527ccf4e5b38a Mon Sep 17 00:00:00 2001 From: MaryamZi Date: Fri, 16 Oct 2020 17:03:25 +0530 Subject: [PATCH 14/19] Fix issues in copy in analysis and add negative tests --- .../semantics/analyzer/IsolationAnalyzer.java | 36 ++++++---- .../test/isolation/IsolatedObjectTest.java | 12 ++++ .../isolated_objects_isolation_negative.bal | 66 +++++++++++++++++++ 3 files changed, 102 insertions(+), 12 deletions(-) diff --git a/compiler/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/semantics/analyzer/IsolationAnalyzer.java b/compiler/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/semantics/analyzer/IsolationAnalyzer.java index f44770e3a241..b5592483d014 100644 --- a/compiler/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/semantics/analyzer/IsolationAnalyzer.java +++ b/compiler/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/semantics/analyzer/IsolationAnalyzer.java @@ -1823,10 +1823,9 @@ private boolean isCloneOrCloneReadOnlyInvocation(BLangInvocation invocation) { (methodName.equals(CLONE_LANG_LIB_METHOD) || methodName.equals(CLONE_READONLY_LANG_LIB_METHOD)); } - private boolean isInvalidCopyingOfMutableValueInIsolatedObject(BLangExpression expression, boolean copyOut) { + private boolean isInvalidCopyingOfMutableValueInIsolatedObject(BLangSimpleVarRef expression, boolean copyOut) { return isInvalidCopyingOfMutableValueInIsolatedObject( - expression, copyOut, expression.getKind() == NodeKind.SIMPLE_VARIABLE_REF && - Names.SELF.value.equals(((BLangSimpleVarRef) expression).variableName.value)); + expression, copyOut, Names.SELF.value.equals(expression.variableName.value)); } private boolean isInvalidCopyingOfMutableValueInIsolatedObject(BLangExpression expression, boolean copyOut, @@ -1840,6 +1839,15 @@ private boolean isInvalidCopyingOfMutableValueInIsolatedObject(BLangExpression e NodeKind kind = parent.getKind(); if (!(parent instanceof BLangExpression)) { + if (isIsolatedObjectTypes(type)) { + return false; + } + + if (expression.getKind() == NodeKind.INVOCATION && + isCloneOrCloneReadOnlyInvocation(((BLangInvocation) expression))) { + return false; + } + if (!copyOut) { return true; } @@ -1868,24 +1876,27 @@ private boolean isInvalidCopyingOfMutableValueInIsolatedObject(BLangExpression e BLangExpression parentExpression = (BLangExpression) parent; if (kind != NodeKind.INVOCATION) { - return !isIsolatedObjectTypes(type) && - isInvalidCopyingOfMutableValueInIsolatedObject(parentExpression, copyOut, invokedOnSelf); + return isInvalidCopyingOfMutableValueInIsolatedObject(parentExpression, copyOut, invokedOnSelf); } BLangInvocation invocation = (BLangInvocation) parentExpression; BLangExpression calledOnExpr = invocation.expr; - if (calledOnExpr == expression) { // if this is a method-call, do additional checks - if (invokedOnSelf && copyOut) { - return invocation.type.tag != TypeTags.NIL; + if (calledOnExpr == expression) { + // If this is the analysis of the called-on expression of a method call, do some additional checks. + if (invocation.type.tag == TypeTags.NIL) { + return false; } - if (!Symbols.isFlagOn(calledOnExpr.type.flags, Flags.READONLY) && !isIsolatedObjectTypes(type)) { - return true; + if (invokedOnSelf && !copyOut) { + return false; } + + return isInvalidCopyingOfMutableValueInIsolatedObject(parentExpression, copyOut, invokedOnSelf); } - return !isIsolated(invocation.symbol.flags) || !isCloneOrCloneReadOnlyInvocation(invocation); + // `expression` is an argument to a function + return !isCloneOrCloneReadOnlyInvocation(invocation); } private boolean hasRefDefinedOutsideLock(BLangExpression variableReference) { @@ -1938,7 +1949,8 @@ private boolean hasRefDefinedOutsideLock(BLangExpression variableReference) { } private boolean isDefinedOutsideLock(Name name, int symTag, SymbolEnv currentEnv) { - if (symResolver.lookupSymbolInGivenScope(currentEnv, name, symTag) != symTable.notFoundSymbol) { + if (Names.IGNORE == name || + symResolver.lookupSymbolInGivenScope(currentEnv, name, symTag) != symTable.notFoundSymbol) { return false; } diff --git a/tests/jballerina-unit-test/src/test/java/org/ballerinalang/test/isolation/IsolatedObjectTest.java b/tests/jballerina-unit-test/src/test/java/org/ballerinalang/test/isolation/IsolatedObjectTest.java index 0e5977705ae9..74533a40719d 100644 --- a/tests/jballerina-unit-test/src/test/java/org/ballerinalang/test/isolation/IsolatedObjectTest.java +++ b/tests/jballerina-unit-test/src/test/java/org/ballerinalang/test/isolation/IsolatedObjectTest.java @@ -93,6 +93,18 @@ public void testIsolatedObjectIsolationNegative() { validateError(result, i++, "invalid attempt to copy a mutable value into an 'isolated' object", 204, 25); validateError(result, i++, "invalid attempt to copy a mutable value into an 'isolated' object", 205, 25); validateError(result, i++, "invalid attempt to copy a mutable value into an 'isolated' object", 206, 23); + validateError(result, i++, "invalid attempt to copy out a mutable value from an 'isolated' object", 225, 19); + validateError(result, i++, "invalid attempt to copy out a mutable value from an 'isolated' object", 226, 27); + validateError(result, i++, "invalid attempt to copy out a mutable value from an 'isolated' object", 227, 20); + validateError(result, i++, "invalid attempt to copy out a mutable value from an 'isolated' object", 237, 23); + validateError(result, i++, "invalid attempt to copy out a mutable value from an 'isolated' object", 240, 19); + validateError(result, i++, "invalid attempt to copy out a mutable value from an 'isolated' object", 241, 19); + validateError(result, i++, "invalid attempt to copy out a mutable value from an 'isolated' object", 255, 23); + validateError(result, i++, "invalid attempt to copy out a mutable value from an 'isolated' object", 256, 23); + validateError(result, i++, "invalid attempt to copy out a mutable value from an 'isolated' object", 257, 24); + validateError(result, i++, "invalid attempt to copy out a mutable value from an 'isolated' object", 267, 27); + validateError(result, i++, "invalid attempt to copy out a mutable value from an 'isolated' object", 270, 31); + validateError(result, i++, "invalid attempt to copy out a mutable value from an 'isolated' object", 271, 23); Assert.assertEquals(result.getErrorCount(), i); } } diff --git a/tests/jballerina-unit-test/src/test/resources/test-src/isolated-objects/isolated_objects_isolation_negative.bal b/tests/jballerina-unit-test/src/test/resources/test-src/isolated-objects/isolated_objects_isolation_negative.bal index d69d719dcfa1..527cbeef5829 100644 --- a/tests/jballerina-unit-test/src/test/resources/test-src/isolated-objects/isolated_objects_isolation_negative.bal +++ b/tests/jballerina-unit-test/src/test/resources/test-src/isolated-objects/isolated_objects_isolation_negative.bal @@ -207,3 +207,69 @@ isolated object {} invalidIsolatedObjectWithInvalidCopyIn = object { } } }; + +isolated class InvalidIsolatedClassWithInvalidCopyOut { + public final record {} & readonly a = {"type": "final"}; + private int b = 1; + private map[] c; + + function init() { + self.c = []; + } + + function invalidCopyOutOne(map[] boolMaps) { + map[] bm1 = boolMaps; + lock { + map[] bm2 = [{a: true, b: false}]; + + bm1 = self.c; + globBoolMap = bm2[0]; + bm1 = [self.c[0]]; + } + } + + isolated function invalidCopyOutTwo(map[] boolMaps) { + map bm1 = {}; + lock { + map bm2 = {}; + lock { + map bm3 = boolMaps[0].clone(); + bm1 = bm3; + } + + bm1 = self.c[0]; + bm1 = bm2; + } + } +} + +function testInvalidIsolatedObjectWithInvalidCopyIn() { + isolated object {} invalidIsolatedObjectWithInvalidCopyIn = object { + private map[] c = []; + + isolated function invalidCopyOutOne(map[] boolMaps) { + map[] bm1 = boolMaps; + lock { + map[] bm2 = [{a: true, b: false}]; + + bm1 = self.c; + bm1 = bm2; + bm1 = [self.c[0]]; + } + } + + function invalidCopyOutTwo(map[] boolMaps) { + map bm1 = {}; + lock { + map bm2 = {}; + lock { + map bm3 = boolMaps[0].clone(); + bm1 = bm3; + } + + globBoolMap = self.c[0]; + bm1 = bm2; + } + } + }; +} From f8edfa8180a5de874be3a4c7f0d101b62cfe3a43 Mon Sep 17 00:00:00 2001 From: MaryamZi Date: Sat, 17 Oct 2020 23:53:15 +0530 Subject: [PATCH 15/19] Fix copy out analysis for return stmt and add more tests --- .../semantics/analyzer/IsolationAnalyzer.java | 2 + .../test/isolation/IsolatedObjectTest.java | 34 ++++++++++--- .../isolated-objects/isolated_objects.bal | 16 ++++++ .../isolated_objects_isolation_negative.bal | 50 +++++++++++++++++-- 4 files changed, 91 insertions(+), 11 deletions(-) create mode 100644 tests/jballerina-unit-test/src/test/resources/test-src/isolated-objects/isolated_objects.bal diff --git a/compiler/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/semantics/analyzer/IsolationAnalyzer.java b/compiler/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/semantics/analyzer/IsolationAnalyzer.java index b5592483d014..cd8d057348ed 100644 --- a/compiler/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/semantics/analyzer/IsolationAnalyzer.java +++ b/compiler/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/semantics/analyzer/IsolationAnalyzer.java @@ -1869,6 +1869,8 @@ private boolean isInvalidCopyingOfMutableValueInIsolatedObject(BLangExpression e return hasRefDefinedOutsideLock(((BLangTupleDestructure) parent).varRef); case ERROR_DESTRUCTURE: return hasRefDefinedOutsideLock(((BLangErrorDestructure) parent).varRef); + case RETURN: + return true; } return false; } diff --git a/tests/jballerina-unit-test/src/test/java/org/ballerinalang/test/isolation/IsolatedObjectTest.java b/tests/jballerina-unit-test/src/test/java/org/ballerinalang/test/isolation/IsolatedObjectTest.java index 74533a40719d..f7486106bef1 100644 --- a/tests/jballerina-unit-test/src/test/java/org/ballerinalang/test/isolation/IsolatedObjectTest.java +++ b/tests/jballerina-unit-test/src/test/java/org/ballerinalang/test/isolation/IsolatedObjectTest.java @@ -96,15 +96,35 @@ public void testIsolatedObjectIsolationNegative() { validateError(result, i++, "invalid attempt to copy out a mutable value from an 'isolated' object", 225, 19); validateError(result, i++, "invalid attempt to copy out a mutable value from an 'isolated' object", 226, 27); validateError(result, i++, "invalid attempt to copy out a mutable value from an 'isolated' object", 227, 20); - validateError(result, i++, "invalid attempt to copy out a mutable value from an 'isolated' object", 237, 23); - validateError(result, i++, "invalid attempt to copy out a mutable value from an 'isolated' object", 240, 19); + validateError(result, i++, "invalid attempt to copy out a mutable value from an 'isolated' object", 228, 20); + validateError(result, i++, "invalid attempt to copy out a mutable value from an 'isolated' object", 238, 23); validateError(result, i++, "invalid attempt to copy out a mutable value from an 'isolated' object", 241, 19); - validateError(result, i++, "invalid attempt to copy out a mutable value from an 'isolated' object", 255, 23); + validateError(result, i++, "invalid attempt to copy out a mutable value from an 'isolated' object", 242, 19); validateError(result, i++, "invalid attempt to copy out a mutable value from an 'isolated' object", 256, 23); - validateError(result, i++, "invalid attempt to copy out a mutable value from an 'isolated' object", 257, 24); - validateError(result, i++, "invalid attempt to copy out a mutable value from an 'isolated' object", 267, 27); - validateError(result, i++, "invalid attempt to copy out a mutable value from an 'isolated' object", 270, 31); - validateError(result, i++, "invalid attempt to copy out a mutable value from an 'isolated' object", 271, 23); + validateError(result, i++, "invalid attempt to copy out a mutable value from an 'isolated' object", 257, 23); + validateError(result, i++, "invalid attempt to copy out a mutable value from an 'isolated' object", 258, 24); + validateError(result, i++, "invalid attempt to copy out a mutable value from an 'isolated' object", 259, 24); + validateError(result, i++, "invalid attempt to copy out a mutable value from an 'isolated' object", 269, 27); + validateError(result, i++, "invalid attempt to copy out a mutable value from an 'isolated' object", 272, 31); + validateError(result, i++, "invalid attempt to copy out a mutable value from an 'isolated' object", 273, 23); + validateError(result, i++, "invalid invocation of a non-isolated function in a method accessing a mutable " + + "field of an 'isolated' object", 287, 20); + validateError(result, i++, "invalid invocation of a non-isolated function in a method accessing a mutable " + + "field of an 'isolated' object", 288, 20); + validateError(result, i++, "invalid invocation of a non-isolated function in a method accessing a mutable " + + "field of an 'isolated' object", 290, 17); + validateError(result, i++, "invalid invocation of a non-isolated function in a method accessing a mutable " + + "field of an 'isolated' object", 303, 20); + validateError(result, i++, "invalid invocation of a non-isolated function in a method accessing a mutable " + + "field of an 'isolated' object", 304, 20); + validateError(result, i++, "invalid invocation of a non-isolated function in a method accessing a mutable " + + "field of an 'isolated' object", 306, 17); Assert.assertEquals(result.getErrorCount(), i); } + + @Test + public void testIsolatedObjects() { + CompileResult compileResult = BCompileUtil.compile("test-src/isolated-objects/isolated_objects.bal"); + Assert.assertEquals(compileResult.getDiagnostics().length, 0); + } } diff --git a/tests/jballerina-unit-test/src/test/resources/test-src/isolated-objects/isolated_objects.bal b/tests/jballerina-unit-test/src/test/resources/test-src/isolated-objects/isolated_objects.bal new file mode 100644 index 000000000000..64523c0486da --- /dev/null +++ b/tests/jballerina-unit-test/src/test/resources/test-src/isolated-objects/isolated_objects.bal @@ -0,0 +1,16 @@ +// Copyright (c) 2020 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. + diff --git a/tests/jballerina-unit-test/src/test/resources/test-src/isolated-objects/isolated_objects_isolation_negative.bal b/tests/jballerina-unit-test/src/test/resources/test-src/isolated-objects/isolated_objects_isolation_negative.bal index 527cbeef5829..a909585064d1 100644 --- a/tests/jballerina-unit-test/src/test/resources/test-src/isolated-objects/isolated_objects_isolation_negative.bal +++ b/tests/jballerina-unit-test/src/test/resources/test-src/isolated-objects/isolated_objects_isolation_negative.bal @@ -217,7 +217,7 @@ isolated class InvalidIsolatedClassWithInvalidCopyOut { self.c = []; } - function invalidCopyOutOne(map[] boolMaps) { + function invalidCopyOutOne(map[] boolMaps) returns map[] { map[] bm1 = boolMaps; lock { map[] bm2 = [{a: true, b: false}]; @@ -225,6 +225,7 @@ isolated class InvalidIsolatedClassWithInvalidCopyOut { bm1 = self.c; globBoolMap = bm2[0]; bm1 = [self.c[0]]; + return self.c; } } @@ -243,11 +244,11 @@ isolated class InvalidIsolatedClassWithInvalidCopyOut { } } -function testInvalidIsolatedObjectWithInvalidCopyIn() { - isolated object {} invalidIsolatedObjectWithInvalidCopyIn = object { +function testInvalidIsolatedObjectWithInvalidCopyOut() { + isolated object {} invalidIsolatedObjectWithInvalidCopyOut = object { private map[] c = []; - isolated function invalidCopyOutOne(map[] boolMaps) { + isolated function invalidCopyOutOne(map[] boolMaps) returns map { map[] bm1 = boolMaps; lock { map[] bm2 = [{a: true, b: false}]; @@ -255,6 +256,7 @@ function testInvalidIsolatedObjectWithInvalidCopyIn() { bm1 = self.c; bm1 = bm2; bm1 = [self.c[0]]; + return self.c.pop(); } } @@ -273,3 +275,43 @@ function testInvalidIsolatedObjectWithInvalidCopyIn() { } }; } + +isolated class InvalidIsolatedClassWithNonIsolatedFunctionInvocation { + private int[] x = []; + + function testInvalidNonIsolatedInvocation() { + lock { + int[] a = self.x; + + IsolatedClass ic = new; + a[0] = ic.nonIsolatedFunc(); + a.push(ic.nonIsolatedFunc()); + + a = nonIsolatedFunc(); + } + } +} + +isolated object {} invalidIsolatedObjectWithNonIsolatedFunctionInvocation = object { + private int[] x = []; + + function testInvalidNonIsolatedInvocation() { + lock { + int[] a = self.x; + + IsolatedClass ic = new; + a[0] = ic.nonIsolatedFunc(); + a.push(ic.nonIsolatedFunc()); + + a = nonIsolatedFunc(); + } + } +}; + +isolated class IsolatedClass { + function nonIsolatedFunc() returns int => 1; +} + +function nonIsolatedFunc() returns int[] { + return [1, 2, 3]; +} From cc71335d166210aef69ec0e6646e507a1dc463ac Mon Sep 17 00:00:00 2001 From: MaryamZi Date: Mon, 19 Oct 2020 09:41:02 +0530 Subject: [PATCH 16/19] Add isolation negative test for object-constr with type inclusion --- .../test/isolation/IsolatedObjectTest.java | 106 +++++++++--------- .../isolated_objects_isolation_negative.bal | 7 ++ 2 files changed, 61 insertions(+), 52 deletions(-) diff --git a/tests/jballerina-unit-test/src/test/java/org/ballerinalang/test/isolation/IsolatedObjectTest.java b/tests/jballerina-unit-test/src/test/java/org/ballerinalang/test/isolation/IsolatedObjectTest.java index f7486106bef1..ef84caf7d996 100644 --- a/tests/jballerina-unit-test/src/test/java/org/ballerinalang/test/isolation/IsolatedObjectTest.java +++ b/tests/jballerina-unit-test/src/test/java/org/ballerinalang/test/isolation/IsolatedObjectTest.java @@ -27,7 +27,7 @@ /** * Test cases related to isolated objects. * - * @since Swan Lake + * @since 2.0.0 */ public class IsolatedObjectTest { @@ -56,69 +56,71 @@ public void testIsolatedObjectIsolationNegative() { validateError(result, i++, "invalid non-private mutable field in an 'isolated' object", 30, 5); validateError(result, i++, "invalid non-private mutable field in an 'isolated' object", 45, 6); validateError(result, i++, "invalid non-private mutable field in an 'isolated' object", 45, 6); + validateError(result, i++, "invalid non-private mutable field in an 'isolated' object", 53, 101); + validateError(result, i++, "invalid non-private mutable field in an 'isolated' object", 53, 101); validateError(result, i++, - "invalid access of a mutable field of an 'isolated' object outside a 'lock' statement", 62, 39); + "invalid access of a mutable field of an 'isolated' object outside a 'lock' statement", 69, 39); validateError(result, i++, - "invalid access of a mutable field of an 'isolated' object outside a 'lock' statement", 65, 9); + "invalid access of a mutable field of an 'isolated' object outside a 'lock' statement", 72, 9); validateError(result, i++, - "invalid access of a mutable field of an 'isolated' object outside a 'lock' statement", 72, 16); + "invalid access of a mutable field of an 'isolated' object outside a 'lock' statement", 79, 16); validateError(result, i++, - "invalid access of a mutable field of an 'isolated' object outside a 'lock' statement", 82, 43); + "invalid access of a mutable field of an 'isolated' object outside a 'lock' statement", 89, 43); validateError(result, i++, - "invalid access of a mutable field of an 'isolated' object outside a 'lock' statement", 85, 13); + "invalid access of a mutable field of an 'isolated' object outside a 'lock' statement", 92, 13); validateError(result, i++, - "invalid access of a mutable field of an 'isolated' object outside a 'lock' statement", 92, 20); - validateError(result, i++, "invalid initial value expression: expected a unique expression", 109, 30); - validateError(result, i++, "invalid initial value expression: expected a unique expression", 115, 22); - validateError(result, i++, "invalid initial value expression: expected a unique expression", 117, 23); - validateError(result, i++, "invalid initial value expression: expected a unique expression", 117, 35); - validateError(result, i++, "invalid initial value expression: expected a unique expression", 122, 18); - validateError(result, i++, "invalid initial value expression: expected a unique expression", 128, 30); - validateError(result, i++, "invalid initial value expression: expected a unique expression", 128, 42); - validateError(result, i++, "invalid initial value expression: expected a unique expression", 129, 34); - validateError(result, i++, "invalid initial value expression: expected a unique expression", 136, 22); - validateError(result, i++, "invalid attempt to copy a mutable value into an 'isolated' object", 157, 25); - validateError(result, i++, "invalid attempt to copy a mutable value into an 'isolated' object", 158, 25); - validateError(result, i++, "invalid attempt to copy a mutable value into an 'isolated' object", 159, 23); - validateError(result, i++, "invalid attempt to copy a mutable value into an 'isolated' object", 168, 36); - validateError(result, i++, "invalid attempt to copy out a mutable value from an 'isolated' object", 169, 23); - validateError(result, i++, "invalid attempt to copy a mutable value into an 'isolated' object", 172, 25); - validateError(result, i++, "invalid attempt to copy a mutable value into an 'isolated' object", 173, 25); - validateError(result, i++, "invalid attempt to copy a mutable value into an 'isolated' object", 174, 23); - validateError(result, i++, "invalid attempt to copy a mutable value into an 'isolated' object", 189, 25); - validateError(result, i++, "invalid attempt to copy a mutable value into an 'isolated' object", 190, 25); - validateError(result, i++, "invalid attempt to copy a mutable value into an 'isolated' object", 191, 23); - validateError(result, i++, "invalid attempt to copy a mutable value into an 'isolated' object", 200, 36); - validateError(result, i++, "invalid attempt to copy out a mutable value from an 'isolated' object", 201, 23); - validateError(result, i++, "invalid attempt to copy a mutable value into an 'isolated' object", 204, 25); - validateError(result, i++, "invalid attempt to copy a mutable value into an 'isolated' object", 205, 25); - validateError(result, i++, "invalid attempt to copy a mutable value into an 'isolated' object", 206, 23); - validateError(result, i++, "invalid attempt to copy out a mutable value from an 'isolated' object", 225, 19); - validateError(result, i++, "invalid attempt to copy out a mutable value from an 'isolated' object", 226, 27); - validateError(result, i++, "invalid attempt to copy out a mutable value from an 'isolated' object", 227, 20); - validateError(result, i++, "invalid attempt to copy out a mutable value from an 'isolated' object", 228, 20); - validateError(result, i++, "invalid attempt to copy out a mutable value from an 'isolated' object", 238, 23); - validateError(result, i++, "invalid attempt to copy out a mutable value from an 'isolated' object", 241, 19); - validateError(result, i++, "invalid attempt to copy out a mutable value from an 'isolated' object", 242, 19); - validateError(result, i++, "invalid attempt to copy out a mutable value from an 'isolated' object", 256, 23); - validateError(result, i++, "invalid attempt to copy out a mutable value from an 'isolated' object", 257, 23); - validateError(result, i++, "invalid attempt to copy out a mutable value from an 'isolated' object", 258, 24); - validateError(result, i++, "invalid attempt to copy out a mutable value from an 'isolated' object", 259, 24); - validateError(result, i++, "invalid attempt to copy out a mutable value from an 'isolated' object", 269, 27); - validateError(result, i++, "invalid attempt to copy out a mutable value from an 'isolated' object", 272, 31); - validateError(result, i++, "invalid attempt to copy out a mutable value from an 'isolated' object", 273, 23); + "invalid access of a mutable field of an 'isolated' object outside a 'lock' statement", 99, 20); + validateError(result, i++, "invalid initial value expression: expected a unique expression", 116, 30); + validateError(result, i++, "invalid initial value expression: expected a unique expression", 122, 22); + validateError(result, i++, "invalid initial value expression: expected a unique expression", 124, 23); + validateError(result, i++, "invalid initial value expression: expected a unique expression", 124, 35); + validateError(result, i++, "invalid initial value expression: expected a unique expression", 129, 18); + validateError(result, i++, "invalid initial value expression: expected a unique expression", 135, 30); + validateError(result, i++, "invalid initial value expression: expected a unique expression", 135, 42); + validateError(result, i++, "invalid initial value expression: expected a unique expression", 136, 34); + validateError(result, i++, "invalid initial value expression: expected a unique expression", 143, 22); + validateError(result, i++, "invalid attempt to copy a mutable value into an 'isolated' object", 164, 25); + validateError(result, i++, "invalid attempt to copy a mutable value into an 'isolated' object", 165, 25); + validateError(result, i++, "invalid attempt to copy a mutable value into an 'isolated' object", 166, 23); + validateError(result, i++, "invalid attempt to copy a mutable value into an 'isolated' object", 175, 36); + validateError(result, i++, "invalid attempt to copy out a mutable value from an 'isolated' object", 176, 23); + validateError(result, i++, "invalid attempt to copy a mutable value into an 'isolated' object", 179, 25); + validateError(result, i++, "invalid attempt to copy a mutable value into an 'isolated' object", 180, 25); + validateError(result, i++, "invalid attempt to copy a mutable value into an 'isolated' object", 181, 23); + validateError(result, i++, "invalid attempt to copy a mutable value into an 'isolated' object", 196, 25); + validateError(result, i++, "invalid attempt to copy a mutable value into an 'isolated' object", 197, 25); + validateError(result, i++, "invalid attempt to copy a mutable value into an 'isolated' object", 198, 23); + validateError(result, i++, "invalid attempt to copy a mutable value into an 'isolated' object", 207, 36); + validateError(result, i++, "invalid attempt to copy out a mutable value from an 'isolated' object", 208, 23); + validateError(result, i++, "invalid attempt to copy a mutable value into an 'isolated' object", 211, 25); + validateError(result, i++, "invalid attempt to copy a mutable value into an 'isolated' object", 212, 25); + validateError(result, i++, "invalid attempt to copy a mutable value into an 'isolated' object", 213, 23); + validateError(result, i++, "invalid attempt to copy out a mutable value from an 'isolated' object", 232, 19); + validateError(result, i++, "invalid attempt to copy out a mutable value from an 'isolated' object", 233, 27); + validateError(result, i++, "invalid attempt to copy out a mutable value from an 'isolated' object", 234, 20); + validateError(result, i++, "invalid attempt to copy out a mutable value from an 'isolated' object", 235, 20); + validateError(result, i++, "invalid attempt to copy out a mutable value from an 'isolated' object", 245, 23); + validateError(result, i++, "invalid attempt to copy out a mutable value from an 'isolated' object", 248, 19); + validateError(result, i++, "invalid attempt to copy out a mutable value from an 'isolated' object", 249, 19); + validateError(result, i++, "invalid attempt to copy out a mutable value from an 'isolated' object", 263, 23); + validateError(result, i++, "invalid attempt to copy out a mutable value from an 'isolated' object", 264, 23); + validateError(result, i++, "invalid attempt to copy out a mutable value from an 'isolated' object", 265, 24); + validateError(result, i++, "invalid attempt to copy out a mutable value from an 'isolated' object", 266, 24); + validateError(result, i++, "invalid attempt to copy out a mutable value from an 'isolated' object", 276, 27); + validateError(result, i++, "invalid attempt to copy out a mutable value from an 'isolated' object", 279, 31); + validateError(result, i++, "invalid attempt to copy out a mutable value from an 'isolated' object", 280, 23); validateError(result, i++, "invalid invocation of a non-isolated function in a method accessing a mutable " + - "field of an 'isolated' object", 287, 20); + "field of an 'isolated' object", 294, 20); validateError(result, i++, "invalid invocation of a non-isolated function in a method accessing a mutable " + - "field of an 'isolated' object", 288, 20); + "field of an 'isolated' object", 295, 20); validateError(result, i++, "invalid invocation of a non-isolated function in a method accessing a mutable " + - "field of an 'isolated' object", 290, 17); + "field of an 'isolated' object", 297, 17); validateError(result, i++, "invalid invocation of a non-isolated function in a method accessing a mutable " + - "field of an 'isolated' object", 303, 20); + "field of an 'isolated' object", 310, 20); validateError(result, i++, "invalid invocation of a non-isolated function in a method accessing a mutable " + - "field of an 'isolated' object", 304, 20); + "field of an 'isolated' object", 311, 20); validateError(result, i++, "invalid invocation of a non-isolated function in a method accessing a mutable " + - "field of an 'isolated' object", 306, 17); + "field of an 'isolated' object", 313, 17); Assert.assertEquals(result.getErrorCount(), i); } diff --git a/tests/jballerina-unit-test/src/test/resources/test-src/isolated-objects/isolated_objects_isolation_negative.bal b/tests/jballerina-unit-test/src/test/resources/test-src/isolated-objects/isolated_objects_isolation_negative.bal index a909585064d1..0f6c6860bccc 100644 --- a/tests/jballerina-unit-test/src/test/resources/test-src/isolated-objects/isolated_objects_isolation_negative.bal +++ b/tests/jballerina-unit-test/src/test/resources/test-src/isolated-objects/isolated_objects_isolation_negative.bal @@ -50,6 +50,13 @@ isolated class InvalidIsolatedClassNotOverridingMutableFieldsInIncludedIsolatedO } } +isolated object {} invalidIsolatedObjectNotOverridingMutableFieldsInIncludedIsolatedObject = object IsolatedObjectType { + function init() { + self.a = 1; + self.b = []; + } +}; + isolated class InvalidIsolatedClassAccessingMutableFieldsOutsideLock { final int a = 1; private string b = "hello"; From baf164c0fa7fd0bcc56918bec6b1e118d6ea9864 Mon Sep 17 00:00:00 2001 From: MaryamZi Date: Tue, 20 Oct 2020 13:44:00 +0530 Subject: [PATCH 17/19] Update isolated expression analysis --- .../semantics/analyzer/IsolationAnalyzer.java | 208 ++++++++++++++---- .../test/isolation/IsolatedObjectTest.java | 86 ++++---- .../isolated_objects_isolation_negative.bal | 43 ++-- 3 files changed, 242 insertions(+), 95 deletions(-) diff --git a/compiler/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/semantics/analyzer/IsolationAnalyzer.java b/compiler/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/semantics/analyzer/IsolationAnalyzer.java index cd8d057348ed..1099aa1d82f0 100644 --- a/compiler/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/semantics/analyzer/IsolationAnalyzer.java +++ b/compiler/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/semantics/analyzer/IsolationAnalyzer.java @@ -134,6 +134,7 @@ import org.wso2.ballerinalang.compiler.tree.expressions.BLangXMLProcInsLiteral; import org.wso2.ballerinalang.compiler.tree.expressions.BLangXMLQName; import org.wso2.ballerinalang.compiler.tree.expressions.BLangXMLQuotedString; +import org.wso2.ballerinalang.compiler.tree.expressions.BLangXMLSequenceLiteral; import org.wso2.ballerinalang.compiler.tree.expressions.BLangXMLTextLiteral; import org.wso2.ballerinalang.compiler.tree.matchpatterns.BLangConstPattern; import org.wso2.ballerinalang.compiler.tree.matchpatterns.BLangMatchPattern; @@ -221,7 +222,7 @@ public class IsolationAnalyzer extends BLangNodeVisitor { private boolean inferredIsolated = true; private boolean inLockStatement = false; private Map uniqueInitAndReferenceInfo = new HashMap<>(); - private Stack copyInInLockInfoStack = new Stack<>(); + private Stack copyInLockInfoStack = new Stack<>(); private IsolationAnalyzer(CompilerContext context) { context.put(ISOLATION_ANALYZER_KEY, this); @@ -427,7 +428,7 @@ public void visit(BLangSimpleVariable varNode) { UniqueInitAndReferenceInfo varInfo = this.uniqueInitAndReferenceInfo.get(symbol); ArrayList refsUsedInInitExpr = new ArrayList<>(); - boolean uniqueInitExpr = isReferenceOrValidUniqueExpression(expr, refsUsedInInitExpr, false); + boolean uniqueInitExpr = isReferenceOrIsolatedExpression(expr, refsUsedInInitExpr, false); if (varInfo == null) { uniqueInitAndReferenceInfo.put(symbol, new UniqueInitAndReferenceInfo(uniqueInitExpr, refsUsedInInitExpr)); } else { @@ -721,19 +722,23 @@ public void visit(BLangWhile whileNode) { public void visit(BLangLock lockNode) { boolean prevInLockStatement = this.inLockStatement; this.inLockStatement = true; - copyInInLockInfoStack.push(new PotentiallyInvalidExpressionInfo(env.enclInvokable)); + copyInLockInfoStack.push(new PotentiallyInvalidExpressionInfo(env.enclInvokable)); analyzeNode(lockNode.body, SymbolEnv.createLockEnv(lockNode, env)); - PotentiallyInvalidExpressionInfo copyInInLockInfo = copyInInLockInfoStack.pop(); + PotentiallyInvalidExpressionInfo copyInInLockInfo = copyInLockInfoStack.pop(); this.inLockStatement = prevInLockStatement; if (copyInInLockInfo.hasMutableAccessInLock) { - for (BLangSimpleVarRef varRef : copyInInLockInfo.varRefs) { + for (BLangSimpleVarRef varRef : copyInInLockInfo.copyInVarRefs) { dlog.error(varRef.pos, DiagnosticCode.INVALID_COPY_IN_OF_MUTABLE_VALUE_INTO_ISOLATED_OBJECT); } + for (BLangSimpleVarRef varRef : copyInInLockInfo.copyOutVarRefs) { + dlog.error(varRef.pos, DiagnosticCode.INVALID_COPY_OUT_OF_MUTABLE_VALUE_FROM_ISOLATED_OBJECT); + } + for (BLangInvocation invocation : copyInInLockInfo.invocations) { dlog.error(invocation.pos, DiagnosticCode.INVALID_NON_ISOLATED_INVOCATION_IN_ISOLATED_OBJECT_METHOD); } @@ -741,10 +746,10 @@ public void visit(BLangLock lockNode) { return; } - for (int i = copyInInLockInfoStack.size() - 1; i >= 0; i--) { - PotentiallyInvalidExpressionInfo prevCopyInInLockInfo = copyInInLockInfoStack.get(i); + for (int i = copyInLockInfoStack.size() - 1; i >= 0; i--) { + PotentiallyInvalidExpressionInfo prevCopyInLockInfo = copyInLockInfoStack.get(i); - BLangInvokableNode enclInvokableNode = prevCopyInInLockInfo.enclInvokableNode; + BLangInvokableNode enclInvokableNode = prevCopyInLockInfo.enclInvokableNode; if (enclInvokableNode.getKind() != NodeKind.FUNCTION) { return; } @@ -754,7 +759,8 @@ public void visit(BLangLock lockNode) { return; } - prevCopyInInLockInfo.varRefs.addAll(copyInInLockInfo.varRefs); + prevCopyInLockInfo.copyInVarRefs.addAll(copyInInLockInfo.copyInVarRefs); + prevCopyInLockInfo.copyOutVarRefs.addAll(copyInInLockInfo.copyOutVarRefs); } } @@ -900,10 +906,10 @@ public void visit(BLangSimpleVarRef varRefExpr) { if (inLockStatement && isInIsolatedObjectMethod(env, true)) { if ((!varRefExpr.lhsVar || varRefExpr.parent.getKind() != NodeKind.ASSIGNMENT) && !Names.SELF.value.equals(varRefExpr.variableName.value) && isInvalidCopyIn(varRefExpr, env)) { - copyInInLockInfoStack.peek().varRefs.add(varRefExpr); + copyInLockInfoStack.peek().copyInVarRefs.add(varRefExpr); } else if (!varRefExpr.lhsVar && isInvalidCopyingOfMutableValueInIsolatedObject(varRefExpr, true)) { - dlog.error(varRefExpr.pos, DiagnosticCode.INVALID_COPY_OUT_OF_MUTABLE_VALUE_FROM_ISOLATED_OBJECT); + copyInLockInfoStack.peek().copyOutVarRefs.add(varRefExpr); } } @@ -971,7 +977,7 @@ public void visit(BLangFieldBasedAccess fieldAccessExpr) { } if (inLockStatement) { - copyInInLockInfoStack.peek().hasMutableAccessInLock = true; + copyInLockInfoStack.peek().hasMutableAccessInLock = true; return; } @@ -1538,7 +1544,7 @@ private void analyzeInvocation(BLangInvocation invocationExpr) { } if (inLockStatement && isInIsolatedObjectMethod(env, true)) { - copyInInLockInfoStack.peek().invocations.add(invocationExpr); + copyInLockInfoStack.peek().invocations.add(invocationExpr); } inferredIsolated = false; @@ -1658,7 +1664,7 @@ private void validateIsolatedObjectFieldInitialValue(BType fieldType, BLangExpre return; } - isReferenceOrValidUniqueExpression(expression); + isReferenceOrIsolatedExpression(expression); } private boolean isIsolatedObjectTypes(BType type) { @@ -1680,14 +1686,14 @@ private boolean isIsolatedObjectTypes(BType type) { return true; } - private void isReferenceOrValidUniqueExpression(BLangExpression expression) { - isReferenceOrValidUniqueExpression(expression, null, true); + private void isReferenceOrIsolatedExpression(BLangExpression expression) { + isReferenceOrIsolatedExpression(expression, null, true); } - private boolean isReferenceOrValidUniqueExpression(BLangExpression expression, - List refList, - boolean logErrors) { - if (Symbols.isFlagOn(expression.type.flags, Flags.READONLY)) { + private boolean isReferenceOrIsolatedExpression(BLangExpression expression, List refList, + boolean logErrors) { + BType type = expression.type; + if (Symbols.isFlagOn(type.flags, Flags.READONLY) || isIsolatedObjectTypes(type)) { return true; } @@ -1717,7 +1723,16 @@ private boolean isReferenceOrValidUniqueExpression(BLangExpression expression, return true; case LIST_CONSTRUCTOR_EXPR: for (BLangExpression expr : ((BLangListConstructorExpr) expression).exprs) { - if (isReferenceOrValidUniqueExpression(expr, refList, logErrors) || logErrors) { + if (isReferenceOrIsolatedExpression(expr, refList, logErrors) || logErrors) { + continue; + } + + return false; + } + return true; + case TABLE_CONSTRUCTOR_EXPR: + for (BLangRecordLiteral mappingConstr : ((BLangTableConstructorExpr) expression).recordLiteralList) { + if (isReferenceOrIsolatedExpression(mappingConstr, refList, logErrors) || logErrors) { continue; } @@ -1732,12 +1747,12 @@ private boolean isReferenceOrValidUniqueExpression(BLangExpression expression, BLangRecordLiteral.BLangRecordKey key = keyValueField.key; if (key.computedKey) { - if (!isReferenceOrValidUniqueExpression(key.expr, refList, logErrors) && !logErrors) { + if (!isReferenceOrIsolatedExpression(key.expr, refList, logErrors) && !logErrors) { return false; } } - if (isReferenceOrValidUniqueExpression(keyValueField.valueExpr, refList, logErrors) || + if (isReferenceOrIsolatedExpression(keyValueField.valueExpr, refList, logErrors) || logErrors) { continue; } @@ -1745,7 +1760,7 @@ private boolean isReferenceOrValidUniqueExpression(BLangExpression expression, } if (field.getKind() == NodeKind.RECORD_LITERAL_SPREAD_OP) { - if (isReferenceOrValidUniqueExpression( + if (isReferenceOrIsolatedExpression( ((BLangRecordLiteral.BLangRecordSpreadOperatorField) field).expr, refList, logErrors) || logErrors) { continue; @@ -1753,35 +1768,153 @@ private boolean isReferenceOrValidUniqueExpression(BLangExpression expression, return false; } - if (isReferenceOrValidUniqueExpression((BLangRecordLiteral.BLangRecordVarNameField) field, refList, - logErrors) || logErrors) { + if (isReferenceOrIsolatedExpression((BLangRecordLiteral.BLangRecordVarNameField) field, refList, + logErrors) || logErrors) { continue; } return false; } return true; - case INVOCATION: - BLangInvocation invocation = (BLangInvocation) expression; + case XML_COMMENT_LITERAL: + BLangXMLCommentLiteral commentLiteral = (BLangXMLCommentLiteral) expression; + + for (BLangExpression textFragment : commentLiteral.textFragments) { + if (isReferenceOrIsolatedExpression(textFragment, refList, logErrors) || logErrors) { + continue; + } + + return false; + } + + BLangExpression commentLiteralConcatExpr = commentLiteral.concatExpr; + if (commentLiteralConcatExpr == null) { + return true; + } + return isReferenceOrIsolatedExpression(commentLiteralConcatExpr, refList, logErrors); + case XML_TEXT_LITERAL: + BLangXMLTextLiteral textLiteral = (BLangXMLTextLiteral) expression; + + for (BLangExpression textFragment : textLiteral.textFragments) { + if (isReferenceOrIsolatedExpression(textFragment, refList, logErrors) || logErrors) { + continue; + } + + return false; + } - if (Symbols.isFlagOn(invocation.type.flags, Flags.READONLY)) { + BLangExpression textLiteralConcatExpr = textLiteral.concatExpr; + if (textLiteralConcatExpr == null) { return true; } + return isReferenceOrIsolatedExpression(textLiteralConcatExpr, refList, logErrors); + case XML_PI_LITERAL: + BLangXMLProcInsLiteral procInsLiteral = (BLangXMLProcInsLiteral) expression; + + for (BLangExpression dataFragment : procInsLiteral.dataFragments) { + if (isReferenceOrIsolatedExpression(dataFragment, refList, logErrors) || logErrors) { + continue; + } + + return false; + } + + BLangExpression procInsLiteralConcatExpr = procInsLiteral.dataConcatExpr; + if (procInsLiteralConcatExpr == null) { + return true; + } + return isReferenceOrIsolatedExpression(procInsLiteralConcatExpr, refList, logErrors); + case XML_ELEMENT_LITERAL: + for (BLangExpression child : ((BLangXMLElementLiteral) expression).children) { + if (isReferenceOrIsolatedExpression(child, refList, logErrors) || logErrors) { + continue; + } + + return false; + } + return true; + case XML_SEQUENCE_LITERAL: + for (BLangExpression xmlItem : ((BLangXMLSequenceLiteral) expression).xmlItems) { + if (isReferenceOrIsolatedExpression(xmlItem, refList, logErrors) || logErrors) { + continue; + } + + return false; + } + return true; + case STRING_TEMPLATE_LITERAL: + for (BLangExpression expr : ((BLangStringTemplateLiteral) expression).exprs) { + if (isReferenceOrIsolatedExpression(expr, refList, logErrors) || logErrors) { + continue; + } + + return false; + } + return true; + case TYPE_CONVERSION_EXPR: + return isReferenceOrIsolatedExpression(((BLangTypeConversionExpr) expression).expr, refList, logErrors); + case CHECK_EXPR: + case CHECK_PANIC_EXPR: + return isReferenceOrIsolatedExpression(((BLangCheckedExpr) expression).expr, refList, logErrors); + case TRAP_EXPR: + return isReferenceOrIsolatedExpression(((BLangTrapExpr) expression).expr, refList, logErrors); + case TERNARY_EXPR: + BLangTernaryExpr ternaryExpr = (BLangTernaryExpr) expression; + + if (!isReferenceOrIsolatedExpression(ternaryExpr.expr, refList, logErrors) && !logErrors) { + return false; + } + + if (!isReferenceOrIsolatedExpression(ternaryExpr.thenExpr, refList, logErrors) && !logErrors) { + return false; + } + + return isReferenceOrIsolatedExpression(ternaryExpr.elseExpr, refList, logErrors); + case ELVIS_EXPR: + BLangElvisExpr elvisExpr = (BLangElvisExpr) expression; + + if (!isReferenceOrIsolatedExpression(elvisExpr.lhsExpr, refList, logErrors) && !logErrors) { + return false; + } + + return isReferenceOrIsolatedExpression(elvisExpr.rhsExpr, refList, logErrors); + case TYPE_INIT_EXPR: + BLangTypeInit typeInitExpr = (BLangTypeInit) expression; + + BLangInvocation typeInitInvocation = typeInitExpr.initInvocation; + + if (typeInitExpr == null) { + return true; + } + + return isReferenceOrIsolatedExpression(typeInitInvocation, refList, logErrors); + case INVOCATION: + BLangInvocation invocation = (BLangInvocation) expression; if (isCloneOrCloneReadOnlyInvocation(invocation)) { return true; } if (isIsolated(invocation.symbol.type.flags)) { - for (BLangExpression requiredArg : invocation.requiredArgs) { + List requiredArgs = invocation.requiredArgs; + + BLangExpression calledOnExpr = invocation.expr; + + if (calledOnExpr != null && + (requiredArgs.isEmpty() || calledOnExpr != requiredArgs.get(0)) && + (!isReferenceOrIsolatedExpression(calledOnExpr, refList, logErrors) && !logErrors)) { + return false; + } + + for (BLangExpression requiredArg : requiredArgs) { if (requiredArg.getKind() == NodeKind.NAMED_ARGS_EXPR) { - if (isReferenceOrValidUniqueExpression(((BLangNamedArgsExpression) requiredArg).expr, - refList, logErrors) || logErrors) { + if (isReferenceOrIsolatedExpression(((BLangNamedArgsExpression) requiredArg).expr, + refList, logErrors) || logErrors) { continue; } return false; } - if (isReferenceOrValidUniqueExpression(requiredArg, refList, logErrors) || logErrors) { + if (isReferenceOrIsolatedExpression(requiredArg, refList, logErrors) || logErrors) { continue; } return false; @@ -1789,14 +1922,14 @@ private boolean isReferenceOrValidUniqueExpression(BLangExpression expression, for (BLangExpression restArg : invocation.restArgs) { if (restArg.getKind() == NodeKind.REST_ARGS_EXPR) { - if (isReferenceOrValidUniqueExpression(((BLangRestArgsExpression) restArg).expr, refList, - logErrors) || logErrors) { + if (isReferenceOrIsolatedExpression(((BLangRestArgsExpression) restArg).expr, refList, + logErrors) || logErrors) { continue; } return false; } - if (isReferenceOrValidUniqueExpression(restArg, refList, logErrors) || logErrors) { + if (isReferenceOrIsolatedExpression(restArg, refList, logErrors) || logErrors) { continue; } return false; @@ -2039,7 +2172,8 @@ private static class PotentiallyInvalidExpressionInfo { BLangInvokableNode enclInvokableNode; boolean hasMutableAccessInLock = false; - List varRefs = new ArrayList<>(); + List copyInVarRefs = new ArrayList<>(); + List copyOutVarRefs = new ArrayList<>(); List invocations = new ArrayList<>(); private PotentiallyInvalidExpressionInfo(BLangInvokableNode enclInvokableNode) { diff --git a/tests/jballerina-unit-test/src/test/java/org/ballerinalang/test/isolation/IsolatedObjectTest.java b/tests/jballerina-unit-test/src/test/java/org/ballerinalang/test/isolation/IsolatedObjectTest.java index ef84caf7d996..34487a867785 100644 --- a/tests/jballerina-unit-test/src/test/java/org/ballerinalang/test/isolation/IsolatedObjectTest.java +++ b/tests/jballerina-unit-test/src/test/java/org/ballerinalang/test/isolation/IsolatedObjectTest.java @@ -71,56 +71,56 @@ public void testIsolatedObjectIsolationNegative() { validateError(result, i++, "invalid access of a mutable field of an 'isolated' object outside a 'lock' statement", 99, 20); validateError(result, i++, "invalid initial value expression: expected a unique expression", 116, 30); - validateError(result, i++, "invalid initial value expression: expected a unique expression", 122, 22); - validateError(result, i++, "invalid initial value expression: expected a unique expression", 124, 23); - validateError(result, i++, "invalid initial value expression: expected a unique expression", 124, 35); validateError(result, i++, "invalid initial value expression: expected a unique expression", 129, 18); - validateError(result, i++, "invalid initial value expression: expected a unique expression", 135, 30); - validateError(result, i++, "invalid initial value expression: expected a unique expression", 135, 42); - validateError(result, i++, "invalid initial value expression: expected a unique expression", 136, 34); - validateError(result, i++, "invalid initial value expression: expected a unique expression", 143, 22); - validateError(result, i++, "invalid attempt to copy a mutable value into an 'isolated' object", 164, 25); - validateError(result, i++, "invalid attempt to copy a mutable value into an 'isolated' object", 165, 25); - validateError(result, i++, "invalid attempt to copy a mutable value into an 'isolated' object", 166, 23); - validateError(result, i++, "invalid attempt to copy a mutable value into an 'isolated' object", 175, 36); - validateError(result, i++, "invalid attempt to copy out a mutable value from an 'isolated' object", 176, 23); - validateError(result, i++, "invalid attempt to copy a mutable value into an 'isolated' object", 179, 25); - validateError(result, i++, "invalid attempt to copy a mutable value into an 'isolated' object", 180, 25); - validateError(result, i++, "invalid attempt to copy a mutable value into an 'isolated' object", 181, 23); - validateError(result, i++, "invalid attempt to copy a mutable value into an 'isolated' object", 196, 25); - validateError(result, i++, "invalid attempt to copy a mutable value into an 'isolated' object", 197, 25); - validateError(result, i++, "invalid attempt to copy a mutable value into an 'isolated' object", 198, 23); - validateError(result, i++, "invalid attempt to copy a mutable value into an 'isolated' object", 207, 36); - validateError(result, i++, "invalid attempt to copy out a mutable value from an 'isolated' object", 208, 23); - validateError(result, i++, "invalid attempt to copy a mutable value into an 'isolated' object", 211, 25); - validateError(result, i++, "invalid attempt to copy a mutable value into an 'isolated' object", 212, 25); - validateError(result, i++, "invalid attempt to copy a mutable value into an 'isolated' object", 213, 23); - validateError(result, i++, "invalid attempt to copy out a mutable value from an 'isolated' object", 232, 19); - validateError(result, i++, "invalid attempt to copy out a mutable value from an 'isolated' object", 233, 27); - validateError(result, i++, "invalid attempt to copy out a mutable value from an 'isolated' object", 234, 20); - validateError(result, i++, "invalid attempt to copy out a mutable value from an 'isolated' object", 235, 20); - validateError(result, i++, "invalid attempt to copy out a mutable value from an 'isolated' object", 245, 23); - validateError(result, i++, "invalid attempt to copy out a mutable value from an 'isolated' object", 248, 19); - validateError(result, i++, "invalid attempt to copy out a mutable value from an 'isolated' object", 249, 19); - validateError(result, i++, "invalid attempt to copy out a mutable value from an 'isolated' object", 263, 23); - validateError(result, i++, "invalid attempt to copy out a mutable value from an 'isolated' object", 264, 23); - validateError(result, i++, "invalid attempt to copy out a mutable value from an 'isolated' object", 265, 24); - validateError(result, i++, "invalid attempt to copy out a mutable value from an 'isolated' object", 266, 24); - validateError(result, i++, "invalid attempt to copy out a mutable value from an 'isolated' object", 276, 27); - validateError(result, i++, "invalid attempt to copy out a mutable value from an 'isolated' object", 279, 31); - validateError(result, i++, "invalid attempt to copy out a mutable value from an 'isolated' object", 280, 23); + validateError(result, i++, "invalid initial value expression: expected a unique expression", 129, 24); + validateError(result, i++, "invalid initial value expression: expected a unique expression", 129, 36); + validateError(result, i++, "invalid initial value expression: expected a unique expression", 132, 18); + validateError(result, i++, "invalid initial value expression: expected a unique expression", 139, 30); + validateError(result, i++, "invalid initial value expression: expected a unique expression", 139, 42); + validateError(result, i++, "invalid initial value expression: expected a unique expression", 140, 34); + validateError(result, i++, "invalid initial value expression: expected a unique expression", 155, 22); + validateError(result, i++, "invalid attempt to copy a mutable value into an 'isolated' object", 177, 25); + validateError(result, i++, "invalid attempt to copy a mutable value into an 'isolated' object", 178, 25); + validateError(result, i++, "invalid attempt to copy a mutable value into an 'isolated' object", 179, 23); + validateError(result, i++, "invalid attempt to copy a mutable value into an 'isolated' object", 188, 36); + validateError(result, i++, "invalid attempt to copy out a mutable value from an 'isolated' object", 189, 23); + validateError(result, i++, "invalid attempt to copy a mutable value into an 'isolated' object", 192, 25); + validateError(result, i++, "invalid attempt to copy a mutable value into an 'isolated' object", 193, 25); + validateError(result, i++, "invalid attempt to copy a mutable value into an 'isolated' object", 194, 23); + validateError(result, i++, "invalid attempt to copy a mutable value into an 'isolated' object", 209, 25); + validateError(result, i++, "invalid attempt to copy a mutable value into an 'isolated' object", 210, 25); + validateError(result, i++, "invalid attempt to copy a mutable value into an 'isolated' object", 211, 23); + validateError(result, i++, "invalid attempt to copy a mutable value into an 'isolated' object", 220, 36); + validateError(result, i++, "invalid attempt to copy out a mutable value from an 'isolated' object", 221, 23); + validateError(result, i++, "invalid attempt to copy a mutable value into an 'isolated' object", 224, 25); + validateError(result, i++, "invalid attempt to copy a mutable value into an 'isolated' object", 225, 25); + validateError(result, i++, "invalid attempt to copy a mutable value into an 'isolated' object", 226, 23); + validateError(result, i++, "invalid attempt to copy out a mutable value from an 'isolated' object", 245, 19); + validateError(result, i++, "invalid attempt to copy out a mutable value from an 'isolated' object", 246, 27); + validateError(result, i++, "invalid attempt to copy out a mutable value from an 'isolated' object", 247, 20); + validateError(result, i++, "invalid attempt to copy out a mutable value from an 'isolated' object", 248, 20); + validateError(result, i++, "invalid attempt to copy out a mutable value from an 'isolated' object", 258, 23); + validateError(result, i++, "invalid attempt to copy out a mutable value from an 'isolated' object", 261, 19); + validateError(result, i++, "invalid attempt to copy out a mutable value from an 'isolated' object", 262, 19); + validateError(result, i++, "invalid attempt to copy out a mutable value from an 'isolated' object", 276, 23); + validateError(result, i++, "invalid attempt to copy out a mutable value from an 'isolated' object", 277, 23); + validateError(result, i++, "invalid attempt to copy out a mutable value from an 'isolated' object", 278, 24); + validateError(result, i++, "invalid attempt to copy out a mutable value from an 'isolated' object", 279, 24); + validateError(result, i++, "invalid attempt to copy out a mutable value from an 'isolated' object", 289, 27); + validateError(result, i++, "invalid attempt to copy out a mutable value from an 'isolated' object", 292, 31); + validateError(result, i++, "invalid attempt to copy out a mutable value from an 'isolated' object", 293, 23); validateError(result, i++, "invalid invocation of a non-isolated function in a method accessing a mutable " + - "field of an 'isolated' object", 294, 20); + "field of an 'isolated' object", 307, 20); validateError(result, i++, "invalid invocation of a non-isolated function in a method accessing a mutable " + - "field of an 'isolated' object", 295, 20); + "field of an 'isolated' object", 308, 20); validateError(result, i++, "invalid invocation of a non-isolated function in a method accessing a mutable " + - "field of an 'isolated' object", 297, 17); + "field of an 'isolated' object", 310, 17); validateError(result, i++, "invalid invocation of a non-isolated function in a method accessing a mutable " + - "field of an 'isolated' object", 310, 20); + "field of an 'isolated' object", 323, 20); validateError(result, i++, "invalid invocation of a non-isolated function in a method accessing a mutable " + - "field of an 'isolated' object", 311, 20); + "field of an 'isolated' object", 324, 20); validateError(result, i++, "invalid invocation of a non-isolated function in a method accessing a mutable " + - "field of an 'isolated' object", 313, 17); + "field of an 'isolated' object", 326, 17); Assert.assertEquals(result.getErrorCount(), i); } diff --git a/tests/jballerina-unit-test/src/test/resources/test-src/isolated-objects/isolated_objects_isolation_negative.bal b/tests/jballerina-unit-test/src/test/resources/test-src/isolated-objects/isolated_objects_isolation_negative.bal index 0f6c6860bccc..7e384d1095c4 100644 --- a/tests/jballerina-unit-test/src/test/resources/test-src/isolated-objects/isolated_objects_isolation_negative.bal +++ b/tests/jballerina-unit-test/src/test/resources/test-src/isolated-objects/isolated_objects_isolation_negative.bal @@ -104,29 +104,33 @@ function testInvalidIsolatedObjectConstructorAccessingMutableFieldsOutsideLock() int[] globIntArr = [1200, 12, 12345]; int[] globIntArrCopy = globIntArr; int[] globIntArrCopy2 = globIntArrCopy; - +string globStr = "global string"; map globBoolMap = {a: true, b: false}; -function accessGlobBool() { - _ = globBoolMap; +function accessGlobBoolMap(string s) { + _ = (globBoolMap["a"] ?: true) && (globStr == s); } isolated class InvalidIsolatedClassWithNonUniqueInitializerExprs { private int[][] a; private map b = globBoolMap; - private record {} c; - final string d = "str"; - - function init(int[][]? a) { - if a is int[][] { - self.a = a; - } else { - self.a = [globIntArr, globIntArr]; - } - + private record {} c = {[globStr]: accessGlobBoolMap(globStr)}; + final string d = globStr; + private table e = table key (id) [ + {id: 1, "name": "foo"}, + {id: 2, "name": string `str ${globStr}`} + ]; + final readonly & xml[] f = [xml `hello ${globStr}`, xml ``, xml `ok`, + xml `?pi ${globBoolMap["a"] is boolean ? "true" : globStr}?`]; + final int g = checkpanic trap 'int:fromString(globStr); + private float h; + + function init(int[][]? a) returns error? { + self.a = a ?: [globIntArr, globIntArr]; record {} rec = {"a": 1, "b": 2.0}; anydata ad = rec; self.c = rec; + self.h = check 'float:fromString(globStr); } } @@ -134,13 +138,22 @@ function testInvalidIsolatedObjectWithNonUniqueInitializerExprs() { isolated object {} invalidIsolatedObjectWithNonUniqueInitializerExprs = object { private int[][] a = [globIntArr, globIntArr]; private map b = globBoolMap; - private record {} c; - final string d = "str"; + private record {} c = {[globStr]: accessGlobBoolMap(globStr)}; + final string d = globStr; + private table e = table key (id) [ + {id: 1, "name": "foo"}, + {id: 2, "name": string `str ${globStr}`} + ]; + final readonly & xml[] f = [xml `hello ${globStr}`, xml ``, xml `ok`, + xml `?pi ${globBoolMap["a"] is boolean ? "true" : globStr}?`]; + final int g = checkpanic trap 'int:fromString(globStr); + private float h; function init() { record {} rec = {"a": 1, "b": 2.0}; anydata ad = rec; self.c = rec; + self.h = checkpanic 'float:fromString(globStr); } }; } From 43b6e7ac6cc45e641c4c0dac9f0073b691412a27 Mon Sep 17 00:00:00 2001 From: MaryamZi Date: Tue, 20 Oct 2020 15:35:52 +0530 Subject: [PATCH 18/19] Fix several deviations with the spec for isolated objects - allow final, non-private, isolated object fields in isolated objects - validate initial values for non-private isolated object fields also - disallow refering to self outside lock statements --- .../util/diagnostic/DiagnosticCode.java | 2 + .../semantics/analyzer/IsolationAnalyzer.java | 63 +++-- .../src/main/resources/compiler.properties | 3 + .../test/isolation/IsolatedObjectTest.java | 7 + .../isolated-objects/isolated_objects.bal | 255 ++++++++++++++++++ .../isolated_objects_isolation_negative.bal | 27 ++ 6 files changed, 335 insertions(+), 22 deletions(-) diff --git a/compiler/ballerina-lang/src/main/java/org/ballerinalang/util/diagnostic/DiagnosticCode.java b/compiler/ballerina-lang/src/main/java/org/ballerinalang/util/diagnostic/DiagnosticCode.java index eac4da9fdd9c..2e7c4b42f7cf 100644 --- a/compiler/ballerina-lang/src/main/java/org/ballerinalang/util/diagnostic/DiagnosticCode.java +++ b/compiler/ballerina-lang/src/main/java/org/ballerinalang/util/diagnostic/DiagnosticCode.java @@ -603,6 +603,8 @@ public enum DiagnosticCode { INVALID_NON_PRIVATE_MUTABLE_FIELD_IN_ISOLATED_OBJECT("invalid.non.private.mutable.field.in.isolated.object"), INVALID_MUTABLE_FIELD_ACCESS_IN_ISOLATED_OBJECT_OUTSIDE_LOCK( "invalid.mutable.field.access.in.isolated.object.outside.lock"), + INVALID_REFERENCE_TO_SELF_IN_ISOLATED_OBJECT_OUTSIDE_LOCK( + "invalid.reference.to.self.in.isolated.object.outside.lock"), INVALID_NON_UNIQUE_EXPRESSION_AS_INITIAL_VALUE_IN_ISOLATED_OBJECT( "invalid.non.unique.expression.as.initial.value.in.isolated.object"), INVALID_COPY_OUT_OF_MUTABLE_VALUE_FROM_ISOLATED_OBJECT("invalid.copy.out.of.mutable.value.from.isolated.object"), diff --git a/compiler/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/semantics/analyzer/IsolationAnalyzer.java b/compiler/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/semantics/analyzer/IsolationAnalyzer.java index 1099aa1d82f0..cf4f1fffa7c6 100644 --- a/compiler/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/semantics/analyzer/IsolationAnalyzer.java +++ b/compiler/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/semantics/analyzer/IsolationAnalyzer.java @@ -409,20 +409,21 @@ public void visit(BLangSimpleVariable varNode) { BLangExpression expr = varNode.expr; BType fieldType = varNode.type; - if (isIsolatedClassField() && isMutableField(symbol, fieldType)) { - if (!Symbols.isFlagOn(flags, Flags.PRIVATE)) { - dlog.error(varNode.pos, DiagnosticCode.INVALID_NON_PRIVATE_MUTABLE_FIELD_IN_ISOLATED_OBJECT); - } + boolean isolatedClassField = isIsolatedClassField(); - if (expr != null) { - validateIsolatedObjectFieldInitialValue(fieldType, expr); - } + if (isolatedClassField && isExpectedToBeAPrivateField(symbol, fieldType) && + !Symbols.isFlagOn(flags, Flags.PRIVATE)) { + dlog.error(varNode.pos, DiagnosticCode.INVALID_NON_PRIVATE_MUTABLE_FIELD_IN_ISOLATED_OBJECT); } if (expr == null) { return; } + if (isolatedClassField) { + validateIsolatedObjectFieldInitialValue(fieldType, expr); + } + analyzeNode(expr, env); UniqueInitAndReferenceInfo varInfo = this.uniqueInitAndReferenceInfo.get(symbol); @@ -902,14 +903,18 @@ public void visit(BLangSimpleVarRef varRefExpr) { varInfo.totalRefCount++; } - - if (inLockStatement && isInIsolatedObjectMethod(env, true)) { - if ((!varRefExpr.lhsVar || varRefExpr.parent.getKind() != NodeKind.ASSIGNMENT) && - !Names.SELF.value.equals(varRefExpr.variableName.value) && isInvalidCopyIn(varRefExpr, env)) { - copyInLockInfoStack.peek().copyInVarRefs.add(varRefExpr); - } else if (!varRefExpr.lhsVar && - isInvalidCopyingOfMutableValueInIsolatedObject(varRefExpr, true)) { - copyInLockInfoStack.peek().copyOutVarRefs.add(varRefExpr); + if (isInIsolatedObjectMethod(env, true)) { + if (inLockStatement) { + if ((!varRefExpr.lhsVar || varRefExpr.parent.getKind() != NodeKind.ASSIGNMENT) && + !Names.SELF.value.equals(varRefExpr.variableName.value) && isInvalidCopyIn(varRefExpr, env)) { + copyInLockInfoStack.peek().copyInVarRefs.add(varRefExpr); + } else if (!varRefExpr.lhsVar && + isInvalidCopyingOfMutableValueInIsolatedObject(varRefExpr, true)) { + copyInLockInfoStack.peek().copyOutVarRefs.add(varRefExpr); + } + } else if (Names.SELF.value.equals(varRefExpr.variableName.value) && + varRefExpr.parent.getKind() != NodeKind.FIELD_BASED_ACCESS_EXPR) { + dlog.error(varRefExpr.pos, DiagnosticCode.INVALID_MUTABLE_FIELD_ACCESS_IN_ISOLATED_OBJECT_OUTSIDE_LOCK); } } @@ -972,7 +977,7 @@ public void visit(BLangSimpleVarRef varRefExpr) { public void visit(BLangFieldBasedAccess fieldAccessExpr) { analyzeNode(fieldAccessExpr.expr, env); - if (!isIsolatedObjectMutableFieldAccessViaSelf(fieldAccessExpr, true)) { + if (!isValidIsolatedObjectFieldAccessViaSelfOutsideLock(fieldAccessExpr, true)) { return; } @@ -1528,7 +1533,7 @@ private void analyzeInvocation(BLangInvocation invocationExpr) { List args = new ArrayList<>(invocationExpr.requiredArgs); BLangExpression expr = invocationExpr.expr; - if (expr != null && !args.isEmpty() && args.get(0) != expr) { + if (expr != null && (args.isEmpty() || args.get(0) != expr)) { args.add(0, expr); } @@ -1630,8 +1635,22 @@ private boolean isIsolatedClassField() { return node.getKind() == NodeKind.CLASS_DEFN && ((BLangClassDefinition) node).flagSet.contains(Flag.ISOLATED); } - private boolean isMutableField(BVarSymbol symbol, BType type) { - return !Symbols.isFlagOn(symbol.flags, Flags.FINAL) || !Symbols.isFlagOn(type.flags, Flags.READONLY); + private boolean isExpectedToBeAPrivateField(BVarSymbol symbol, BType type) { + return !Symbols.isFlagOn(symbol.flags, Flags.FINAL) || isNotSubTypeOfReadOnlyOrIsolatedObject(type); + } + + private boolean isNotSubTypeOfReadOnlyOrIsolatedObject(BType type) { + if (type.tag == TypeTags.UNION) { + for (BType memberType : ((BUnionType) type).getMemberTypes()) { + if (isNotSubTypeOfReadOnlyOrIsolatedObject(memberType)) { + return true; + } + } + return false; + } + + int flags = type.flags; + return !Symbols.isFlagOn(flags, Flags.READONLY) && !isIsolatedObjectTypes(type); } private boolean isIsolatedObjectFieldAccessViaSelf(BLangFieldBasedAccess fieldAccessExpr, boolean ignoreInit) { @@ -1648,15 +1667,15 @@ private boolean isIsolatedObjectFieldAccessViaSelf(BLangFieldBasedAccess fieldAc return isInIsolatedObjectMethod(env, ignoreInit); } - private boolean isIsolatedObjectMutableFieldAccessViaSelf(BLangFieldBasedAccess fieldAccessExpr, - boolean ignoreInit) { + private boolean isValidIsolatedObjectFieldAccessViaSelfOutsideLock(BLangFieldBasedAccess fieldAccessExpr, + boolean ignoreInit) { if (!isIsolatedObjectFieldAccessViaSelf(fieldAccessExpr, ignoreInit)) { return false; } BField field = ((BObjectType) env.enclInvokable.symbol.owner.type).fields.get(fieldAccessExpr.field.value); - return isMutableField(field.symbol, field.type); + return isExpectedToBeAPrivateField(field.symbol, field.type); } private void validateIsolatedObjectFieldInitialValue(BType fieldType, BLangExpression expression) { diff --git a/compiler/ballerina-lang/src/main/resources/compiler.properties b/compiler/ballerina-lang/src/main/resources/compiler.properties index 30879576b792..26ce7fccb294 100644 --- a/compiler/ballerina-lang/src/main/resources/compiler.properties +++ b/compiler/ballerina-lang/src/main/resources/compiler.properties @@ -1517,6 +1517,9 @@ error.invalid.non.private.mutable.field.in.isolated.object=\ error.invalid.mutable.field.access.in.isolated.object.outside.lock=\ invalid access of a mutable field of an ''isolated'' object outside a ''lock'' statement +error.invalid.reference.to.self.in.isolated.object.outside.lock=\ + invalid reference to ''self'' outside a ''lock'' statement in an ''isolated'' object + error.invalid.non.unique.expression.as.initial.value.in.isolated.object=\ invalid initial value expression: expected a unique expression diff --git a/tests/jballerina-unit-test/src/test/java/org/ballerinalang/test/isolation/IsolatedObjectTest.java b/tests/jballerina-unit-test/src/test/java/org/ballerinalang/test/isolation/IsolatedObjectTest.java index 34487a867785..1018d89692b6 100644 --- a/tests/jballerina-unit-test/src/test/java/org/ballerinalang/test/isolation/IsolatedObjectTest.java +++ b/tests/jballerina-unit-test/src/test/java/org/ballerinalang/test/isolation/IsolatedObjectTest.java @@ -121,6 +121,13 @@ public void testIsolatedObjectIsolationNegative() { "field of an 'isolated' object", 324, 20); validateError(result, i++, "invalid invocation of a non-isolated function in a method accessing a mutable " + "field of an 'isolated' object", 326, 17); + validateError(result, i++, "invalid non-private mutable field in an 'isolated' object", 332, 5); + validateError(result, i++, "invalid non-private mutable field in an 'isolated' object", 333, 5); + validateError(result, i++, "invalid non-private mutable field in an 'isolated' object", 337, 5); + validateError(result, i++, "invalid access of a mutable field of an 'isolated' object outside a 'lock' " + + "statement", 345, 13); + validateError(result, i++, "invalid access of a mutable field of an 'isolated' object outside a 'lock' " + + "statement", 346, 9); Assert.assertEquals(result.getErrorCount(), i); } diff --git a/tests/jballerina-unit-test/src/test/resources/test-src/isolated-objects/isolated_objects.bal b/tests/jballerina-unit-test/src/test/resources/test-src/isolated-objects/isolated_objects.bal index 64523c0486da..0205804890d0 100644 --- a/tests/jballerina-unit-test/src/test/resources/test-src/isolated-objects/isolated_objects.bal +++ b/tests/jballerina-unit-test/src/test/resources/test-src/isolated-objects/isolated_objects.bal @@ -14,3 +14,258 @@ // specific language governing permissions and limitations // under the License. +public isolated class IsolatedClassWithNoMutableFields { + public final int[] & readonly a; + final readonly & record {int i;} b; + + function init(int[] & readonly a, record {int i;} & readonly b) { + self.a = a; + self.b = b; + } +} + +isolated class IsolatedClassWithPrivateMutableFields { + public final int[] & readonly a = [10, 20, 30]; + final readonly & record {int i;} b; + + private int c; + private map[] d; + + isolated function init(record {int i;} & readonly b, int c) { + self.b = b; + self.c = c; + } +} + +final readonly & string[] immutableStringArray = ["hello", "world"]; + +type IsolatedObjectType isolated object { + int a; + string[] b; +}; + +isolated class IsolatedClassOverridingMutableFieldsInIncludedIsolatedObject { + *IsolatedObjectType; + + final int a = 100; + private string[] b; + + function init() { + self.b = []; + } + + function accessImmutableField() returns int => self.a + 1; + + isolated function accessMutableField() returns int { + lock { + self.b.push(...immutableStringArray); + return self.b.length(); + } + } +} + +function testIsolatedObjectOverridingMutableFieldsInIncludedIsolatedObject() { + isolated object {} isolatedObjectOverridingMutableFieldsInIncludedIsolatedObject = object IsolatedObjectType { + + final int a = 100; + private string[] b = []; + + function accessImmutableField() returns int => self.a + 1; + + isolated function accessMutableField() returns int { + lock { + self.b.push(...immutableStringArray); + return self.b.length(); + } + } + }; +} + +isolated class IsolatedClassWithMethodsAccessingPrivateMutableFieldsWithinLockStatements { + public final int[] & readonly a = [10, 20, 30]; + final readonly & record {int i;} b; + + private int c; + private map d; + + isolated function init(record {int i;} & readonly b, int c, map d) { + self.b = b; + self.c = c; + self.d = d.clone(); + } + + isolated function accessMutableFieldInLockOne() returns int[] { + int i = 1; + + lock { + self.c = i; + + int j = i; + self.c = i; + + map k = { + a: [1, 2, i, j, i + j], + b: self.a + }; + self.d = k; + k = self.d; + } + + return self.a; + } + + function accessMutableFieldInLockTwo() returns int[] { + int i = 1; + int[] x; + + lock { + int j = i; + + lock { + int[] y = [1, 2, 3]; + x = y.clone(); + self.c = i; + } + + map k = { + a: [1, 2, i, j, i + j], + b: self.a + }; + self.d = k; + k = self.d; + } + + return self.a; + } + + isolated function accessImmutableFieldOutsideLock() returns int[] { + int[] arr = []; + + arr.push(...self.a); + arr[5] = self.b.i; + return arr; + } +} + +isolated object {} isolatedObjectWithMethodsAccessingPrivateMutableFieldsWithinLockStatements = object { + public final int[] & readonly a = [10, 20, 30]; + final readonly & record {int i;} b = {i: 101}; + + private int c = 1; + private map d = {}; + + function accessMutableFieldInLockOne() returns int[] { + int i = 1; + + lock { + self.c = i; + + int j = i; + self.c = i; + + map k = { + a: [1, 2, i, j, i + j], + b: self.a + }; + self.d = k; + k = self.d; + } + + return self.a; + } + + isolated function accessMutableFieldInLockTwo() returns int[] { + int i = 1; + int[] x; + + lock { + int j = i; + + lock { + int[] y = [1, 2, 3]; + x = y.clone(); + self.c = i; + } + + map k = { + a: [1, 2, i, j, i + j], + b: self.a + }; + self.d = k; + k = self.d; + } + + return self.a; + } + + isolated function accessImmutableFieldOutsideLock() returns int[] { + int[] arr = []; + + arr.push(...self.a); + arr[5] = self.b.i; + return arr; + } +}; + +isolated class IsolatedClassWithNonPrivateIsolatedObjectFields { + final isolated object {} a = object { + final int i = 1; + private map j = {}; + }; + final IsolatedClassWithMethodsAccessingPrivateMutableFieldsWithinLockStatements b; + private final IsolatedClassWithMethodsAccessingPrivateMutableFieldsWithinLockStatements c = new ({i: 1}, 102, {}); + private int[] d = [1, 2]; + + function init() { + self.b = new ({i: 1}, 102, {}); + } + + isolated function accessNonPrivateIsoltedObjectFieldOutsideLock() { + int[] x = self.b.accessMutableFieldInLockOne(); + } +} + +isolated object {} isolatedObjectWithNonPrivateIsolatedObjectFields = object { + final isolated object { function foo() returns int; } a = object { + final int i = 1; + private map j = {}; + + isolated function foo() returns int { + lock { + return self.i + (self.j["a"] ?: 1); + } + } + }; + final int|IsolatedClassWithMethodsAccessingPrivateMutableFieldsWithinLockStatements b = new ({i: 1}, 102, {}); + private final IsolatedClassWithMethodsAccessingPrivateMutableFieldsWithinLockStatements c = new ({i: 1}, 102, {}); + private int[] d = [1, 2]; + + function accessNonPrivateIsoltedObjectFieldOutsideLock() { + int x = self.a.foo(); + } +}; + +const INT = 100; + +isolated class IsolatedClassWithUniqueInitializerExprs { + private map a = > {}; + final int[] & readonly b = [1, INT]; + private table c; + + isolated function init(table tb) { + self.c = tb.clone(); + } +} + +isolated object {} isolatedObjectWithUniqueInitializerExprs = object { + private map a = > {}; + final int[] & readonly b = [1, INT]; + private table c; + + isolated function init() { + self.c = table [ + {id: INT, name: "foo"}, + {id: INT + 100, name: "bar"} + ]; + } +}; diff --git a/tests/jballerina-unit-test/src/test/resources/test-src/isolated-objects/isolated_objects_isolation_negative.bal b/tests/jballerina-unit-test/src/test/resources/test-src/isolated-objects/isolated_objects_isolation_negative.bal index 7e384d1095c4..c176818c27b6 100644 --- a/tests/jballerina-unit-test/src/test/resources/test-src/isolated-objects/isolated_objects_isolation_negative.bal +++ b/tests/jballerina-unit-test/src/test/resources/test-src/isolated-objects/isolated_objects_isolation_negative.bal @@ -328,6 +328,33 @@ isolated object {} invalidIsolatedObjectWithNonIsolatedFunctionInvocation = obje } }; +isolated class InvalidIsolatedClassWithNonInvalidObjectFields { + IsolatedClass a = new; // Should be `final` + isolated object {} b = object { // Should be `final` + final int i = 1; + private map j = {}; + }; + final object {} c = object {}; // should be an `isolated object` +} + +isolated class InvalidIsolatedClassReferringSelfOutsideLock { + final int a = 1; + private int[] b = []; + + function foo() { + bar(self); + self.baz(); + } + + function baz() { + + } +} + +function bar(InvalidIsolatedClassReferringSelfOutsideLock x) { + +} + isolated class IsolatedClass { function nonIsolatedFunc() returns int => 1; } From 83c8d51539a33a690cf4364e793d7c162220ccc3 Mon Sep 17 00:00:00 2001 From: MaryamZi Date: Tue, 20 Oct 2020 17:16:34 +0530 Subject: [PATCH 19/19] Fix analysis for new-expr with stream --- .../semantics/analyzer/IsolationAnalyzer.java | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/compiler/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/semantics/analyzer/IsolationAnalyzer.java b/compiler/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/semantics/analyzer/IsolationAnalyzer.java index cf4f1fffa7c6..fd5e043cb04a 100644 --- a/compiler/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/semantics/analyzer/IsolationAnalyzer.java +++ b/compiler/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/semantics/analyzer/IsolationAnalyzer.java @@ -1712,7 +1712,7 @@ private void isReferenceOrIsolatedExpression(BLangExpression expression) { private boolean isReferenceOrIsolatedExpression(BLangExpression expression, List refList, boolean logErrors) { BType type = expression.type; - if (Symbols.isFlagOn(type.flags, Flags.READONLY) || isIsolatedObjectTypes(type)) { + if (type != null && (Symbols.isFlagOn(type.flags, Flags.READONLY) || isIsolatedObjectTypes(type))) { return true; } @@ -1913,7 +1913,16 @@ private boolean isReferenceOrIsolatedExpression(BLangExpression expression, List return true; } - if (isIsolated(invocation.symbol.type.flags)) { + BSymbol invocationSymbol = invocation.symbol; + if (invocationSymbol == null) { + // This is `new` used with a stream. + List argExprs = invocation.argExprs; + if (argExprs.isEmpty()) { + return true; + } + + return isReferenceOrIsolatedExpression(argExprs.get(0), refList, logErrors); + } else if (isIsolated(invocationSymbol.type.flags)) { List requiredArgs = invocation.requiredArgs; BLangExpression calledOnExpr = invocation.expr;