Skip to content

Allowed unused parameters before used ones #12139

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

Conversation

JoshuaKGoldberg
Copy link
Contributor

@JoshuaKGoldberg JoshuaKGoldberg commented Nov 10, 2016

This PR addresses concerns listed in #9458 by removing errors logged for any unused parameters, particularly from @KnisterPeter and myself. Errors are now skipped for unused parameters that come before used parameters.

Asking for _-prefixed variable names as the primary method of marking variables as unused can be non-optimal for several reasons:

  • We're binding compiler logic to the variable name
  • It's easy to forget to remove the prefix, such as during refactoring
  • Prefixed names clash with some existing coding styles

Josh Goldberg added 5 commits November 9, 2016 23:18
A running tally of unused parameters is kept durin checking unused
locals. It's reset to `[]` whenever a used parameter is found. If any
are left after done checking a scope they're logged as errors.
Added a third parameter existing tests that previously had an unused and
a used parameter in that order.
@msftclas
Copy link

Hi @JoshuaKGoldberg, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!


It looks like you're a Microsoft contributor (Josh Goldberg). If you're full-time, we DON'T require a Contribution License Agreement. If you are a vendor, please DO sign the electronic Contribution License Agreement. It will take 2 minutes and there's no faxing! https://cla.microsoft.com.

TTYL, MSBOT;

}

function checkUnusedParameterLocals(locals: Map<Symbol>): void {
let unusedParameterLocals: Symbol[] | undefined = [];
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From a performance perspective I was hesitant to make a new [] no matter what. Hence the slight added complexity.

}
}
}

if (unusedParameterLocals) {
forEach(unusedParameterLocals, local => error(local.valueDeclaration.name, Diagnostics._0_is_declared_but_never_used, local.name));
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should unusedParameterLocals be sorted? Map guarantees iteration in insertion order, but does TS guarantee locals be in inserted order? Do we want to rely on that guarantee?

@ghost
Copy link

ghost commented Nov 10, 2016

In my experience, it's useful to know when you aren't using a parameter, regardless of where that parameter appears.
I know it can be annoying if an API wants you to provide a callback where only the last parameter is useful, but I wouldn't want a solution that doesn't consider users who want all of their unused parameters to be errors, not just ones on the end.

Copy link
Contributor

@mhegazy mhegazy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not believe this is needed. we do have support for _ as a special case for this purpose.

Moreover this change does not have a matching issue. Please file issues to discuss the design first.

@JoshuaKGoldberg
Copy link
Contributor Author

@Andy-MS I was actually referring to the technical implementation, but that's a good point.
@mhegazy filed #12149

I'll close this PR and leave the branch as reference.

@microsoft microsoft locked and limited conversation to collaborators Jun 19, 2018
@JoshuaKGoldberg JoshuaKGoldberg deleted the unused-parameters-ordered branch March 22, 2022 18:29
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants