Merge pull request #1615 from DanielXMoore/switch-return

Fix fallthrough in implicitly returned switch with semicolon, improve `hasExit` heuristic

source/parser.hera

@@ -30,6 +30,7 @@ import {
   getPrecedence,
   getTrimmingSpace,
   hasAwait,
+  hasTrailingComment,
   hasYield,
   insertTrimmingSpace,
   isComma,
@@ -2801,13 +2802,8 @@ SingleLineStatements
     const expressions = [...stmts]
     if (last) expressions.push(last)
 
-    // check for trailing comment
-    const maybeComment = expressions.at(-1)?.[2]?.children?.[2]?.at(-1)
-    const hasTrailingComment =
-      maybeComment?.type === "Comment" && maybeComment.token.startsWith("//")
-
     const children = [expressions]
-    if (hasTrailingComment) children.push("\n")
+    if (hasTrailingComment(expressions)) children.push(["\n"])
 
     return {
       type: "BlockStatement",

source/parser/function.civet

@@ -62,6 +62,7 @@ import {
   deepCopy
   getTrimmingSpace
   hasAwait
+  hasTrailingComment
   hasYield
   inplacePrepend
   isEmptyBareBlock
@@ -275,13 +276,10 @@ function processReturnValue(func: FunctionNode)
 
   // Implicit return before }
   unless block.children.-2?.type is "ReturnStatement"
-    indent := getIndent(block.expressions.-1) or ";"
+    indent := getIndent block.expressions.-1
     block.expressions.push [
-      [indent]
-    ,
-      type: "ReturnStatement",
-      expression: ref,
-      children: ["return ", ref]
+      indent
+      wrapWithReturn ref, block, not indent
     ]
 
   return true
@@ -443,13 +441,15 @@ function assignResults(node: StatementTuple[] | ASTNode, collect: (node: ASTNode
       return
     when "SwitchStatement"
       // insert a results.push in each case block
-      assignResults(exp.children[2], collect)
+      for each clause of exp.caseBlock.clauses
+        assignResults clause, collect
       return
     when "TryStatement"
       // NOTE: CoffeeScript doesn't add a push to an empty catch block but does add if there is any statement in the catch block
       // we always add a push to the catch block
       // NOTE: does not insert a push in the finally block
-      exp.blocks.forEach((block) => assignResults(block, collect))
+      for each block of exp.blocks
+        assignResults block, collect
       return
 
   // Don't push if there's a trailing semicolon
@@ -464,23 +464,31 @@ function insertReturn(node: ASTNode): void
   // TODO: unify this with the `exp` switch
   switch node.type
     when "BlockStatement"
-      if node.expressions.length
+      if node.expressions#
         return if node.expressions.some ([, exp]) => isExit exp
         last := node.expressions[node.expressions.length - 1]
         insertReturn(last)
       else
         // NOTE: Kind of hacky but I'm too much of a coward to make `->` add an implicit return
-        if node.parent.type is "CatchClause"
-          node.expressions.push(["return"])
+        if node.parent?.type is like "CatchClause", "WhenClause"
+          node.expressions.push ["", wrapWithReturn(undefined, node)]
       return
     // NOTE: "CaseClause"s don't get a return statement inserted
     when "WhenClause"
       // Remove inserted `break;` if it hasn't already been removed
-      node.children.splice node.children.indexOf(node.break), 1 if node.break
-      if node.block.expressions.length
-        insertReturn(node.block)
-      else
-        node.block.expressions.push(wrapWithReturn())
+      if node.break
+        breakIndex := node.children.indexOf node.break
+        assert.notEqual breakIndex, -1, "Could not find break in when clause"
+        node.children.splice breakIndex, 1
+        node.break = undefined
+      // Then try to add implicit return
+      insertReturn node.block
+      unless isExit node.block
+        comment := hasTrailingComment node.block.expressions
+        node.block.expressions.push [
+          comment ? node.block.expressions.-1.0 or "\n" : ""
+          wrapWithReturn undefined, node, not comment
+        ]
       return
     when "DefaultClause"
       insertReturn(node.block)
@@ -507,7 +515,7 @@ function insertReturn(node: ASTNode): void
       parent := outer.parent as BlockStatement?
       index := findChildIndex parent?.expressions, outer
       assert.notEqual index, -1, "Could not find declaration in parent"
-      parent!.expressions.splice index+1, 0, ["", {
+      parent!.expressions.splice index+1, 0, ["",
         type: "ReturnStatement"
         expression: value
         children: [
@@ -516,7 +524,7 @@ function insertReturn(node: ASTNode): void
           value
         ]
         parent: exp
-      }]
+      ]
       braceBlock parent!
       return
     when "FunctionExpression"
@@ -526,10 +534,7 @@ function insertReturn(node: ASTNode): void
         index := findChildIndex parent?.expressions, outer
         assert.notEqual index, -1, "Could not find function declaration in parent"
         parent!.expressions.splice index+1, 0, ["",
-          type: "ReturnStatement"
-          expression: exp.id
-          children: [";return ", exp.id]
-          parent: exp
+          wrapWithReturn exp.id, exp, true
         ]
         braceBlock parent!
         return
@@ -549,37 +554,31 @@ function insertReturn(node: ASTNode): void
       // else block
       if (exp.else) insertReturn(exp.else.block)
       // Add explicit return after if block if no else block
-      else exp.children.push ["", {
-        type: "ReturnStatement"
-        // NOTE: add a prefixed semi-colon because the if block may not be braced
-        children: [";return"]
-        parent: exp
-      }]
+      else exp.children.push ["",
+        // NOTE: add a prefixed semicolon because the if block may not be braced
+        wrapWithReturn undefined, exp, true
+      ]
       return
     when "PatternMatchingStatement"
-      insertReturn(exp.children[0])
+      insertReturn exp.children[0]
       return
     when "SwitchStatement"
-      insertSwitchReturns(exp)
+      // insert a return in each when/else/default block
+      // case blocks don't get implicit returns
+      for each clause of exp.caseBlock.clauses
+        insertReturn clause
       return
     when "TryStatement"
-      exp.blocks.forEach((block) => insertReturn(block))
+      for each block of exp.blocks
+        insertReturn block
       // NOTE: do not insert a return in the finally block
       return
 
   // Don't add return if there's a trailing semicolon
   return if node.-1?.type is "SemicolonDelimiter"
 
   // Insert return after indentation and before expression
-  const returnStatement = wrapWithReturn(node[1])
-  node.splice(1, 1, returnStatement)
-
-// insert a return in each when/else/default block
-// case blocks don't get implicit returns
-// maybe default blocks shouldn't either?
-function insertSwitchReturns(exp): void
-  exp.caseBlock.clauses.forEach (clause) =>
-    insertReturn clause
+  node[1] = wrapWithReturn node[1]
 
 // Process `break with` and `continue with` within a loop statement
 // that already has a resultsRef attribute.

source/parser/lib.civet

@@ -61,6 +61,7 @@ import {
   hasAwait
   hasExportDeclaration
   hasImportDeclaration
+  hasTrailingComment
   hasYield
   inplaceInsertTrimmingSpace
   inplacePrepend
@@ -1797,6 +1798,7 @@ export {
   hasAwait
   hasExportDeclaration
   hasImportDeclaration
+  hasTrailingComment
   hasYield
   insertTrimmingSpace
   isComma

source/parser/types.civet

@@ -28,6 +28,7 @@ export type StatementNode =
   | ImportDeclaration
   | IterationStatement
   | LabelledStatement
+  | PatternMatchingStatement
   | ReturnStatement
   | SwitchStatement
   | ThrowStatement
@@ -452,6 +453,13 @@ export type CaseClause
   cases: ASTNode[]
   block: BlockStatement
 
+export type PatternMatchingStatement
+  type: "PatternMatchingStatement"
+  children: Children & [StatementTuple]
+  parent?: Parent
+  condition: Condition
+  caseBlock: CaseBlock
+
 export type WhenClause
   type: "WhenClause"
   children: Children

source/parser/util.civet

@@ -9,6 +9,7 @@ import type {
   IsToken
   IterationStatement
   Literal
+  Parent
   StatementNode
   TypeSuffix
   ReturnTypeAnnotation
@@ -192,9 +193,25 @@ function isExit(node: ASTNode): boolean
       true
     // if checks then and else clause
     when "IfStatement"
+      (or)
+        // `insertReturn` for IfStatement adds a return to children
+        // when there's no else block
+        node.children.-1?.type is "ReturnStatement"
+        (node.children.-1 as StatementTuple)?[1]?.type is "ReturnStatement"
+        (and)
+          isExit node.then
+          isExit node.else?.block
+    when "PatternMatchingStatement"
+      isExit node.children[0][1]
+    when "SwitchStatement"
       (and)
-        isExit node.then
-        isExit node.else?.block
+        // Ensure exhaustive by requiring an else/default clause
+        node.caseBlock.clauses.some .type is "DefaultClause"
+        // Every clause should exit
+        node.caseBlock.clauses.every isExit
+    when "TryStatement"
+      // Require all non-finally blocks to exit
+      node.blocks.every isExit
     when "BlockStatement"
       // [1] extracts statement from [indent, statement, delim]
       node.expressions.some (s) => isExit s[1]
@@ -235,6 +252,15 @@ function stripTrailingImplicitComma(children: ASTNode[])
   else
     children
 
+function hasTrailingComment(node: ASTNode): boolean
+  return false unless node?
+  return true if node.type is "Comment"
+  if Array.isArray node
+    return hasTrailingComment node.-1
+  if "children" in node
+    return hasTrailingComment (node.children as Children).-1
+  false
+
 /**
  * Trims the first single space from the spacing array or node's children if present
  * Inserts string `c` in the first position.
@@ -788,14 +814,15 @@ function wrapIIFE(expressions: StatementTuple[], asyncFlag?: boolean, generator?
 
   return exp
 
-function wrapWithReturn(expression?: ASTNode): ASTNode
-  const children = expression ? ["return ", expression] : ["return"]
+function wrapWithReturn(expression?: ASTNode, parent: Parent = expression?.parent, semi = false): ASTNode
+  children := expression ? ["return ", expression] : ["return"]
+  children.unshift ";" if semi
 
   return makeNode {
     type: "ReturnStatement"
     children
     expression
-    parent: expression?.parent
+    parent
   }
 
 function flatJoin<T, S>(array: T[][], separator: S): (T | S)[]
@@ -819,6 +846,7 @@ export {
   hasAwait
   hasExportDeclaration
   hasImportDeclaration
+  hasTrailingComment
   hasYield
   inplaceInsertTrimmingSpace
   inplacePrepend

test/switch.civet

@@ -461,6 +461,43 @@ describe "switch", ->
     }
   """
 
+  testCase """
+    don't fall through after omitted implicit return
+    ---
+    =>
+      switch foo
+        when 'empty'
+        when 'value'
+          1
+        when 'semi'
+          ; // omitted `return`
+        when 'multi'
+          a;b; // omitted `return`
+        else
+          0
+    ---
+    () => {
+      switch(foo) {
+        case 'empty': {return
+        }
+        case 'value': {
+          return 1
+        }
+        case 'semi': {
+          ; // omitted `return`
+          return
+        }
+        case 'multi': {
+          a;b; // omitted `return`
+    return
+        }
+        default: {
+          return 0
+        }
+      }
+    }
+  """
+
   // #1118
   testCase """
     break after pattern matching statement