Fix implicit return of pipeline with return/throw

Fixes #1675
DanielXMoore · Jan 5, 2025 · 295bbfe · 295bbfe
1 parent 3514f86
commit 295bbfe
Show file tree

Hide file tree

Showing 3 changed files with 159 additions and 134 deletions.
diff --git a/source/parser/function.civet b/source/parser/function.civet
@@ -465,6 +465,8 @@ function assignResults(node: StatementTuple[] | ASTNode, collect: (node: ASTNode
         assignResults block, collect
       return
     when "PipelineExpression"
+      // Child 0 is whitespace; if child 1 is exiting statement, don't return it
+      return if exp.children.1?.type is like "ReturnStatement", "ThrowStatement"
       // At statement level, pipeline generates semicolons between statements
       // Return the last one
       semi := exp.children.lastIndexOf ";"
@@ -594,6 +596,8 @@ function insertReturn(node: ASTNode): void
       // NOTE: do not insert a return in the finally block
       return
     when "PipelineExpression"
+      // Child 0 is whitespace; if child 1 is exiting statement, don't return it
+      return if exp.children.1?.type is like "ReturnStatement", "ThrowStatement"
       // At statement level, pipeline generates semicolons between statements
       // Return the last one
       semi := exp.children.lastIndexOf ";"

diff --git a/source/parser/pipe.civet b/source/parser/pipe.civet
@@ -127,146 +127,142 @@ function constructPipeStep(fn: ExprWithComments, arg: ASTNode!, returning: ASTNo
 // body: [ws, pipe, ws, expr][]
 
 function processPipelineExpressions(statements): void
-  gatherRecursiveAll(statements, (n) => n.type is "PipelineExpression")
-    .forEach (s) =>
-      [ws, , body] := s.children
-      [, arg] .= s.children
-
-      i .= 0
-      l := body.length
-
-      children := [ws]
-      comma := blockContainingStatement(s) ? ";" : ","
-
-      let usingRef = null
-
-      for each step, i of body
-        const [leadingComment, pipe, trailingComment, expr] = step
-        const returns = pipe.token is "||>"
-        let ref, result,
-          returning = returns ? arg : null
-
-        if pipe.token is "|>="
-          let initRef
-          if i is 0
-            checkValidLHS arg
-
-            :outer switch arg.type
-              when "MemberExpression"
-                // If there is only a single access then we don't need a ref
-                break if arg.children.length <= 2
-                continue switch
-              when "CallExpression"
-                const access = arg.children.pop()
-                // processAssignments will check that this is a valid
-                // last access to assign to
-
-                usingRef = makeRef()
-                initRef = {
-                  type: "AssignmentExpression"
-                  children: [usingRef, " = ", arg, comma]
-                }
-
-                arg = {
-                  type: "MemberExpression"
-                  children: [usingRef, access]
-                }
-
-            // assignment node
-            lhs := [[
-              [initRef]
-              arg
-              []
-              { token: "=", children: [" = "] }
-            ]]
-
-            Object.assign s, {
-              type: "AssignmentExpression"
-              children: [lhs, children]
-              names: null
-              lhs
-              assigned: arg
-              expression: children
-            } satisfies AssignmentExpression
-
-            // Clone so that the same node isn't on the left and right because splice manipulation
-            // moves things around and can cause a loop in the graph
-            arg = clone arg
-            removeHoistDecs arg
-
-            // except keep the ref the same
-            if arg.children[0].type is "Ref"
-              arg.children[0] = usingRef
-
-          else
-            children.unshift({
-              type: "Error",
-              $loc: pipe.token.$loc,
-              message: "Can't use |>= in the middle of a pipeline",
-            })
-        else
-          if (i is 0) s.children = children
-
-        if returns and (ref = needsRef(arg))
-          // Use the existing ref if present
-          usingRef = usingRef or ref
-          arg = {
-            type: "ParenthesizedExpression",
-            children: ["(", {
-              type: "AssignmentExpression"
-              children: [usingRef, " = ", arg]
-            }, ")"],
-          }
-          returning = usingRef
+  for each s of gatherRecursiveAll statements, .type is "PipelineExpression"
+    [ws, , body] := s.children
+    [, arg] .= s.children
+
+    children := [ws]
+    comma := blockContainingStatement(s) ? ";" : ","
+
+    let usingRef = null
+
+    for each step, i of body
+      const [leadingComment, pipe, trailingComment, expr] = step
+      const returns = pipe.token is "||>"
+      let ref, result,
+        returning = returns ? arg : null
+
+      if pipe.token is "|>="
+        let initRef
+        if i is 0
+          checkValidLHS arg
+
+          :outer switch arg.type
+            when "MemberExpression"
+              // If there is only a single access then we don't need a ref
+              break if arg.children.length <= 2
+              continue switch
+            when "CallExpression"
+              const access = arg.children.pop()
+              // processAssignments will check that this is a valid
+              // last access to assign to
+
+              usingRef = makeRef()
+              initRef = {
+                type: "AssignmentExpression"
+                children: [usingRef, " = ", arg, comma]
+              }
+
+              arg = {
+                type: "MemberExpression"
+                children: [usingRef, access]
+              }
+
+          // assignment node
+          lhs := [[
+            [initRef]
+            arg
+            []
+            { token: "=", children: [" = "] }
+          ]]
+
+          Object.assign s, {
+            type: "AssignmentExpression"
+            children: [lhs, children]
+            names: null
+            lhs
+            assigned: arg
+            expression: children
+          } satisfies AssignmentExpression
+
+          // Clone so that the same node isn't on the left and right because splice manipulation
+          // moves things around and can cause a loop in the graph
+          arg = clone arg
+          removeHoistDecs arg
+
+          // except keep the ref the same
+          if arg.children[0].type is "Ref"
+            arg.children[0] = usingRef
 
-        [result, returning] = constructPipeStep
-          {
-            leadingComment: skipIfOnlyWS(leadingComment)
-            trailingComment: skipIfOnlyWS(trailingComment)
-            expr
-          }
-          arg
-          returning
-
-        if result.type is "ReturnStatement"
-          // Attach errors/warnings if there are more steps
-          if i < l - 1
-            result.children.push({
-              type: "Error",
-              message: "Can't continue a pipeline after returning",
-            })
-          arg = result
-          if children.-1 is ","
-            children.pop()
-            children.push(";")
-          break
-
-        if returning
-          arg = returning
-          children.push(result, comma)
         else
-          arg = result
-
-      if usingRef
-        s.hoistDec = {
-          type: "Declaration",
-          children: ["let ", usingRef],
-          names: [],
+          children.unshift({
+            type: "Error",
+            $loc: pipe.token.$loc,
+            message: "Can't use |>= in the middle of a pipeline",
+          })
+      else
+        if (i is 0) s.children = children
+
+      if returns and (ref = needsRef(arg))
+        // Use the existing ref if present
+        usingRef = usingRef or ref
+        arg = {
+          type: "ParenthesizedExpression",
+          children: ["(", {
+            type: "AssignmentExpression"
+            children: [usingRef, " = ", arg]
+          }, ")"],
         }
+        returning = usingRef
 
-      children.push(arg)
-
-      // Wrap with parens because comma operator has low precedence
-      if !children.some(?.type is "ReturnStatement") and children.some & is ","
-        { parent } := s
-        parenthesizedExpression := makeLeftHandSideExpression { ...s }
-        Object.assign s, parenthesizedExpression, {
-          parent
-          hoistDec: undefined
+      [result, returning] = constructPipeStep
+        {
+          leadingComment: skipIfOnlyWS(leadingComment)
+          trailingComment: skipIfOnlyWS(trailingComment)
+          expr
         }
+        arg
+        returning
 
-      // Update parent pointers
-      addParentPointers(s, s.parent)
+      if result.type is "ReturnStatement"
+        // Attach errors/warnings if there are more steps
+        if i < body.length - 1
+          result.children.push({
+            type: "Error",
+            message: "Can't continue a pipeline after returning",
+          })
+        arg = result
+        if children.-1 is ","
+          children.pop()
+          children.push(";")
+        break
+
+      if returning
+        arg = returning
+        children.push(result, comma)
+      else
+        arg = result
+
+    if usingRef
+      s.hoistDec = {
+        type: "Declaration",
+        children: ["let ", usingRef],
+        names: [],
+      }
+
+    children.push(arg)
+
+    // Wrap with parens because comma operator has low precedence
+    if !children.some(?.type is "ReturnStatement") and children.some & is ","
+      { parent } := s
+      parenthesizedExpression := makeLeftHandSideExpression { ...s }
+      Object.assign s, parenthesizedExpression, {
+        parent
+        hoistDec: undefined
+      }
+
+    // Update parent pointers
+    addParentPointers(s, s.parent)
 
 export {
   processPipelineExpressions

diff --git a/test/pipe.civet b/test/pipe.civet
@@ -182,6 +182,18 @@ describe "pipe", ->
     throw new Error
   """
 
+  testCase """
+    throw statement in pipeline in function
+    ---
+    function f()
+      new Error
+      |> throw
+    ---
+    function f() {
+      throw new Error
+    }
+  """
+
   testCase """
     throw expression in pipeline
     ---
@@ -218,6 +230,19 @@ describe "pipe", ->
     return foo(x + 1)
   """
 
+  testCase """
+    return in pipeline in function
+    ---
+    function f()
+      x + 1
+      |> foo
+      |> return
+    ---
+    function f() {
+      return foo(x + 1)
+    }
+  """
+
   testCase """
     return.value in pipeline
     ---