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

JavaScript output has variable below return #4138

Closed
blakeembrey opened this issue Aug 4, 2015 · 6 comments
Closed

JavaScript output has variable below return #4138

blakeembrey opened this issue Aug 4, 2015 · 6 comments
Labels
Revisit An issue worth coming back to Suggestion An idea for TypeScript

Comments

@blakeembrey
Copy link
Contributor

Using latest commit (4c7b214) and compiling the code below results in the variable initialization below return.

class C {
  constructor (...args: any[]) {}
}

function fn (...args) {
  return new (C)(...args)
}
function fn() {
    var args = [];
    for (var _i = 0; _i < arguments.length; _i++) {
        args[_i - 0] = arguments[_i];
    }
    return new ((_a = (C)).bind.apply(_a, [void 0].concat(args)))();
    var _a;
}

The variable is always below the return, even when the statement is assigned to a variable instead of returning.

Edit: node node_modules/.bin/tsc test.ts --target es5

@DanielRosenwasser DanielRosenwasser added the By Design Deprecated - use "Working as Intended" or "Design Limitation" instead label Aug 4, 2015
@DanielRosenwasser
Copy link
Member

It doesn't matter where the variable is placed; it just needs to be declared so that we can use it as a temporary value.

@blakeembrey
Copy link
Contributor Author

I realised, but it didn't look by design and I didn't see a previous issue logged. I actually found Istanbul has issues with code coverage with this, though I've logged an issue there too.

@kitsonk
Copy link
Contributor

kitsonk commented Aug 4, 2015

I realised, but it didn't look by design and I didn't see a previous issue logged.

I suspect it was because it wasn't supported before when targeting ES5, so it wasn't a "break" of previous behaviour, because there was no previous behaviour.

Declaring things after the return though doesn't seem like clean idiomatic JavaScript though.

@danquirk
Copy link
Member

danquirk commented Aug 4, 2015

Seems reasonable to just put the var before the return isn't it? The objections here (not very idiomatic, plays poorly with tools like code coverage and linters checking for unreachable code) seem compelling enough. Obviously a minor issue but there's not really a good reason to not fix it (eventually) right?

@danquirk danquirk reopened this Aug 4, 2015
@danquirk danquirk added Suggestion An idea for TypeScript In Discussion Not yet reached consensus Help Wanted You can do this and removed By Design Deprecated - use "Working as Intended" or "Design Limitation" instead labels Aug 4, 2015
@mhegazy mhegazy removed the Help Wanted You can do this label Aug 4, 2015
@mhegazy
Copy link
Contributor

mhegazy commented Aug 4, 2015

This is not something that can be easily changed. the way the emitter works today is by doing transformations on the fly. transformations are done as we descend into the tree. at the time we are visiting a destructuring pattern we have emitted previous statements. moreover, destructuring can be in an expression position. so to emit the variables first we need to do a pre-walk to find destructuring assignments, somehow predict the variables needed, emit them, then as we are doing the destructuring use them.

I would say leave this issue open. we value the cleanliness of the emitted code, and there is an oprtnity to make it cleaner here. however the real fix would be to split the emitter to a transformation phase followed by a writing phase. we are looking into this now.

@mhegazy mhegazy added the Revisit An issue worth coming back to label Feb 22, 2016
@mhegazy
Copy link
Contributor

mhegazy commented Feb 22, 2016

closing for now. marking as revisit. This should be a simple change once #5595 is in.

@mhegazy mhegazy closed this as completed Feb 22, 2016
@mhegazy mhegazy removed the In Discussion Not yet reached consensus label Feb 22, 2016
@microsoft microsoft locked and limited conversation to collaborators Jun 19, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Revisit An issue worth coming back to Suggestion An idea for TypeScript
Projects
None yet
Development

No branches or pull requests

5 participants