Report a missing return statement in a closure or function

.docusaurus_site/sidebars.js

@@ -344,6 +344,7 @@ module.exports = {
                     label: "Migration notes",
                     link: { type: "doc", id: "migrations/index" },
                     items: [
+                        "migrations/26-10",
                         "migrations/26-04",
                         "migrations/25-10",
                         "migrations/25-04",

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<ASTNode> missingReturns = new ArrayList<>();
+
     public ReturnStatementVisitor(SourceUnit sourceUnit, ErrorCollector errorCollector) {
         this.sourceUnit = sourceUnit;
         this.errorCollector = errorCollector;
@@ -65,46 +74,78 @@ protected SourceUnit getSourceUnit() {
         return sourceUnit;
     }
 
-    public void visit(ClassNode returnType, Statement code) {
-        visit(returnType, code, false);
+    public void visit(ASTNode owner, ClassNode returnType, Statement code) {
+        visit(owner, returnType, code, false);
     }
 
     /**
+     * @param owner      function or closure that contains the code
      * @param returnType
      * @param code
      * @param coerce     accept any return value that can be coerced to the return type
      */
-    public void visit(ClassNode returnType, Statement code, boolean coerce) {
+    public void visit(ASTNode owner, ClassNode returnType, Statement code, boolean coerce) {
         this.returnType = returnType;
         this.coerce = coerce;
-        visit(addReturnsIfNeeded(code));
+        visit(addReturnsIfNeeded(code, owner));
+        if( returnsValue() ) {
+            for( var node : missingReturns )
+                addError("Missing return statement", node);
+        }
         this.returnType = null;
     }
 
-    private Statement addReturnsIfNeeded(Statement 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, ASTNode 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/main/java/nextflow/script/control/TypeCheckingVisitor.java

@@ -341,7 +341,7 @@ public void visitFunction(FunctionNode node) {
 
         // check return statements against declared return type
         var visitor = new ReturnStatementVisitor(sourceUnit, errorCollector);
-        visitor.visit(node.getReturnType(), node.getCode());
+        visitor.visit(node, node.getReturnType(), node.getCode());
 
         var inferredReturnType = visitor.getInferredReturnType();
         if( inferredReturnType != null && ClassHelper.isDynamicTyped(node.getReturnType()) )
@@ -1377,7 +1377,7 @@ public void visitClosureExpression(ClosureExpression node) {
             // type, as long as it returns a value
             var coerce = Types.isEqual(returnType, ClassHelper.Boolean_TYPE);
             var visitor = new ReturnStatementVisitor(sourceUnit, errorCollector);
-            visitor.visit(returnType, node.getCode(), coerce);
+            visitor.visit(node, returnType, node.getCode(), coerce);
 
             var inferredReturnType = visitor.getInferredReturnType();
             if( inferredReturnType != null )

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

@@ -416,6 +416,82 @@ 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"
+
+        when: 'the body is empty'
+        errors = getErrors(
+            '''\
+            def hello(x: Integer) -> String { }
+
+            workflow {
+                channel.of(1).map { x -> }
+            }
+            '''
+        )
+        then: 'the error is reported against the function or closure'
+        errors.size() == 2
+        errors[0].getStartLine() == 1
+        errors[0].getStartColumn() == 1
+        errors[1].getStartLine() == 4
+        errors[1].getStartColumn() == 23
+        errors.every { it.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)