Skip to content

Redundant rest parameter should be omitted from compiled output #11053

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

Closed
axefrog opened this issue Sep 22, 2016 · 6 comments
Closed

Redundant rest parameter should be omitted from compiled output #11053

axefrog opened this issue Sep 22, 2016 · 6 comments
Labels
Question An issue which isn't directly actionable in code

Comments

@axefrog
Copy link

axefrog commented Sep 22, 2016

TypeScript Version: 2.0.2 (rc)

In some cases I am iterating through arguments using the arguments object, and don't actually need to reference the paremeters. In these sorts of cases, the only reason to include the rest parameter in the original source is to tell TypeScript that the function may be called with any number of arguments. If the parameter is never actually referenced (particularly if you then access the arguments object), it should be able to infer that the parameter is redundant and drop it from the compiled output.

Having an intermediate array built, or even just a redundant rest parameter when targeting ES2015, is not ideal when your code is performance-critical. Optimizing compilers might be smart enough to skip an unused rest parameter, but it still feels sloppy, and ES5 output will be worse, of course.

The example below is contrived of course, but it serves to illustrate the redundant output:

function run(...args: any[]) {
  const values = arguments;
  for(var i = 0; i < values.length; i++) {
    console.log(values[i]);
  }
}

ES5 output:

function run() {
    var args = [];
    for (var _i = 0; _i < arguments.length; _i++) {
        args[_i - 0] = arguments[_i];
    }
    var values = arguments;
    for (var i = 0; i < values.length; i++) {
        console.log(values[i]);
    }
}

and ES2015 output:

function run(...args) {
    const values = arguments;
    for (var i = 0; i < values.length; i++) {
        console.log(values[i]);
    }
}
@yortus
Copy link
Contributor

yortus commented Sep 22, 2016

@axefrog since the prime concern here seems to be performance, what is the performance difference if you actually use the args rest parameter instead of arguments in your implementation? E.g. is run1 slower than run2 in the code below? I'm genuinely curious.

function run1(...args: any[]) {
  for(var i = 0; i < args.length; i++) {
    console.log(args[i]);
  }
}

function run2() {
  const values = arguments;
  for(var i = 0; i < values.length; i++) {
    console.log(values[i]);
  }
}

@axefrog
Copy link
Author

axefrog commented Sep 22, 2016

@yortus it's not quite as specific as this (yes I do have one function in mind right now, but I don't want to focus on that); rather than optimizing for one specific case I'm experiencing, and then changing my code to try and cater to TypeScript idiosyncracies, I'm just observing that, because JavaScript offers an arguments object and that sometimes it will be used directly, and that sometimes doing so will be in an area of code where redundancy creates a question mark as to expected performance characteristics on different runtime compilers, it seems logical that I should be able to take advantage of this language feature of JavaScript without paying the cost of redundant compiler output. Also, generating redundant syntax seems sloppy.

@yortus
Copy link
Contributor

yortus commented Sep 22, 2016

Would the following meet your needs? It has both the compile-time (variadic) and runtime (no redundant var) characteristics you mentioned.

const foo: (...args) => void = function () {
    const values = arguments;
    for (var i = 0; i < values.length; i++) {
        console.log(values[i]);
    }
}

EDIT: @RyanCavanaugh's solution below is more idiomatic TS and preserves hoisting, so use that.

@RyanCavanaugh
Copy link
Member

RyanCavanaugh commented Sep 22, 2016

The solution is simple, though undiscoverable:

function run(...args: any[]): void;
function run() {
  const values = arguments;
  for(var i = 0; i < values.length; i++) {
    console.log(values[i]);
  }
}

@RyanCavanaugh RyanCavanaugh added the Question An issue which isn't directly actionable in code label Sep 22, 2016
@mhegazy mhegazy closed this as completed Sep 22, 2016
@axefrog
Copy link
Author

axefrog commented Sep 22, 2016

@RyanCavanaugh Ah, thanks! May I suggest a ticket to clarify this in the documentation?

@RyanCavanaugh
Copy link
Member

We try to keep the documentation to common-ish cases and this seems to be fairly rare. You can also see #498 which we're accepting PRs for.

@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
Question An issue which isn't directly actionable in code
Projects
None yet
Development

No branches or pull requests

4 participants