Improve scope checking for operators and operator closures

Signed-off-by: Ben Sherman <[email protected]>

Author: bentsherman

modules/compiler/src/main/java/script/control/VariableScopeChecker.java

@@ -184,7 +184,7 @@ private Variable findDslVariable(ClassNode cn, String name, ASTNode node) {
         while( cn != null ) {
             for( var mn : cn.getMethods() ) {
                 // processes, workflows, and operators can be accessed as variables, e.g. with pipes
-                if( isCallableVariable(mn) && name.equals(mn.getName()) ) {
+                if( isDataflowMethod(mn) && name.equals(mn.getName()) ) {
                     return wrapMethodAsVariable(mn, name);
                 }
                 // built-in variables are methods annotated as @Constant
@@ -209,15 +209,15 @@ private Variable findDslVariable(ClassNode cn, String name, ASTNode node) {
         return null;
     }
 
-    private boolean isCallableVariable(MethodNode mn) {
-        if( mn instanceof ProcessNode || mn instanceof WorkflowNode )
-            return true;
-        if( findAnnotation(mn, Operator.class).isPresent() )
-            return true;
-        return false;
+    public static boolean isDataflowMethod(MethodNode mn) {
+        return mn instanceof ProcessNode || mn instanceof WorkflowNode || isOperator(mn);
     }
 
-    private PropertyNode wrapMethodAsVariable(MethodNode mn, String name) {
+    public static boolean isOperator(MethodNode mn) {
+        return findAnnotation(mn, Operator.class).isPresent();
+    }
+
+    private static PropertyNode wrapMethodAsVariable(MethodNode mn, String name) {
         var cn = mn.getDeclaringClass();
         var fn = new FieldNode(name, mn.getModifiers() & 0xF, mn.getReturnType(), cn, null);
         fn.setHasNoRealSourcePosition(true);

modules/compiler/src/main/java/script/control/VariableScopeVisitor.java

@@ -108,6 +108,8 @@ public void declare() {
 
     private void declareInclude(IncludeNode node) {
         for( var module : node.modules ) {
+            if( module.getTarget() == null )
+                continue;
             var name = module.getNameOrAlias();
             var otherInclude = vsc.getInclude(name);
             if( otherInclude != null )
@@ -312,9 +314,9 @@ private MethodCallExpression checkDirective(Statement node, String typeLabel, bo
             return null;
         }
         var name = call.getMethodAsString();
-        var defNode = vsc.findDslFunction(name, call.getMethod());
-        if( defNode != null )
-            call.putNodeMetaData(ASTNodeMarker.METHOD_TARGET, defNode);
+        var mn = vsc.findDslFunction(name, call.getMethod());
+        if( mn != null )
+            call.putNodeMetaData(ASTNodeMarker.METHOD_TARGET, mn);
         else
             vsc.addError("Invalid " + typeLabel + " `" + name + "`", node);
         return call;
@@ -459,8 +461,6 @@ private boolean declareAssignedVariable(VariableExpression ve) {
         if( variable != null ) {
             if( isDslVariable(variable) )
                 vsc.addError("Built-in variable cannot be re-assigned", ve);
-            else
-                checkExternalWriteInAsyncClosure(ve, variable);
             ve.setAccessedVariable(variable);
             return false;
         }
@@ -523,9 +523,8 @@ private void checkExternalWriteInAsyncClosure(VariableExpression target, Variabl
             return;
         var scope = currentClosure.getVariableScope();
         var name = variable.getName();
-        // TODO: should apply only to operator closures
-        if( scope.isReferencedLocalVariable(name) && scope.getDeclaredVariable(name) == null )
-            vsc.addParanoidWarning("Mutating an external variable in an operator closure may lead to a race condition", target, "External variable declared here", (ASTNode) variable);
+        if( inOperatorCall && scope.isReferencedLocalVariable(name) && scope.getDeclaredVariable(name) == null )
+            sourceUnit.addWarning("Mutating an external variable in an operator closure can lead to a race condition", target);
     }
 
     // expressions
@@ -537,6 +536,8 @@ private void checkExternalWriteInAsyncClosure(VariableExpression target, Variabl
         "while"
     );
 
+    private boolean inOperatorCall;
+
     @Override
     public void visitMethodCallExpression(MethodCallExpression node) {
         var target = checkSetAssignment(node);
@@ -546,7 +547,15 @@ public void visitMethodCallExpression(MethodCallExpression node) {
             return;
         }
         checkMethodCall(node);
+        var ioc = inOperatorCall;
+        inOperatorCall = isOperatorCall(node);
         super.visitMethodCallExpression(node);
+        inOperatorCall = ioc;
+    }
+
+    private static boolean isOperatorCall(MethodCallExpression node) {
+        return node.getNodeMetaData(ASTNodeMarker.METHOD_TARGET) instanceof MethodNode mn
+            && VariableScopeChecker.isOperator(mn);
     }
 
     /**
@@ -568,30 +577,38 @@ private void checkMethodCall(MethodCallExpression node) {
         if( !node.isImplicitThis() )
             return;
         var name = node.getMethodAsString();
-        var defNode = vsc.findDslFunction(name, node);
-        if( defNode != null ) {
-            if( defNode instanceof ProcessNode || defNode instanceof WorkflowNode )
-                checkProcessOrWorkflowCall(node, defNode);
-            node.putNodeMetaData(ASTNodeMarker.METHOD_TARGET, defNode);
+        var mn = vsc.findDslFunction(name, node);
+        if( mn != null ) {
+            if( VariableScopeChecker.isDataflowMethod(mn) )
+                checkDataflowMethod(node, mn);
+            node.putNodeMetaData(ASTNodeMarker.METHOD_TARGET, mn);
         }
         else if( !KEYWORDS.contains(name) ) {
             vsc.addError("`" + name + "` is not defined", node.getMethod());
         }
     }
 
-    private void checkProcessOrWorkflowCall(MethodCallExpression node, MethodNode defNode) {
+    private void checkDataflowMethod(MethodCallExpression node, MethodNode mn) {
         if( !(currentDefinition instanceof WorkflowNode) ) {
-            var type = defNode instanceof ProcessNode ? "Processes" : "Workflows";
+            var type = dataflowMethodType(mn);
             vsc.addError(type + " can only be called from a workflow", node);
             return;
         }
         if( currentClosure != null ) {
-            var type = defNode instanceof ProcessNode ? "Processes" : "Workflows";
+            var type = dataflowMethodType(mn);
             vsc.addError(type + " cannot be called from within a closure", node);
             return;
         }
     }
 
+    private static String dataflowMethodType(MethodNode mn) {
+        if( mn instanceof ProcessNode )
+            return "Processes";
+        if( mn instanceof WorkflowNode )
+            return "Workflows";
+        return "Operators";
+    }
+
     @Override
     public void visitDeclarationExpression(DeclarationExpression node) {
         visit(node.getRightExpression());