refactor: read_optional_expr (#405)

Author: evilebottnawi

src/parser/expr.js

@@ -192,6 +192,32 @@ module.exports = {
     return result;
   },
 
+  /**
+   * Reads optional expression
+   */
+  read_optional_expr: function(stopToken) {
+    if (this.token !== stopToken) {
+      return this.read_expr();
+    }
+
+    return null;
+  },
+
+  /**
+   * Reads exit expression
+   */
+  read_exit_expr: function() {
+    let expression = null;
+
+    if (this.token === "(") {
+      this.next();
+      expression = this.read_optional_expr(')');
+      this.expect(')') && this.next();
+    }
+
+    return expression;
+  },
+
   /**
    * ```ebnf
    * Reads an expression
@@ -320,17 +346,8 @@ module.exports = {
       case this.tok.T_EXIT: {
         const useDie = this.lexer.yytext.toLowerCase() === "die";
         result = this.node("exit");
-        let expression = null;
-        if (this.next().token === "(") {
-          if (this.next().token !== ")") {
-            expression = this.read_expr();
-            if (this.expect(")")) {
-              this.next();
-            }
-          } else {
-            this.next();
-          }
-        }
+        this.next();
+        const expression = this.read_exit_expr();
         return result(expression, useDie);
       }
 

src/parser/statement.js

@@ -212,10 +212,8 @@ module.exports = {
 
       case this.tok.T_RETURN: {
         const result = this.node("return");
-        let expr = null;
-        if (!this.next().is("EOS")) {
-          expr = this.read_expr();
-        }
+        this.next();
+        const expr = this.read_optional_expr(';');
         this.expectEndOfStatement();
         return result(expr);
       }
@@ -226,11 +224,8 @@ module.exports = {
         const result = this.node(
           this.token === this.tok.T_CONTINUE ? "continue" : "break"
         );
-        let level = null;
-        this.next(); // look ahead
-        if (this.token !== ";") {
-          level = this.read_expr();
-        }
+        this.next();
+        const level = this.read_optional_expr(';');
         this.expectEndOfStatement();
         return result(level);
       }

test/snapshot/__snapshots__/break.test.js.snap

@@ -48,6 +48,27 @@ Program {
 }
 `;
 
+exports[`break should fail when no ';' at end 1`] = `
+Program {
+  "children": Array [
+    Break {
+      "kind": "break",
+      "level": undefined,
+    },
+  ],
+  "errors": Array [
+    Error {
+      "expected": "EXPR",
+      "kind": "error",
+      "line": 1,
+      "message": "Parse Error : syntax error on line 1",
+      "token": "the end of file (EOF)",
+    },
+  ],
+  "kind": "program",
+}
+`;
+
 exports[`break simple 1`] = `
 Program {
   "children": Array [

test/snapshot/__snapshots__/continue.test.js.snap

@@ -48,6 +48,27 @@ Program {
 }
 `;
 
+exports[`continue should fail when no ';' at end 1`] = `
+Program {
+  "children": Array [
+    Continue {
+      "kind": "continue",
+      "level": undefined,
+    },
+  ],
+  "errors": Array [
+    Error {
+      "expected": "EXPR",
+      "kind": "error",
+      "line": 1,
+      "message": "Parse Error : syntax error on line 1",
+      "token": "the end of file (EOF)",
+    },
+  ],
+  "kind": "program",
+}
+`;
+
 exports[`continue simple 1`] = `
 Program {
   "children": Array [

test/snapshot/__snapshots__/return.test.js.snap

@@ -13,6 +13,27 @@ Program {
 }
 `;
 
+exports[`return should fail when no ';' at end 1`] = `
+Program {
+  "children": Array [
+    Return {
+      "expr": undefined,
+      "kind": "return",
+    },
+  ],
+  "errors": Array [
+    Error {
+      "expected": "EXPR",
+      "kind": "error",
+      "line": 1,
+      "message": "Parse Error : syntax error on line 1",
+      "token": "the end of file (EOF)",
+    },
+  ],
+  "kind": "program",
+}
+`;
+
 exports[`return simple 1`] = `
 Program {
   "children": Array [

test/snapshot/break.test.js

@@ -1,23 +1,30 @@
-const parser = require('../main');
+const parser = require("../main");
 
-describe('break', () => {
-  it('simple', () => {
-    expect(parser.parseEval('break;')).toMatchSnapshot();
+describe("break", () => {
+  it("simple", () => {
+    expect(parser.parseEval("break;")).toMatchSnapshot();
   });
-  it('argument 0', () => {
-    expect(parser.parseEval('break 0;')).toMatchSnapshot();
+  it("argument 0", () => {
+    expect(parser.parseEval("break 0;")).toMatchSnapshot();
   });
-  it('argument 1', () => {
-    expect(parser.parseEval('break 1;')).toMatchSnapshot();
+  it("argument 1", () => {
+    expect(parser.parseEval("break 1;")).toMatchSnapshot();
   });
-  it('argument 2', () => {
-    expect(parser.parseEval('break 2;')).toMatchSnapshot();
+  it("argument 2", () => {
+    expect(parser.parseEval("break 2;")).toMatchSnapshot();
   });
-  it('with parens', () => {
-    expect(parser.parseEval('break (1);')).toMatchSnapshot();
+  it("with parens", () => {
+    expect(parser.parseEval("break (1);")).toMatchSnapshot();
   });
   // Deprecated since 5.4.0
-  it('with expression', () => {
-    expect(parser.parseEval('break $var;')).toMatchSnapshot();
+  it("with expression", () => {
+    expect(parser.parseEval("break $var;")).toMatchSnapshot();
+  });
+  it("should fail when no ';' at end", function() {
+    expect(
+      parser.parseEval("break", {
+        parser: { suppressErrors: true }
+      })
+    ).toMatchSnapshot();
   });
 });

test/snapshot/continue.test.js

@@ -1,23 +1,30 @@
-const parser = require('../main');
+const parser = require("../main");
 
-describe('continue', () => {
-  it('simple', () => {
-    expect(parser.parseEval('continue;')).toMatchSnapshot();
+describe("continue", () => {
+  it("simple", () => {
+    expect(parser.parseEval("continue;")).toMatchSnapshot();
   });
-  it('argument 0', () => {
-    expect(parser.parseEval('continue 0;')).toMatchSnapshot();
+  it("argument 0", () => {
+    expect(parser.parseEval("continue 0;")).toMatchSnapshot();
   });
-  it('argument 1', () => {
-    expect(parser.parseEval('continue 1;')).toMatchSnapshot();
+  it("argument 1", () => {
+    expect(parser.parseEval("continue 1;")).toMatchSnapshot();
   });
-  it('argument 2', () => {
-    expect(parser.parseEval('continue 2;')).toMatchSnapshot();
+  it("argument 2", () => {
+    expect(parser.parseEval("continue 2;")).toMatchSnapshot();
   });
-  it('with parens', () => {
-    expect(parser.parseEval('continue (1);')).toMatchSnapshot();
+  it("with parens", () => {
+    expect(parser.parseEval("continue (1);")).toMatchSnapshot();
   });
   // Deprecated since 5.4.0
-  it('with expression', () => {
-    expect(parser.parseEval('continue $var;')).toMatchSnapshot();
+  it("with expression", () => {
+    expect(parser.parseEval("continue $var;")).toMatchSnapshot();
+  });
+  it("should fail when no ';' at end", function() {
+    expect(
+      parser.parseEval("continue", {
+        parser: { suppressErrors: true }
+      })
+    ).toMatchSnapshot();
   });
 });

test/snapshot/return.test.js

@@ -1,10 +1,17 @@
-const parser = require('../main');
+const parser = require("../main");
 
 describe("return", function() {
   it("simple", function() {
     expect(parser.parseEval('return "string";')).toMatchSnapshot();
   });
   it("no expression", function() {
-    expect(parser.parseEval('return;')).toMatchSnapshot();
+    expect(parser.parseEval("return;")).toMatchSnapshot();
+  });
+  it("should fail when no ';' at end", function() {
+    expect(
+      parser.parseEval("return", {
+        parser: { suppressErrors: true }
+      })
+    ).toMatchSnapshot();
   });
 });