Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

parameter initializer is bound to the wrong variable #22769

Closed
ajafff opened this issue Mar 21, 2018 · 10 comments
Closed

parameter initializer is bound to the wrong variable #22769

ajafff opened this issue Mar 21, 2018 · 10 comments
Labels
Effort: Moderate Requires experience with the TypeScript codebase, but feasible. Harder than "Effort: Casual". Fixed A PR has been merged for this issue Help Wanted You can do this Suggestion An idea for TypeScript
Milestone

Comments

@ajafff
Copy link
Contributor

ajafff commented Mar 21, 2018

TypeScript Version: 2.8.0-dev.20180321

Search Terms:

Code

let foo = 1;

(function (bar = foo) { // unexpected compiler error; works at runtime
    var foo = 2;
    return bar; // returns 1
})();

(function (bar = (baz = foo) => baz) { // unexpected compiler error; works at runtime
    var foo = 2;
    return bar(); // returns 1
})();

(function (bar = foo, foo = 2) { // correct compiler error, error at runtime
    return bar;
})();

Expected behavior:
No error on the first and second function expressions
This used to work in a previous release.

Actual behavior:
Compiler error on the first two function declarations even though the code works at runtime.

Initializer of parameter 'bar' cannot reference identifier 'foo' declared after it.

Variable 'foo' is used before being assigned.

Playground Link:

Related Issues:

@mhegazy
Copy link
Contributor

mhegazy commented Mar 26, 2018

Compiler error on the first two function declarations even though the code works at runtime.

The code as emitted by the TS compiler will not work. the first example for instance will result in bar having the value undefined and not 1.

To generate code that would work, the compiler will have to do the initialization in a function scope other than the function itself, effectively warping the function in an IIFE.

We choose a while back to not do that as it impacts the readability of the generated code, and instead we opted in adding an error instead.

This used to work in a previous release.

I do not think this ever worked. I went back to 1.8 and the same errors are reported.

@mhegazy mhegazy added the Working as Intended The behavior described is the intended behavior; this is not a bug label Mar 26, 2018
@ajafff
Copy link
Contributor Author

ajafff commented Mar 27, 2018

@mhegazy

The code as emitted by the TS compiler will not work. the first example for instance will result in bar having the value undefined and not 1.

That's only true if the target is ES3 or ES5. I'm currently targeting ES2015.

To generate code that would work, the compiler will have to do the initialization in a function scope other than the function itself, effectively warping the function in an IIFE.

Or you could rename all function scoped identifiers (that are used somewhere in parameter initializers) that shadow declarations outside of the function. (cc @rbuckton)
You already do similar renames for lexical declarations:

// foo.ts
var foo = 1;
{
    let foo = 2;
}

// foo.js
var foo = 1;
{
    var foo_1 = 2;
}

/**********************/

// fn.ts
let foo = 1;

(function (bar = foo) { // unexpected compiler error; works at runtime
    var foo = 2;
    return bar;
})();

// fn.js
var foo = 1;
(function (bar) {
    if (bar === void 0) { bar = foo; }
    var foo_1 = 2;
    return bar; // returns 1
})();

We choose a while back to not do that as it impacts the readability of the generated code, and instead we opted in adding an error instead.

May I ask you to reconsider this decision?

I find this rather concerning as it only applies to code transpiled to ES3 and ES5. It makes using ES2015 features difficult to use because it shows errors that are technically not correct. If I didn't know much ES2015 I would think that it's not possible at all as I assume TypeScript does the correct thing.

You also use this same implementation to power the JS language service which doesn't show errors by default. As a user I'm really confused when "Go to declaration" jumps to a different variable than what's used at runtime.

@RyanCavanaugh
Copy link
Member

Or you could rename all function scoped identifiers (that are used somewhere in parameter initializers) that shadow declarations outside of the function.

In the general case, we can't know what other outside variables in scope might exist.

@mhegazy
Copy link
Contributor

mhegazy commented Mar 27, 2018

That's only true if the target is ES3 or ES5. I'm currently targeting ES2015.

We can disable the whole check if we are in ES2015 or higher.

@mhegazy
Copy link
Contributor

mhegazy commented Mar 27, 2018

feel free to send us a PR for disabling the check for ES2015 or later.

@ajafff
Copy link
Contributor Author

ajafff commented Mar 27, 2018

Unfortunately it's not only the error message. The whole name lookup needs to be changed to make this work as expected.

@mhegazy
Copy link
Contributor

mhegazy commented Mar 27, 2018

U are right, we can change that too.

@mhegazy mhegazy added Suggestion An idea for TypeScript In Discussion Not yet reached consensus and removed Working as Intended The behavior described is the intended behavior; this is not a bug labels Mar 28, 2018
@mhegazy
Copy link
Contributor

mhegazy commented Mar 28, 2018

one concern i have is that the semantics of the same code snippet would be different based on your target compilation..

@ajafff
Copy link
Contributor Author

ajafff commented Mar 28, 2018

one concern i have is that the semantics of the same code snippet would be different based on your target compilation..

That's why I would still prefer to rename function scoped variables that shadow variables used in a parameter initializer when transpiling to ES3 or ES5. That way you don't need to maintain and test two different implementations depending on the target.

In the general case, we can't know what other outside variables in scope might exist.

You already make similar assumptions when transpiling lexical declarations:

// no-shadow.ts
{
    let foo = 1;
}

// no-shadow.js
{
    var foo = 1; // assumes there is no variable `foo` in the global scope
}

/********************/

// shadow.ts
var foo = 0;
{
    let foo = 1;
}

// shadow.js
var foo = 0;
{
    var foo_1 = 1; // assumes there is no variable `foo_1` in the outer scope
}

@RyanCavanaugh RyanCavanaugh added Help Wanted You can do this and removed In Discussion Not yet reached consensus labels Aug 7, 2018
@RyanCavanaugh RyanCavanaugh added this to the Future milestone Aug 7, 2018
@RyanCavanaugh RyanCavanaugh added the Effort: Moderate Requires experience with the TypeScript codebase, but feasible. Harder than "Effort: Casual". label Aug 7, 2018
@RyanCavanaugh
Copy link
Member

Accepting PRs to allow the ES6 scoping behavior when targeting ES6+; code should remain an error in downlevel targets

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Effort: Moderate Requires experience with the TypeScript codebase, but feasible. Harder than "Effort: Casual". Fixed A PR has been merged for this issue Help Wanted You can do this Suggestion An idea for TypeScript
Projects
None yet
Development

No branches or pull requests

4 participants