Fix scope of function body when var redeclares param

[openorclose]

Q	A
Fixed Issues?	Fixes #10471 Fixes #10798
Patch: Bug Fix?	Y
Major: Breaking Change?
Minor: New Feature?
Tests Added + Pass?	Yes
Documentation PR Link
Any Dependency Changes?	No
License	MIT

The below is valid js, but babel currently throws a redeclaration error:

function f(a = 1) {
  var a = 1;
}

Link: https://babeljs.io/repl#?browsers=%3E0.5%25%2C%20not%20ie%20%3C%3D11%2C%20not%20safari%205.1%2C%20not%20samsung%204&build=&builtIns=false&spec=false&loose=false&code_lz=GYVwdgxgLglg9mABMAFAQ0QXkQRgJSIDeAUIogG5oBOiG2OA3MQL5A&debug=false&forceAllTransforms=false&shippedProposals=false&circleciRepo=&evaluate=false&fileSize=false&timeTravel=false&sourceType=module&lineWrap=true&presets=env%2Ces2015%2Cenv&prettier=false&targets=&version=7.8.4&externalPlugins=%40babel%2Fplugin-transform-react-jsx%407.8.3

babel-traverse registers a var or function declaration that redeclare a function parameter as a constant violation.

So now during function parameter transformation, if we encounter a parameter whose bindings contain constant violations, we check if they are var or function declarations. If we find empty var declarations (e.g. var x;), we delete them since such declarations do not override the parameter. If not, so we have var declarations with initalisers or function declarations, and we need to wrap the function body in an iife to allow these differently scoped declarations.

However callDelegate always hoists up these var declarations into the real function scope instead of letting them remain in the IIFE function scope, so let's add another parameter to callDelegate that allows us to prevent this hoisting.

We only need to check for var and function declarations redefinition, because redeclaration of parameters via const or let is a parse error.

[openorclose]

On a separate note, there isn't a "function parameter scope" property on a Function, so parameters of the function and local variables of the function all get lumped together in the same scope, so we have to choose between the current behaviour, where params take precedence over function body vars, or this pr, where we let the vars "override" the function params.

[nicolo-ribaudo]

Can you also add this test, which I think should return 2?

function f(a = 2) {
  var a;
  return a;
}

[openorclose]

Can you also add this test, which I think should return 2?

function f(a = 2) {
  var a;
  return a;
}

Oh right, this pr will return the wrong value for the above.

[openorclose]

Can you also add this test, which I think should return 2?

function f(a = 2) {
  var a;
  return a;
}

Added and fixed, thanks for the test case!

Updated the implementation and the pr description, whenever we see an empty var declaration that clashes with a parameter it gets deleted.

[openorclose]

Can I get a review on this (I can't seem to request reviewers)?

[openorclose]

The following test failed when merging latest master.

Can this be updated?

Summary of all failing tests
 FAIL  packages/babel-preset-env/test/fixtures.js (5.254s)
  ● babel-preset-env/debug › browserslists last 2 versions not ie

    Expected /home/circleci/babel/packages/babel-preset-env/test/fixtures/debug/browserslists-last-2-versions-not-ie/stdout.txt to match transform output.
    To autogenerate a passing version of this file, delete the file and re-run the tests.

    Diff:

    - Expected
    + Received

    @@ -3,29 +3,29 @@
      Using targets:
      {
        "android": "79",
        "chrome": "79",
        "edge": "79",
    -   "firefox": "71",
    +   "firefox": "72",
        "ios": "13",
        "opera": "65",
        "safari": "12.1",
        "samsung": "9.2"
      }
      
      Using modules transform: auto
      
      Using plugins:
    -   proposal-nullish-coalescing-operator { "android":"79", "chrome":"79", "edge":"79", "firefox":"71", "ios":"13", "opera":"65", "safari":"12.1", "samsung":"9.2" }
    -   proposal-optional-chaining { "android":"79", "chrome":"79", "edge":"79", "firefox":"71", "ios":"13", "opera":"65", "safari":"12.1", "samsung":"9.2" }
    -   syntax-json-strings { "android":"79", "chrome":"79", "edge":"79", "firefox":"71", "ios":"13", "opera":"65", "safari":"12.1", "samsung":"9.2" }
    -   syntax-optional-catch-binding { "android":"79", "chrome":"79", "edge":"79", "firefox":"71", "ios":"13", "opera":"65", "safari":"12.1", "samsung":"9.2" }
    -   syntax-async-generators { "android":"79", "chrome":"79", "edge":"79", "firefox":"71", "ios":"13", "opera":"65", "safari":"12.1", "samsung":"9.2" }
    -   syntax-object-rest-spread { "android":"79", "chrome":"79", "edge":"79", "firefox":"71", "ios":"13", "opera":"65", "safari":"12.1", "samsung":"9.2" }
    -   transform-dotall-regex { "firefox":"71" }
    -   proposal-unicode-property-regex { "firefox":"71" }
    -   transform-named-capturing-groups-regex { "firefox":"71" }
    +   proposal-nullish-coalescing-operator { "android":"79", "chrome":"79", "edge":"79", "ios":"13", "opera":"65", "safari":"12.1", "samsung":"9.2" }
    +   proposal-optional-chaining { "android":"79", "chrome":"79", "edge":"79", "firefox":"72", "ios":"13", "opera":"65", "safari":"12.1", "samsung":"9.2" }
    +   syntax-json-strings { "android":"79", "chrome":"79", "edge":"79", "firefox":"72", "ios":"13", "opera":"65", "safari":"12.1", "samsung":"9.2" }
    +   syntax-optional-catch-binding { "android":"79", "chrome":"79", "edge":"79", "firefox":"72", "ios":"13", "opera":"65", "safari":"12.1", "samsung":"9.2" }
    +   syntax-async-generators { "android":"79", "chrome":"79", "edge":"79", "firefox":"72", "ios":"13", "opera":"65", "safari":"12.1", "samsung":"9.2" }
    +   syntax-object-rest-spread { "android":"79", "chrome":"79", "edge":"79", "firefox":"72", "ios":"13", "opera":"65", "safari":"12.1", "samsung":"9.2" }
    +   transform-dotall-regex { "firefox":"72" }
    +   proposal-unicode-property-regex { "firefox":"72" }
    +   transform-named-capturing-groups-regex { "firefox":"72" }
        transform-template-literals { "safari":"12.1" }
    -   transform-modules-commonjs { "android":"79", "chrome":"79", "edge":"79", "firefox":"71", "ios":"13", "opera":"65", "safari":"12.1", "samsung":"9.2" }
    -   proposal-dynamic-import { "android":"79", "chrome":"79", "edge":"79", "firefox":"71", "ios":"13", "opera":"65", "safari":"12.1", "samsung":"9.2" }
    +   transform-modules-commonjs { "android":"79", "chrome":"79", "edge":"79", "firefox":"72", "ios":"13", "opera":"65", "safari":"12.1", "samsung":"9.2" }
    +   proposal-dynamic-import { "android":"79", "chrome":"79", "edge":"79", "firefox":"72", "ios":"13", "opera":"65", "safari":"12.1", "samsung":"9.2" }

[JLHwung]

@openorclose Can you rebase on the upstream master? It should be fixed now.

[openorclose]

@openorclose Can you rebase on the upstream master? It should be fixed now.

Rebased!

[existentialism]

Nice work @openorclose!

[openorclose]

Thanks for the reviews!

[nicolo-ribaudo]

Thanks!