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

When body function use "arguments" object, should there be an error (using "noUnusedParameter" is true) #9792

Closed
yuit opened this issue Jul 18, 2016 · 9 comments
Labels
Working as Intended The behavior described is the intended behavior; this is not a bug

Comments

@yuit
Copy link
Contributor

yuit commented Jul 18, 2016

TypeScript : 2.0.0

While trying to apply "noUnusedParameter" option in TypeScript code base, I ran into situation when one use Function.apply and pass in "arguments" object. Our compiler will then issue an error that the containing function's parameter is unused, should we still give an error in this case or by seeing "arguments" object use in function body we should not give an error

@RyanCavanaugh
Copy link
Member

I would argue this is a bit unusual and the author should write something like this to make their intent clear:

function fn(arg1, arg2): void;
function fn() {
  // do something with 'arguments' here
}

@RyanCavanaugh
Copy link
Member

... though I guess this doesn't handle the case where you want to use arg2 but not arg1

@yortus
Copy link
Contributor

yortus commented Jul 19, 2016

You've got to declare formal parameters if you want the function to have a specific arity (eg (function fn(a, b, c) {}).length === 3). But for whatever reason you might process the actual parameters using arguments.

Doesn't #9458 solve this? i.e., declare it like function fn(_arg1, _arg2) {...}

@yortus
Copy link
Contributor

yortus commented Jul 19, 2016

Also, I don't think the mere presence of arguments in a function body is enough to satisfy the 'unused parameter' check, since it can appear in arbitrary expressions that in some cases access all the parameters, and in other cases only some of them, or even none of them. You couldn't in general tell one case from the other at compile time.

@poke
Copy link

poke commented Jul 19, 2016

Shouldn’t rest parameters eliminate the need for arguments here?

@mhegazy
Copy link
Contributor

mhegazy commented Jul 21, 2016

Shouldn’t rest parameters eliminate the need for arguments here?

rest parameters will add a for loop and a copy operation to your output. if you are using arguments, and this is a performance critical path, i would not use rest args.

@mhegazy
Copy link
Contributor

mhegazy commented Jul 21, 2016

As noted by @RyanCavanaugh and @yortus, you have two options, either remove the arguments and use an overload, or rename them to start with _.

@mhegazy mhegazy closed this as completed Jul 21, 2016
@mhegazy mhegazy added the Working as Intended The behavior described is the intended behavior; this is not a bug label Jul 21, 2016
@yortus
Copy link
Contributor

yortus commented Jul 21, 2016

you have two options, either remove the arguments and use an overload, or rename them to start with _.

@mhegazy or both! The overload supplies pretty param names for intellisense, and the implementation maintains the correct arity (ie Function#length) by declaring formal params. E.g.:

function fn(arg1, arg2): void; // pretty param names for intellisense
function fn(_1, _2) {  // formal params to preserve function arity
  // do something with 'arguments' here
}```

@mhegazy
Copy link
Contributor

mhegazy commented Jul 21, 2016

Sure, that would work as well.

@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
Working as Intended The behavior described is the intended behavior; this is not a bug
Projects
None yet
Development

No branches or pull requests

5 participants