Fix location for optional params in arrow functions (#9998)

* Fix location with optional params in arrow functions

* add test

* Ensure rollup replaces NODE_ENV and create sourcemap in dev

* Ensure finishNod*() is never called twice on a node

* Fix check for already finished nodes

Gulpfile.js

@@ -15,6 +15,7 @@ const merge = require("merge-stream");
 const rollup = require("rollup");
 const rollupBabel = require("rollup-plugin-babel");
 const rollupNodeResolve = require("rollup-plugin-node-resolve");
+const rollupReplace = require("rollup-plugin-replace");
 const { registerStandalonePackageTask } = require("./scripts/gulp-tasks");
 
 const sources = ["codemods", "packages"];
@@ -92,6 +93,9 @@ function buildRollup(packages) {
         .rollup({
           input,
           plugins: [
+            rollupReplace({
+              "process.env.NODE_ENV": JSON.stringify(process.env.NODE_ENV),
+            }),
             rollupBabel({
               envName: "babel-parser",
             }),
@@ -103,6 +107,7 @@ function buildRollup(packages) {
             file: path.join(pkg, "lib/index.js"),
             format: "cjs",
             name: "babel-parser",
+            sourcemap: process.env.NODE_ENV !== "production",
           });
         });
     })

Makefile

@@ -124,7 +124,7 @@ prepublish-build:
 	rm -rf packages/babel-runtime/helpers
 	rm -rf packages/babel-runtime-corejs2/helpers
 	rm -rf packages/babel-runtime-corejs2/core-js
-	BABEL_ENV=production make build-dist
+	NODE_ENV=production BABEL_ENV=production make build-dist
 	make clone-license
 
 prepublish:

package.json

@@ -62,6 +62,7 @@
     "rollup": "^1.6.0",
     "rollup-plugin-babel": "^4.0.0",
     "rollup-plugin-node-resolve": "^4.0.1",
+    "rollup-plugin-replace": "^2.2.0",
     "test262-stream": "^1.2.0",
     "through2": "^2.0.0",
     "warnings-to-errors-webpack-plugin": "^2.0.0",

packages/babel-parser/src/parser/node.js

@@ -83,6 +83,12 @@ export class NodeUtils extends UtilParser {
     pos: number,
     loc: Position,
   ): T {
+    if (process.env.NODE_ENV !== "production" && node.end > 0) {
+      throw new Error(
+        "Do not call finishNode*() twice on the same node." +
+          " Instead use resetEndLocation() or change type directly.",
+      );
+    }
     node.type = type;
     node.end = pos;
     node.loc.end = loc;
@@ -97,6 +103,16 @@ export class NodeUtils extends UtilParser {
     if (this.options.ranges) node.range[0] = start;
   }
 
+  resetEndLocation(
+    node: NodeBase,
+    end?: number = this.state.lastTokEnd,
+    endLoc?: Position = this.state.lastTokEndLoc,
+  ): void {
+    node.end = end;
+    node.loc.end = endLoc;
+    if (this.options.ranges) node.range[1] = end;
+  }
+
   /**
    * Reset the start location of node to the start location of locationNode
    */

packages/babel-parser/src/plugins/flow.js

@@ -222,8 +222,7 @@ export default (superClass: Class<Parser>): Class<Parser> =>
 
       id.typeAnnotation = this.finishNode(typeContainer, "TypeAnnotation");
 
-      this.finishNode(id, id.type);
+      this.resetEndLocation(id);
-
       this.semicolon();
 
       return this.finishNode(node, "DeclareFunction");
@@ -432,7 +431,9 @@ export default (superClass: Class<Parser>): Class<Parser> =>
     ): N.FlowDeclareTypeAlias {
       this.next();
       this.flowParseTypeAlias(node);
-      return this.finishNode(node, "DeclareTypeAlias");
+      // Don't do finishNode as we don't want to process comments twice
+      node.type = "DeclareTypeAlias";
+      return node;
     }
 
     flowParseDeclareOpaqueType(
@@ -440,7 +441,9 @@ export default (superClass: Class<Parser>): Class<Parser> =>
     ): N.FlowDeclareOpaqueType {
       this.next();
       this.flowParseOpaqueType(node, true);
-      return this.finishNode(node, "DeclareOpaqueType");
+      // Don't do finishNode as we don't want to process comments twice
+      node.type = "DeclareOpaqueType";
+      return node;
     }
 
     flowParseDeclareInterface(
@@ -1520,7 +1523,7 @@ export default (superClass: Class<Parser>): Class<Parser> =>
         : this.flowParseRestrictedIdentifier();
       if (this.match(tt.colon)) {
         ident.typeAnnotation = this.flowParseTypeAnnotation();
-        this.finishNode(ident, ident.type);
+        this.resetEndLocation(ident);
       }
       return ident;
     }
@@ -1528,12 +1531,13 @@ export default (superClass: Class<Parser>): Class<Parser> =>
     typeCastToParameter(node: N.Node): N.Node {
       node.expression.typeAnnotation = node.typeAnnotation;
 
-      return this.finishNodeAt(
+      this.resetEndLocation(
         node.expression,
-        node.expression.type,
         node.typeAnnotation.end,
         node.typeAnnotation.loc.end,
       );
+
+      return node.expression;
     }
 
     flowParseVariance(): ?N.FlowVariance {
@@ -1848,6 +1852,10 @@ export default (superClass: Class<Parser>): Class<Parser> =>
       node = super.parseParenItem(node, startPos, startLoc);
       if (this.eat(tt.question)) {
         node.optional = true;
+        // Include questionmark in location of node
+        // Don't use this.finishNode() as otherwise we might process comments twice and
+        // include already consumed parens
+        this.resetEndLocation(node);
       }
 
       if (this.match(tt.colon)) {
@@ -2205,7 +2213,7 @@ export default (superClass: Class<Parser>): Class<Parser> =>
       if (this.match(tt.colon)) {
         param.typeAnnotation = this.flowParseTypeAnnotation();
       }
-      this.finishNode(param, param.type);
+      this.resetEndLocation(param);
       return param;
     }
 
@@ -2393,7 +2401,7 @@ export default (superClass: Class<Parser>): Class<Parser> =>
       super.parseVarId(decl, kind);
       if (this.match(tt.colon)) {
         decl.id.typeAnnotation = this.flowParseTypeAnnotation();
-        this.finishNode(decl.id, decl.id.type);
+        this.resetEndLocation(decl.id); // set end position to end of type
       }
     }
 

packages/babel-parser/src/plugins/placeholders.js

@@ -71,8 +71,10 @@ export default (superClass: Class<Parser>): Class<Parser> =>
       node: N.Node,
       expectedNode: T,
     ): /*N.Placeholder<T>*/ MaybePlaceholder<T> {
+      const isFinished = !!(node.expectedNode && node.type === "Placeholder");
       node.expectedNode = expectedNode;
-      return this.finishNode(node, "Placeholder");
+
+      return isFinished ? node : this.finishNode(node, "Placeholder");
     }
 
     /* ============================================================ *

packages/babel-parser/src/plugins/typescript/index.js

@@ -422,7 +422,7 @@ export default (superClass: Class<Parser>): Class<Parser> =>
       this.expect(tt.bracketL);
       const id = this.parseIdentifier();
       id.typeAnnotation = this.tsParseTypeAnnotation();
-      this.finishNode(id, "Identifier"); // set end position to end of type
+      this.resetEndLocation(id); // set end position to end of type
 
       this.expect(tt.bracketR);
       node.parameters = [id];
@@ -1961,6 +1961,10 @@ export default (superClass: Class<Parser>): Class<Parser> =>
       node = super.parseParenItem(node, startPos, startLoc);
       if (this.eat(tt.question)) {
         node.optional = true;
+        // Include questionmark in location of node
+        // Don't use this.finishNode() as otherwise we might process comments twice and
+        // include already consumed parens
+        this.resetEndLocation(node);
       }
 
       if (this.match(tt.colon)) {
@@ -1974,7 +1978,7 @@ export default (superClass: Class<Parser>): Class<Parser> =>
         return this.finishNode(typeCastNode, "TSTypeCastExpression");
       }
 
-      return this.finishNode(node, node.type);
+      return node;
     }
 
     parseExportDeclaration(node: N.ExportNamedDeclaration): ?N.Declaration {
@@ -2095,7 +2099,7 @@ export default (superClass: Class<Parser>): Class<Parser> =>
       const type = this.tsTryParseTypeAnnotation();
       if (type) {
         decl.id.typeAnnotation = type;
-        this.finishNode(decl.id, decl.id.type); // set end position to end of type
+        this.resetEndLocation(decl.id); // set end position to end of type
       }
     }
 
@@ -2239,7 +2243,9 @@ export default (superClass: Class<Parser>): Class<Parser> =>
       }
       const type = this.tsTryParseTypeAnnotation();
       if (type) param.typeAnnotation = type;
-      return this.finishNode(param, param.type);
+      this.resetEndLocation(param);
+
+      return param;
     }
 
     toAssignable(
@@ -2401,12 +2407,13 @@ export default (superClass: Class<Parser>): Class<Parser> =>
     typeCastToParameter(node: N.TsTypeCastExpression): N.Node {
       node.expression.typeAnnotation = node.typeAnnotation;
 
-      return this.finishNodeAt(
+      this.resetEndLocation(
         node.expression,
-        node.expression.type,
         node.typeAnnotation.end,
         node.typeAnnotation.loc.end,
       );
+
+      return node.expression;
     }
 
     toReferencedList(

packages/babel-parser/test/fixtures/flow/optional-type/1/output.json

@@ -96,7 +96,7 @@
                 {
                   "type": "Identifier",
                   "start": 11,
-                  "end": 12,
+                  "end": 13,
                   "loc": {
                     "start": {
                       "line": 1,
@@ -104,7 +104,7 @@
                     },
                     "end": {
                       "line": 1,
-                      "column": 12
+                      "column": 13
                     },
                     "identifierName": "x"
                   },

packages/babel-parser/test/fixtures/flow/optional-type/3/output.json

@@ -96,7 +96,7 @@
                 {
                   "type": "Identifier",
                   "start": 11,
-                  "end": 12,
+                  "end": 13,
                   "loc": {
                     "start": {
                       "line": 1,
@@ -104,7 +104,7 @@
                     },
                     "end": {
                       "line": 1,
-                      "column": 12
+                      "column": 13
                     },
                     "identifierName": "x"
                   },

packages/babel-parser/test/fixtures/flow/optional-type/4/output.json

@@ -96,7 +96,7 @@
                 {
                   "type": "RestElement",
                   "start": 11,
-                  "end": 15,
+                  "end": 16,
                   "loc": {
                     "start": {
                       "line": 1,
@@ -104,7 +104,7 @@
                     },
                     "end": {
                       "line": 1,
-                      "column": 15
+                      "column": 16
                     }
                   },
                   "argument": {

packages/babel-parser/test/fixtures/typescript/cast/as-const/output.json

@@ -3951,7 +3951,7 @@
               "expression": {
                 "type": "UnaryExpression",
                 "start": 858,
-                "end": 862,
+                "end": 861,
                 "loc": {
                   "start": {
                     "line": 29,
@@ -3959,7 +3959,7 @@
                   },
                   "end": {
                     "line": 29,
-                    "column": 15
+                    "column": 14
                   }
                 },
                 "operator": "-",
@@ -4104,7 +4104,7 @@
                   {
                     "type": "NumericLiteral",
                     "start": 886,
-                    "end": 889,
+                    "end": 888,
                     "loc": {
                       "start": {
                         "line": 30,
@@ -4112,7 +4112,7 @@
                       },
                       "end": {
                         "line": 30,
-                        "column": 15
+                        "column": 14
                       }
                     },
                     "extra": {

packages/babel-parser/test/fixtures/typescript/regression/comments-disappearing/input.ts

@@ -0,0 +1,6 @@
+let fun = () => {
+    // one
+    // two
+    // three
+    return (1);
+}

packages/babel-parser/test/fixtures/typescript/regression/comments-disappearing/output.json

@@ -0,0 +1,259 @@
+{
+  "type": "File",
+  "start": 0,
+  "end": 70,
+  "loc": {
+    "start": {
+      "line": 1,
+      "column": 0
+    },
+    "end": {
+      "line": 6,
+      "column": 1
+    }
+  },
+  "program": {
+    "type": "Program",
+    "start": 0,
+    "end": 70,
+    "loc": {
+      "start": {
+        "line": 1,
+        "column": 0
+      },
+      "end": {
+        "line": 6,
+        "column": 1
+      }
+    },
+    "sourceType": "module",
+    "interpreter": null,
+    "body": [
+      {
+        "type": "VariableDeclaration",
+        "start": 0,
+        "end": 70,
+        "loc": {
+          "start": {
+            "line": 1,
+            "column": 0
+          },
+          "end": {
+            "line": 6,
+            "column": 1
+          }
+        },
+        "declarations": [
+          {
+            "type": "VariableDeclarator",
+            "start": 4,
+            "end": 70,
+            "loc": {
+              "start": {
+                "line": 1,
+                "column": 4
+              },
+              "end": {
+                "line": 6,
+                "column": 1
+              }
+            },
+            "id": {
+              "type": "Identifier",
+              "start": 4,
+              "end": 7,
+              "loc": {
+                "start": {
+                  "line": 1,
+                  "column": 4
+                },
+                "end": {
+                  "line": 1,
+                  "column": 7
+                },
+                "identifierName": "fun"
+              },
+              "name": "fun"
+            },
+            "init": {
+              "type": "ArrowFunctionExpression",
+              "start": 10,
+              "end": 70,
+              "loc": {
+                "start": {
+                  "line": 1,
+                  "column": 10
+                },
+                "end": {
+                  "line": 6,
+                  "column": 1
+                }
+              },
+              "id": null,
+              "generator": false,
+              "async": false,
+              "params": [],
+              "body": {
+                "type": "BlockStatement",
+                "start": 16,
+                "end": 70,
+                "loc": {
+                  "start": {
+                    "line": 1,
+                    "column": 16
+                  },
+                  "end": {
+                    "line": 6,
+                    "column": 1
+                  }
+                },
+                "body": [
+                  {
+                    "type": "ReturnStatement",
+                    "start": 57,
+                    "end": 68,
+                    "loc": {
+                      "start": {
+                        "line": 5,
+                        "column": 4
+                      },
+                      "end": {
+                        "line": 5,
+                        "column": 15
+                      }
+                    },
+                    "argument": {
+                      "type": "NumericLiteral",
+                      "start": 65,
+                      "end": 66,
+                      "loc": {
+                        "start": {
+                          "line": 5,
+                          "column": 12
+                        },
+                        "end": {
+                          "line": 5,
+                          "column": 13
+                        }
+                      },
+                      "extra": {
+                        "rawValue": 1,
+                        "raw": "1",
+                        "parenthesized": true,
+                        "parenStart": 64
+                      },
+                      "value": 1
+                    },
+                    "leadingComments": [
+                      {
+                        "type": "CommentLine",
+                        "value": " one",
+                        "start": 22,
+                        "end": 28,
+                        "loc": {
+                          "start": {
+                            "line": 2,
+                            "column": 4
+                          },
+                          "end": {
+                            "line": 2,
+                            "column": 10
+                          }
+                        }
+                      },
+                      {
+                        "type": "CommentLine",
+                        "value": " two",
+                        "start": 33,
+                        "end": 39,
+                        "loc": {
+                          "start": {
+                            "line": 3,
+                            "column": 4
+                          },
+                          "end": {
+                            "line": 3,
+                            "column": 10
+                          }
+                        }
+                      },
+                      {
+                        "type": "CommentLine",
+                        "value": " three",
+                        "start": 44,
+                        "end": 52,
+                        "loc": {
+                          "start": {
+                            "line": 4,
+                            "column": 4
+                          },
+                          "end": {
+                            "line": 4,
+                            "column": 12
+                          }
+                        }
+                      }
+                    ]
+                  }
+                ],
+                "directives": []
+              }
+            }
+          }
+        ],
+        "kind": "let"
+      }
+    ],
+    "directives": []
+  },
+  "comments": [
+    {
+      "type": "CommentLine",
+      "value": " one",
+      "start": 22,
+      "end": 28,
+      "loc": {
+        "start": {
+          "line": 2,
+          "column": 4
+        },
+        "end": {
+          "line": 2,
+          "column": 10
+        }
+      }
+    },
+    {
+      "type": "CommentLine",
+      "value": " two",
+      "start": 33,
+      "end": 39,
+      "loc": {
+        "start": {
+          "line": 3,
+          "column": 4
+        },
+        "end": {
+          "line": 3,
+          "column": 10
+        }
+      }
+    },
+    {
+      "type": "CommentLine",
+      "value": " three",
+      "start": 44,
+      "end": 52,
+      "loc": {
+        "start": {
+          "line": 4,
+          "column": 4
+        },
+        "end": {
+          "line": 4,
+          "column": 12
+        }
+      }
+    }
+  ]
+}

yarn.lock

@@ -1456,19 +1456,15 @@
   version "0.0.30"
   resolved "https://registry.yarnpkg.com/@types/escape-string-regexp/-/escape-string-regexp-0.0.30.tgz#8cfaf0b5d2e46943d6efd77d3f4d18bfa1c9f225"
 
-"@types/estree@*":
+"@types/estree@*", "@types/estree@0.0.39":
-  version "0.0.38"
+  version "0.0.39"
-  resolved "https://registry.yarnpkg.com/@types/estree/-/estree-0.0.38.tgz#c1be40aa933723c608820a99a373a16d215a1ca2"
+  resolved "https://registry.yarnpkg.com/@types/estree/-/estree-0.0.39.tgz#e177e699ee1b8c22d23174caaa7422644389509f"
+  integrity sha512-EYNwp3bU+98cpU4lAWYYL7Zz+2gryWH1qbdDTidVd6hkiR6weksdbMadyXKXNPEkQFhXM+hVO9ZygomHXp+AIw==
 
 "@types/estree@0.0.35":
   version "0.0.35"
   resolved "https://registry.yarnpkg.com/@types/estree/-/estree-0.0.35.tgz#8999974b34028686a8d61a719e61c138d3755107"
 
-"@types/estree@0.0.39":
-  version "0.0.39"
-  resolved "https://registry.yarnpkg.com/@types/estree/-/estree-0.0.39.tgz#e177e699ee1b8c22d23174caaa7422644389509f"
-  integrity sha512-EYNwp3bU+98cpU4lAWYYL7Zz+2gryWH1qbdDTidVd6hkiR6weksdbMadyXKXNPEkQFhXM+hVO9ZygomHXp+AIw==
-
 "@types/istanbul-lib-coverage@^1.1.0":
   version "1.1.0"
   resolved "https://registry.yarnpkg.com/@types/istanbul-lib-coverage/-/istanbul-lib-coverage-1.1.0.tgz#2cc2ca41051498382b43157c8227fea60363f94a"
@@ -1478,11 +1474,7 @@
   version "4.14.104"
   resolved "https://registry.yarnpkg.com/@types/lodash/-/lodash-4.14.104.tgz#53ee2357fa2e6e68379341d92eb2ecea4b11bb80"
 
-"@types/node@*":
+"@types/node@*", "@types/node@^11.9.5":
-  version "9.4.6"
-  resolved "https://registry.yarnpkg.com/@types/node/-/node-9.4.6.tgz#d8176d864ee48753d053783e4e463aec86b8d82e"
-
-"@types/node@^11.9.5":
   version "11.10.4"
   resolved "https://registry.yarnpkg.com/@types/node/-/node-11.10.4.tgz#3f5fc4f0f322805f009e00ab35a2ff3d6b778e42"
   integrity sha512-wa09itaLE8L705aXd8F80jnFpxz3Y1/KRHfKsYL2bPc0XF+wEWu8sR9n5bmeu8Ba1N9z2GRNzm/YdHcghLkLKg==
@@ -6304,6 +6296,13 @@ lru-cache@^4.0.1, lru-cache@^4.1.2, lru-cache@^4.1.3:
     pseudomap "^1.0.2"
     yallist "^2.1.2"
 
+magic-string@^0.25.2:
+  version "0.25.2"
+  resolved "https://registry.yarnpkg.com/magic-string/-/magic-string-0.25.2.tgz#139c3a729515ec55e96e69e82a11fe890a293ad9"
+  integrity sha512-iLs9mPjh9IuTtRsqqhNGYcZXGei0Nh/A4xirrsqW7c+QhKVFL2vm7U09ru6cHRD22azaP/wMDgI+HCqbETMTtg==
+  dependencies:
+    sourcemap-codec "^1.4.4"
+
 make-dir@^1.0.0, make-dir@^1.3.0:
   version "1.3.0"
   resolved "https://registry.yarnpkg.com/make-dir/-/make-dir-1.3.0.tgz#79c1033b80515bd6d24ec9933e860ca75ee27f0c"
@@ -7411,13 +7410,7 @@ pinkie@^2.0.0:
   version "2.0.4"
   resolved "https://registry.yarnpkg.com/pinkie/-/pinkie-2.0.4.tgz#72556b80cfa0d48a974e80e77248e80ed4f7f870"
 
-pirates@^4.0.0:
+pirates@^4.0.0, pirates@^4.0.1:
-  version "4.0.0"
-  resolved "https://registry.yarnpkg.com/pirates/-/pirates-4.0.0.tgz#850b18781b4ac6ec58a43c9ed9ec5fe6796addbd"
-  dependencies:
-    node-modules-regexp "^1.0.0"
-
-pirates@^4.0.1:
   version "4.0.1"
   resolved "https://registry.yarnpkg.com/pirates/-/pirates-4.0.1.tgz#643a92caf894566f91b2b986d2c66950a8e2fb87"
   integrity sha512-WuNqLTbMI3tmfef2TKxlQmAiLHKtFhlsCZnPIpuv2Ow0RDVO8lfy1Opf4NUzlMXLjPl+Men7AuVdX6TA+s+uGA==
@@ -8126,10 +8119,18 @@ rollup-plugin-node-resolve@^4.0.1:
     is-module "^1.0.0"
     resolve "^1.10.0"
 
-rollup-pluginutils@^2.3.0:
+rollup-plugin-replace@^2.2.0:
-  version "2.4.1"
+  version "2.2.0"
-  resolved "https://registry.yarnpkg.com/rollup-pluginutils/-/rollup-pluginutils-2.4.1.tgz#de43ab54965bbf47843599a7f3adceb723de38db"
+  resolved "https://registry.yarnpkg.com/rollup-plugin-replace/-/rollup-plugin-replace-2.2.0.tgz#f41ae5372e11e7a217cde349c8b5d5fd115e70e3"
-  integrity sha512-wesMQ9/172IJDIW/lYWm0vW0LiKe5Ekjws481R7z9WTRtmO59cqyM/2uUlxvf6yzm/fElFmHUobeQOYz46dZJw==
+  integrity sha512-/5bxtUPkDHyBJAKketb4NfaeZjL5yLZdeUihSfbF2PQMz+rSTEb8ARKoOl3UBT4m7/X+QOXJo3sLTcq+yMMYTA==
+  dependencies:
+    magic-string "^0.25.2"
+    rollup-pluginutils "^2.6.0"
+
+rollup-pluginutils@^2.3.0, rollup-pluginutils@^2.6.0:
+  version "2.7.1"
+  resolved "https://registry.yarnpkg.com/rollup-pluginutils/-/rollup-pluginutils-2.7.1.tgz#a7915ce8b12c177364784bf38a1590cc6c2c8250"
+  integrity sha512-3nRf3buQGR9qz/IsSzhZAJyoK663kzseps8itkYHr+Z7ESuaffEPfgRinxbCRA0pf0gzLqkNKkSb8aNVTq75NA==
   dependencies:
     estree-walker "^0.6.0"
     micromatch "^3.1.10"
@@ -8423,6 +8424,11 @@ source-map@~0.4.0, source-map@~0.4.2:
   dependencies:
     amdefine ">=0.0.4"
 
+sourcemap-codec@^1.4.4:
+  version "1.4.4"
+  resolved "https://registry.yarnpkg.com/sourcemap-codec/-/sourcemap-codec-1.4.4.tgz#c63ea927c029dd6bd9a2b7fa03b3fec02ad56e9f"
+  integrity sha512-CYAPYdBu34781kLHkaW3m6b/uUSyMOC2R61gcYMWooeuaGtjof86ZA/8T+qVPPt7np1085CR9hmMGrySwEc8Xg==
+
 sparkles@^1.0.0:
   version "1.0.0"
   resolved "https://registry.yarnpkg.com/sparkles/-/sparkles-1.0.0.tgz#1acbbfb592436d10bbe8f785b7cc6f82815012c3"