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

with --noUnusedParameters how can i skip uneeded parameters #9458

Closed
aleksey-bykov opened this Issue Jun 30, 2016 · 17 comments

Comments

Projects
None yet
8 participants
@aleksey-bykov
Copy link

aleksey-bykov commented Jun 30, 2016

sorry for being persistent, same question as in

#9403 (comment)

how is this going to work with unused parameters?

fiction third(x:any, y: any, z: T) : T { return z; } // need only last one
@mhegazy

This comment has been minimized.

Copy link
Contributor

mhegazy commented Jun 30, 2016

i was thinking of _ as a special case. but that does not seem nice. the other option is to use arguments.

@mhegazy mhegazy changed the title how can i skip uneeded parameters With `--nuUnusedParameters`, how can i skip uneeded parameters Jun 30, 2016

@aleksey-bykov

This comment has been minimized.

Copy link

aleksey-bykov commented Jun 30, 2016

_ works well in f#, and i like it a lot, but might be a breaking change for stuff like underscore.js

@RyanCavanaugh

This comment has been minimized.

Copy link
Member

RyanCavanaugh commented Jun 30, 2016

I would propose that any local starting with _ is not subject to unused checks. For multiple parameters you could then write (_0, _1, x) => x

@aleksey-bykov

This comment has been minimized.

Copy link

aleksey-bykov commented Jun 30, 2016

consider third<T>(,, z: T): T { return z; }

@RyanCavanaugh

This comment has been minimized.

Copy link
Member

RyanCavanaugh commented Jul 1, 2016

Discussed quite a bit here https://esdiscuss.org/topic/uninteresting-parameters and I would defer to their judgement

@mhegazy

This comment has been minimized.

Copy link
Contributor

mhegazy commented Jul 1, 2016

I have a fix out in #9464. any parameter name starting with _ is exempt from the check.

@mhegazy

This comment has been minimized.

Copy link
Contributor

mhegazy commented Jul 1, 2016

if @RyanCavanaugh gives me a :+: i can get it in

@aleksey-bykov

This comment has been minimized.

Copy link

aleksey-bykov commented Jul 1, 2016

@RyanCavanaugh the discussion you mentioned ended nowhere, but I like the point that was raised there: destructuring has been already accepting commas for unused array elements:

const values = [1,2,3];
const [,,z] = values;

@aleksey-bykov aleksey-bykov changed the title With `--nuUnusedParameters`, how can i skip uneeded parameters with --noUnusedParameters how can i skip uneeded parameters Jul 1, 2016

@mhegazy mhegazy closed this in #9464 Jul 1, 2016

mhegazy added a commit that referenced this issue Jul 1, 2016

Merge pull request #9464 from Microsoft/excludeUnderscoreFromUnusedPa…
…ramterChecks

Fix #9458: exclude parameters starting with underscore from unusedParamter checks

yuit added a commit that referenced this issue Jul 18, 2016

[Transforms] Merge master 07/11 into transform (#9697)
* Use merge2, gulp-if, gulp-newer, and more projects

* Add watch task

* Working non-inline sourcemaps for runtests

* browser tests now also loads sourcemaps from disk

* Lazypipes and better services stream management

* export interface used by other exported functions

* Make goto-definition work for `this` parameter

* Add new error for rest parameters

* Add error message for rest parameter properties

* Fix case when a document contains multiple script blocks with different base indentations.
Use the base indent size if it is greater that the indentation of the inherited predecessor

* Fix rwc-runner from breaking change in compiler (#9284)

* Signatures use JSDoc to determine optionality

* Changed implementation to use closure

* Updated tests

* Fixed linting error

* Adding Code of Conduct notice

* Don't crash when JS class property is self-referential.

Fixes #9293

* Remove stale baselines

* For optionality, check question token before JSDoc

* Accept rest parameter properties error baselines

* Change binding pattern parameter property error

* Accept binding pattern properties error baselines

* Lint

* Port the sync version diagnostics API from tsserverVS-WIP branch to 2.0

* Do copyright without gulp-if and lazypipe

* Change test comment and accept baseline

* Remove tsd scripts task from gulpfile

* Make use of module compiler option explicit, add strip internal to tsconfigs

* Remove Signature#thisType and use Signature#thisParameter everywhere

* Add Gulpfile lint to jake, fix lints

* Change reference tests to verify actual ranges referenced and not just their count

* Respond to PR comments

* Add new lint rule

* Fix object whitespace lints

* Fix case of gulpfile dependencies

* 1. pass subshell args 2. fix build order in services

1. /bin/sh requires its arguments joined into a single string unlike
cmd.
2. services/ depends on a couple of files from server/ but the order was
implicit, and changed from jakefile. Now the order is explicit in the
tsconfig.

* Fix single-quote lint

* Check for exactly one space

* Fix excess whitespace issues

* Add matchFiles test to Gulpfile

This was merged while the gulpfile was still in-progress

* Fix LKG useDebug task and newLine flag

* Update LKG

* Clean before LKG in Gulpfile

* Fix lint

* Correct the api string name

* Allow space in exec cmds

* Fix typo

* Add new APIs to protocol

* Fix bug where `exports.` was prepended to namespace export accesses

* Remove unnecessary parameter

* extract expression into function

* Add fourslash tests & address CR comments

* Fix 8549: Using variable as Jsx tagname (#9337)

* Parse JSXElement's name as property access instead of just entity name. So when one accesses property of the class through this, checker will check correctly

* wip - just resolve to any type for now

* Resolve string type to anytype and look up property in intrinsicElementsType of Jsx

* Add tests and update baselines

* Remove unneccessary comment

* wip-address PR

* Address PR

* Add tets and update baselines

* Fix linting error

* Unused identifiers compiler code (#9200)

* Code changes to update references of the Identifiers

* Added code for handling function, method and coonstructor level local variables and parameters

* Rebased with origin master

* Code changes to handle unused private variables, private methods and typed parameters

* Code changes to handle namespace level elements

* Code changes to handle unimplemented interfaces

* Code to optimize the d.ts check

* Correct Code change to handle the parameters for methods inside interfaces

* Fix for lint error

* Remove Trailing whitespace

* Code changes to handle interface implementations

* Changes to display the error position correctly

* Compiler Test Cases

* Adding condition to ignore constructor parameters

* Removing unnecessary tests

* Additional changes for compiler code

* Additional changes to handle constructor scenario

* Fixing the consolidated case

* Changed logic to search for private instead of public

* Response to PR Comments

* Changed the error code in test cases as result  of merge with master

* Adding the missing file

* Adding the missing file II

* Response to PR comments

* Code changes for checking unused imports

* Test Cases for Unused Imports

* Response to PR comments

* Code change specific to position of Import Declaration

* Code change for handling the position for unused import

* New scenarios for handling parameters in lambda function, type parameters in methods, etc.

* Additional scenarios based on PR comments

* Removing a redundant check

* Added ambient check to imports and typeparatmeter reporting

* Added one more scenario to handle type parameters

* Added new scenario for TypeParameter on Interface

* Refactoring the code

* Added scenario to handle private class elements declared in constructor.

* Minor change to erro reporting

* Fix 8355: Fix emit metadata different between transpile and tsc --isolatedModule (#9232)

* Instead of returning undefined for unknownSymbol return itself

* Add Transpile unittest

* Wip - Add project tests

* Add project tests and baselines

* Update existed tests

* Add tests for emitting metadata with module targetting system

* Fix 8467: Fix incorrect emit for accessing static property in static propertyDeclaration (#8551)

* Fix incorrect emit for accessing static property in static propertyDeclaration

* Update tests and baselines

* Update function name

* Fix when accessing static property inside arrow function

* Add tests and baselines

* do not format comma/closeparen in jsxelement

* format jsx expression

* Remove extra baselines

* Fixed bugs and linting

* Added project tests for node_modules JavaScript searches

* Removed old TODO comment

* make rules optional

* Fixed the regexp for removing full paths

* Fix type of the disableSizeLimit option

* Update version to 2.0.0

* Remove upper boilerplate from issue template

Our issue stats did not improve appreciably when we added the issue template. Reduce upper boilerplate text and try to make it more action-oriented

* Remove unused compiler option (#9381)

* Update LKG

* Added emitHost method to return source from node modules

* Marked new method internal

* Update issue_template.md

* new options should be optional for compatibility

* Add getCurrentDirectory to ServerHost

* Add nullchecks for typeRoots, remove getCurrentDirectory from ServerHost as it is always the installation location

* VarDate interface and relevant Date.prototype members

* Port 9396 to release 2.0

* Fix 9363: Object destructuring broken-variables are bound to the wrong object (#9383)

* Fix emit incorrect destructuring mapping in var declaration

* Add tests and baselines

* Add additional tests and baselines

* Fix crash in async functions when targetting ES5.

When targetting ES5 and with --noImplicitReturns,
an async function whose return type could not be determined would cause
a compiler crash.

* Add This type to lib

* Merge master into release-2.0 (#9400)

* do not format comma/closeparen in jsxelement

* format jsx expression

* make rules optional

* Remove upper boilerplate from issue template

Our issue stats did not improve appreciably when we added the issue template. Reduce upper boilerplate text and try to make it more action-oriented

* Update issue_template.md

* new options should be optional for compatibility

* Add getCurrentDirectory to ServerHost

* Add nullchecks for typeRoots, remove getCurrentDirectory from ServerHost as it is always the installation location

* VarDate interface and relevant Date.prototype members

* Fix 9363: Object destructuring broken-variables are bound to the wrong object (#9383)

* Fix emit incorrect destructuring mapping in var declaration

* Add tests and baselines

* Add additional tests and baselines

* Fix #9402: Do not report unused identifier errors for catch variables

* getVarDate should be on the Date interface

* Defere checking unsed identifier checks

* Do not scan nodes preceding formatted region, just skip over them

* Don't emit source files found under node_modules

* Destructuring assignment removes undefined from type when default value is given

* Add nullcheck when calculating indentations for implort clause

* Use a deferred list to check for unused identifiers

* push checks to checkUnusedIdentifiersDeferred

* use isParameterPropertyDeclaration to test for paramter propoerties

* runtests-parallel skips empty buckets

Previously, it would enter them as buckets with no tests, which would
make our test runners run *every* test.

This was very obvious on machines with lots of cores.

* Report unused identifiers in for statements

* Do not check ambients, and overloads

* Add tests

* Consolidate type reference marking in getTypeFromTypeReference

* Handel type aliases

* Add tests

* Add test

* Dont load JavaScript if types packages are present

* Renamed API

* Use checkExpression, not checkExpressionCached

* Do not report unused errors for module augmentations

* Consolidate refernce marking in resolveName to allow marking aliases correctelly

* add tests

* Code review comments

* Only mark symbols found in a local symbol table

* Show "<unknown>" if the name of a declaration is unavailable

* Parse `export default async function` as a declaration

* Respond to PR comments

* Better name for test

* handel private properties correctelly

* Port 9426 to release 2.0

* Handel Swtich statements
check for locals on for statments
only mark private properties

* Removed one error to avoid full path issues

* Don't emit source files found under node_modules

(cherry picked from commit 5f8cf1a)

* Dont load JavaScript if types packages are present

(cherry picked from commit 5a45c44)

* Renamed API

(cherry picked from commit d8047b6)

* Removed one error to avoid full path issues

(cherry picked from commit 5e4f13f)

* Fix incorrectly-saved quote symbols in ThirdPartyNoticeText.txt

* Fix #9458: exclude parameters starting with underscore from unusedParamter checks

* change variable name for strict mode

* Increase timeout from running RWC. As UWDWeb takes slightly longer now (#9454)

* Handle relative paths in tsconfig exclude and include globs

* Merge master into release branch 06/30 (#9447)

* do not format comma/closeparen in jsxelement

* format jsx expression

* make rules optional

* Remove upper boilerplate from issue template

Our issue stats did not improve appreciably when we added the issue template. Reduce upper boilerplate text and try to make it more action-oriented

* Update issue_template.md

* new options should be optional for compatibility

* Add getCurrentDirectory to ServerHost

* Add nullchecks for typeRoots, remove getCurrentDirectory from ServerHost as it is always the installation location

* VarDate interface and relevant Date.prototype members

* Fix 9363: Object destructuring broken-variables are bound to the wrong object (#9383)

* Fix emit incorrect destructuring mapping in var declaration

* Add tests and baselines

* Add additional tests and baselines

* Fix crash in async functions when targetting ES5.

When targetting ES5 and with --noImplicitReturns,
an async function whose return type could not be determined would cause
a compiler crash.

* Add This type to lib

* getVarDate should be on the Date interface

* Don't emit source files found under node_modules

* Destructuring assignment removes undefined from type when default value is given

* Add nullcheck when calculating indentations for implort clause

* Add test

* Dont load JavaScript if types packages are present

* Renamed API

* Use checkExpression, not checkExpressionCached

* Show "<unknown>" if the name of a declaration is unavailable

* Parse `export default async function` as a declaration

* Removed one error to avoid full path issues

* Fix incorrectly-saved quote symbols in ThirdPartyNoticeText.txt

* Improve names of whitespace functions

* Handle relative paths in tsconfig exclude and include globs

Port 9475 to release 2.0

* add new method getEmitOutputObject to return result of the emit as object with properties instead of json string

* fix linter

* Fix PromiseLike to be compatible with es6-promise (#9484)

* Fix reading files from IOLog because previous our API captures (#9483)

* Fix reading files from IOLog because previous our API captures

* Refactoring the ioLog

* Exclude FlowSwitchClause from flow graph for case expressions

* Add regression test

* Update LKG

* Update language in comment

* Add .mailmap file

* Add authors script to generate authors from repo

* Update AUTHORS.md for release-2.0

* Update script to pass more than one argument

* Remove the unused text buffer from ScriptInfo

* Fix #9531: account for async as an contextual keyword when parsing export assignments

* Update LKG

* Swap q from a reference to an import

* Fix #9550: exclude 'this' type parameters from unusedParameters checks.

* Update comment to reflect new dependency

* Avoid putting children tags in jsdoccomment

* Parse the result of getDirectories call

* Update harness getDirectories implementation for shims

* Fix multiple Salsa assignment-declarations

Previously, all assignment-declarations needed to be of the same kind:
either all `this.p = ...` assignments or `C.prototype.p = ...`
assignments.

* Test for multiple salsa assignment-declarations

* Add test for parsed @typedef tag node shape

* Provide a symbol for salsa-inferred class types

* Update .mailmap

* Fix module tracking

* Updated test with relative import

* Fixed the node tracking and a harness bug

* fixed lint error

* Fixed implicit any

* Added missing test files

* Removed duplicate logic

* Update conflicting baseline.

PR #9574 added a baseline that #9578 caused to be changed. The two PRs
went in so close to each other that the CI build didn't catch the change
to the new test's baseline.

* Fix type of JSXTagName

* Update baselines to use double-quote

* Update baselines when emitting metadata decorator

* Update baselines for async-await function

* Update baselines for comment in capturing down-level for...of and for...in

* Add missing Transpile tests

* Remove old JS transpile baselines

* Passing program as argument in emitWorker

* Port PR#9607 transforms

* Port new JSDOC tests to use baseline

* substitute alias for class expression in statics

* Address new lint warnings

* Change name for substitution function.
@KnisterPeter

This comment has been minimized.

Copy link
Contributor

KnisterPeter commented Jul 21, 2016

The referenced discussion is about destructuring.
I think it may introduce flaws and false positives to prefix a parameter name with _.

For example if in a current function the first parameter is not used but the second is I need to prefix the first one. If the code is refactored then and afterwards the function only has one parameter left (the first one because of api changes), then the check is disabled, because it is still prefixed.

It is very easy to forget to remove the underscore prefix.

As well we have a compiler meaning in the parameter name which I think is no good practice. If I like to skip a parameter at all, then a single underscore may be okay, but again I would think this does not lead to good readable code.

May I suggest that the compiler reduces the parameters from right to left and only the last x ones not used lead to an error?
If there are parameters on the left side of a used one, the unused error is silently ignored.

@JoshuaKGoldberg

This comment has been minimized.

Copy link
Contributor

JoshuaKGoldberg commented Nov 10, 2016

I would add the linting argument for the variable names. It's against some coding styles to have _s in front of names. As @KnisterPeter mentioned, we're binding compiler logic and the variable name. That feels weird.

Even worse, some coding styles prefer the _ names, and so won't benefit from the compiler flag.

@EnverOsmanov

This comment has been minimized.

Copy link

EnverOsmanov commented Aug 2, 2017

_ is "reserved" by underscore.js so double underscore or asterisk could be used instead

 (__, __, x) => x
 (*, *, x) => x
@kitsonk

This comment has been minimized.

Copy link
Contributor

kitsonk commented Aug 2, 2017

_ is "reserved" by underscore.js so double underscore or asterisk could be used instead

The fix, which is merged, and therefore this issue is closed states that any variable that starts with an underscore will be considered uninteresting, so a double underscore is already valid.

@Melmoth-the-Wanderer

This comment has been minimized.

Copy link

Melmoth-the-Wanderer commented Nov 14, 2017

I agree with @KnisterPeter , compiler should not force developer to use particular naming convention, which might be not possible in certain cases (or may at least put other devs in confusion). I would love to see a fix for compiler, if possible.

@EnverOsmanov

This comment has been minimized.

Copy link

EnverOsmanov commented Nov 14, 2017

@Melmoth-the-Wanderer what is a problem to call unused parameters as double underscore (for example)? Can you provide an example when it is not possible?

@Melmoth-the-Wanderer

This comment has been minimized.

Copy link

Melmoth-the-Wanderer commented Nov 14, 2017

@EnverOsmanov , it's been already briefly explained above. When you change the name by appending underscore, its easy to loose the context of the change in future. Especially if _name is reserved for some kind of special cases in your team. It also might require developers to change coding styles and guidelines.

Imagine working on a long-lasting project with multiple devs involved. Years ago your team used to name private properties with _ at the beginning. And couple of month ago team decided to change coding guidelines so its not acceptable anymore in the team. So you added lint rules to warn users no to do so - and now you have a problem, because your compiler requires special naming rules! And if someone is already using double underscore for some kind of semantic information, you go and tell them what you meant by adding the third one.

In principle, it is possible to name variable starting with _ sign because JS allows that, no doubt. But I've got mixed feeling when it comes to language (compiler in this case) telling programmers how to name their variables.

For now it's just one prefix and maybe I'm overreacting, but if we start allowing such prefixes Its going to be a really messy 'language' pretty soon.

@RyanCavanaugh

This comment has been minimized.

Copy link
Member

RyanCavanaugh commented Nov 14, 2017

No one's being forced to do anything. You can turn off noUnusedParameters if this is bothersome. You can @ts-ignore if you want to silence a particular line of code. You can write your own lint rule that says ignored parameters must be named exactly _0, _1, etc..

People wanted a convenient "out" for an unused parameter in the rare cases where it was provided, and we added one that doesn't really change anything else about the language for the sake of simplicity.

@Melmoth-the-Wanderer

This comment has been minimized.

Copy link

Melmoth-the-Wanderer commented Nov 14, 2017

Sure, I just share my thoughts on how to improve. I just thought it would be nice to make noUnusedParameters work out of the box without doing extra steps on dev side.

Thanks for the hint on how to deal with the problem internally, though

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.