Report a missing return statement in a closure or function

modules/nf-lang/src/main/java/nextflow/script/control/ReturnStatementVisitor.java

@@ -16,18 +16,25 @@
 package nextflow.script.control;
 
 import java.util.ArrayList;
+import java.util.List;
 
+import nextflow.script.ast.AssignmentExpression;
 import nextflow.script.dsl.Types;
 import org.codehaus.groovy.ast.ASTNode;
 import org.codehaus.groovy.ast.ClassHelper;
 import org.codehaus.groovy.ast.ClassNode;
 import org.codehaus.groovy.ast.ClassCodeVisitorSupport;
 import org.codehaus.groovy.ast.expr.ConstantExpression;
+import org.codehaus.groovy.ast.expr.DeclarationExpression;
+import org.codehaus.groovy.ast.expr.Expression;
 import org.codehaus.groovy.ast.stmt.BlockStatement;
+import org.codehaus.groovy.ast.stmt.CatchStatement;
 import org.codehaus.groovy.ast.stmt.ExpressionStatement;
 import org.codehaus.groovy.ast.stmt.IfStatement;
 import org.codehaus.groovy.ast.stmt.ReturnStatement;
 import org.codehaus.groovy.ast.stmt.Statement;
+import org.codehaus.groovy.ast.stmt.ThrowStatement;
+import org.codehaus.groovy.ast.stmt.TryCatchStatement;
 import org.codehaus.groovy.control.ErrorCollector;
 import org.codehaus.groovy.control.SourceUnit;
 import org.codehaus.groovy.control.messages.SyntaxErrorMessage;
@@ -55,6 +62,8 @@ public class ReturnStatementVisitor extends ClassCodeVisitorSupport {
 
     private boolean coerce;
 
+    private List<Statement> missingReturns = new ArrayList<>();
+
     public ReturnStatementVisitor(SourceUnit sourceUnit, ErrorCollector errorCollector) {
         this.sourceUnit = sourceUnit;
         this.errorCollector = errorCollector;
@@ -78,33 +87,68 @@ public void visit(ClassNode returnType, Statement code, boolean coerce) {
         this.returnType = returnType;
         this.coerce = coerce;
         visit(addReturnsIfNeeded(code));
+        if( returnsValue() ) {
+            for( var node : missingReturns )
+                addError("Missing return statement", node);
+        }
         this.returnType = null;
     }
 
     private Statement addReturnsIfNeeded(Statement node) {
+        return addReturnsIfNeeded(node, node);
+    }
+
+    /**
+     * Convert trailing expression statements into return statements,
+     * and record any code path that ends without a return value.
+     *
+     * @param node
+     * @param parent  node to report if the statement has no source position
+     */
+    private Statement addReturnsIfNeeded(Statement node, Statement parent) {
         if( node instanceof BlockStatement block && !block.isEmpty() ) {
             var statements = new ArrayList<>(block.getStatements());
             int lastIndex = statements.size() - 1;
-            var last = addReturnsIfNeeded(statements.get(lastIndex));
+            var last = addReturnsIfNeeded(statements.get(lastIndex), block);
             statements.set(lastIndex, last);
             return withSourcePosition(new BlockStatement(statements, block.getVariableScope()), block);
         }
 
-        if( node instanceof ExpressionStatement es ) {
+        if( node instanceof ExpressionStatement es && !isAssignment(es.getExpression()) ) {
             return withSourcePosition(new ReturnStatement(es.getExpression()), es);
         }
 
         if( node instanceof IfStatement ies ) {
             return withSourcePosition(new IfStatement(
                 ies.getBooleanExpression(),
-                addReturnsIfNeeded(ies.getIfBlock()),
-                addReturnsIfNeeded(ies.getElseBlock()) ), ies);
+                addReturnsIfNeeded(ies.getIfBlock(), ies),
+                addReturnsIfNeeded(ies.getElseBlock(), ies) ), ies);
         }
 
+        if( node instanceof TryCatchStatement tcs ) {
+            var result = new TryCatchStatement(addReturnsIfNeeded(tcs.getTryStatement(), tcs), tcs.getFinallyStatement());
+            for( var cs : tcs.getCatchStatements() )
+                result.addCatch(withSourcePosition(new CatchStatement(cs.getVariable(), addReturnsIfNeeded(cs.getCode(), cs)), cs));
+            return withSourcePosition(result, tcs);
+        }
+
+        if( !(node instanceof ReturnStatement) && !(node instanceof ThrowStatement) )
+            missingReturns.add(node.getLineNumber() != -1 && !(node instanceof BlockStatement) ? node : parent);
+
         return node;
     }
 
-    private static Statement withSourcePosition(Statement node, Statement source) {
+    private static boolean isAssignment(Expression node) {
+        return node instanceof DeclarationExpression || node instanceof AssignmentExpression;
+    }
+
+    private boolean returnsValue() {
+        if( !ClassHelper.isDynamicTyped(returnType) )
+            return !ClassHelper.VOID_TYPE.equals(returnType);
+        return inferredReturnType != null && !ClassHelper.VOID_TYPE.equals(inferredReturnType);
+    }
+
+    private static <T extends Statement> T withSourcePosition(T node, Statement source) {
         node.setSourcePosition(source);
         return node;
     }

modules/nf-lang/src/test/groovy/nextflow/script/types/TypeCheckingTest.groovy

@@ -389,6 +389,64 @@ class TypeCheckingTest extends Specification {
         errors[0].getOriginalMessage() == "Return value with type void does not match the declared return type (String)"
     }
 
+    def 'should report the position of a missing return statement' () {
+        when:
+        def errors = getErrors(
+            '''\
+            def hello(x: Integer) -> String {
+                if( x > 1 ) {
+                    return 'big'
+                }
+            }
+            '''
+        )
+        then:
+        errors.size() == 1
+        errors[0].getStartLine() == 2
+        errors[0].getOriginalMessage() == "Missing return statement"
+
+        when:
+        errors = getErrors(
+            '''\
+            def hello(x: Integer) -> String {
+                if( x > 1 ) {
+                    def y = 'big'
+                } else {
+                    return 'small'
+                }
+            }
+            '''
+        )
+        then:
+        errors.size() == 1
+        errors[0].getStartLine() == 3
+        errors[0].getOriginalMessage() == "Missing return statement"
+    }
+
+    @Unroll
+    def 'should check for a missing return statement' () {
+        expect:
+        check(SOURCE, ERROR)
+
+        where:
+        SOURCE                                                                                  | ERROR
+        "def f(x: Integer) -> String { if( x > 1 ) { return 'a' } }"                            | "Missing return statement"
+        "def f(x: Integer) -> String { if( x > 1 ) { } else { return 'a' } }"                   | "Missing return statement"
+        "def f(x: Integer) -> String { if( x > 1 ) { def y = 'a' } else { return 'b' } }"       | "Missing return statement"
+        "def f(x: Integer) -> String { def y = 'a' ; if( x > 1 ) { y = 'b' } else { 'c' } }"    | "Missing return statement"
+        "def f(x: Integer) -> String { def y = 'a' }"                                           | "Missing return statement"
+        "def f(x: Integer) -> String { try { 'a' } catch( e: Exception ) { def y = 'b' } }"     | "Missing return statement"
+        "def f(x: Integer) -> String { if( x > 1 ) { return 'a' } ; return 'b' }"               | null
+        "def f(x: Integer) -> String { if( x > 1 ) { 'a' } else if( x > 0 ) { 'b' } else { 'c' } }" | null
+        "def f(x: Integer) -> String { if( x > 1 ) { return 'a' } else { throw new Exception() } }" | null
+        "def f(x: Integer) -> String { try { 'a' } catch( e: Exception ) { 'b' } }"             | null
+        "def f(x: Integer) { if( x > 1 ) { println 'a' } }"                                     | null
+        "def f(x: Integer) { if( x > 1 ) { def y = 'a' } }"                                     | null
+        "channel.of(1).map { x -> if( x > 1 ) { x } }"                                          | "Missing return statement"
+        "channel.of(1).map { x -> if( x > 1 ) { x } else { def y = 0 } }"                       | "Missing return statement"
+        "channel.of(1).map { x -> if( x > 1 ) { x } else { 0 } }"                               | null
+    }
+
     def 'should check an assignment' () {
         expect:
         check(SOURCE, ERROR)