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

Revisiting _ underscore for unused parameters: create unused type #29202

Closed
4 of 5 tasks
aryzing opened this issue Dec 30, 2018 · 18 comments
Closed
4 of 5 tasks

Revisiting _ underscore for unused parameters: create unused type #29202

aryzing opened this issue Dec 30, 2018 · 18 comments
Labels
Declined The issue was declined as something which matches the TypeScript vision Suggestion An idea for TypeScript

Comments

@aryzing
Copy link

aryzing commented Dec 30, 2018

Search Terms

_, underscore, noUnusedParameters, --noUnusedParameters, tslint, ts-lint, #9458, palantir/tslint#3442

Suggestion

Create an unused type for function parameters that are not used.

Use Cases

Can be used to clearly state unused parameters, while still playing nice with --noUnusedParameters and linters enforcing variable name conventions.

May also help in cases such as

Examples

1. Unused parameter

function typicalNodeCallback(err: unused, data: any) {
  // do stuff with `data`
}

2. Using unused parameter is an error

function expressMiddleware(req: unused, res: any) {
  const body = req.body // ERROR!
}

Checklist

My suggestion meets these guidelines:

  • This wouldn't be a breaking change in existing TypeScript/JavaScript code
  • This wouldn't change the runtime behavior of existing JavaScript code
  • This could be implemented without emitting different JS based on the types of the expressions
  • This isn't a runtime feature (e.g. library functionality, non-ECMAScript syntax with JavaScript output, etc.)
  • This feature would agree with the rest of TypeScript's Design Goals.
@ajafff
Copy link
Contributor

ajafff commented Dec 30, 2018

I don't think adding a type solves anything. It actually loses type information: Sometimes you need to declare the type of a parameter even if you don't use is. For example in methods of a base class that are meant to be overridden.

@aryzing
Copy link
Author

aryzing commented Dec 30, 2018

Haven't run across that scenario that often, perhaps an example might help? In any case, off the top of my head I can think of something like

myMethod(param: unused | MyType) {
//
}

If it's unused, then unused applies. Otherwise, WhateverType applies.

@ajafff
Copy link
Contributor

ajafff commented Dec 30, 2018

perhaps an example might help?

interface Options {}
class Base {
  constructor(options: Options) {
    this.init(options);
  }
  init(_options: Options) {
    // can be overridden by a subclass
    // the parameter needs to be there so the method in the subclass knows that parameter is available
  }
}

If it's unused, then unused applies. Otherwise, WhateverType applies.

That could be very confusing if that unused "type" magically disappears. I guess a type annotation is not the right place to declare something as unused.
And then we're back to searching a sigil to declare something as unused. How about prefixing with an underscore?

@aryzing
Copy link
Author

aryzing commented Dec 31, 2018

It's hard to talk about what's right or wrong in something that we're making up as we go. Instead it might be of more value to the community to consider things such as convenience, playing well with other tools, and separation of concerns between syntax and compiler logic.

As far as ✨ magic ✨ goes, subjective as it may be, seems no more magical than a leading underscore.

Moreover, the issue for this feature was opened and merged in less than 24 hours, leaving little room for community feedback and a proper discussion during its inception, while the merge request itself has 50% of 👎 . Once merged, several different points and propositions were made regarding issues we could face down the line, which we now are.

In any case, the objective of this issue is not to push the specific syntax I've proposed above, but rather to ensure that we find a lasting and viable solution to unused parameters. In terms of playing well with other tools and separation of concerns, I do believe my proposal is a step in the right direction. Convenience, again, is subjective, so we may yet find a better solution!

@MartinJohns
Copy link
Contributor

MartinJohns commented Dec 31, 2018

[x] This wouldn't be a breaking change in existing TypeScript/JavaScript code

As unused is not a reserved keyword currently, this most definitely would be a breaking change.

Can be used to clearly state unused parameters, while still playing nice with --noUnusedParameters and linters enforcing variable name conventions.

I don't see this as a reason enough to add another keyword/type. Having a _ prefix on the variable name works nice with --noUnusedParameters. And if your linter complains, then you should configure your linter to accept this very valid use case. And it being represented as a type will likely open a can of worms.

The way you have your third-party tools configured should not affect the development of a language.

@aryzing
Copy link
Author

aryzing commented Dec 31, 2018

would be a breaking change.

Thanks, updated original post :) In any case, the impact would be very low. For example, in the entire DefinitelyTyped repo there are only three instances of unused being used.

Having a _ prefix on the variable name works nice with --noUnusedParameters.

Perhaps I should have expressed myself better: it only works nice with --noUnusedParameters. The point I was trying to make is that it would be better if we found a more universal solution, one confined within TypeScript's additions to the language and that leaves no trace when typings are removed.

As per point 9 of the Design Goals, Use a consistent, fully erasable, structural type system. If the type system introduces an underscore "idiom" in order for it to work properly when one of its options is activated (--noUnusedParameters) and just leaves it there when the types are stripped, then I'd say that does not conform to the project goals: the type system is not fully erasable.

Moreover, coming back to the ✨magic✨ discussion we were having earlier, the underscore would probably go against point 7 of Non Goals: Introduce behaviour that is likely to surprise users. Instead have due consideration for patterns adopted by other commonly-used languages. While the pattern may be common, having variable names linked to compiler options for the type system is certainly surprising.

if your linter complains,

The way you have your third-party tools configured should not affect the development of a language.

And that is by no means the angle I'm going for here. I appreciate that you care about my particular dev environment, but rest assured, it is working quite fine. The idea behind this issue is not to discuss the particulars of how this feature interacts with a particular dev environment or tool, but rather to recognize that there is room for improvement.

Seeing that this feature was introduced in less than a day and with a non-negligible amount of controversy, we have more than enough grounds for at least revisiting it. I'm not saying that my particular solution be the one we go for, it's just a starting point.

@AnyhowStep
Copy link
Contributor

The name of a function parameter is not part of its type, or signature.

declare function foo (a : number) : void;
declare function bar (_a : number) : void;

The above are the same type, and those types do get erased. Parameter names just aren't part of a function's type.

@weswigham weswigham added Suggestion An idea for TypeScript Awaiting More Feedback This means we'd like to hear from more people who would be helped by this feature labels Jan 3, 2019
@weswigham
Copy link
Member

The idea behind this issue is not to discuss the particulars of how this feature interacts with a particular dev environment or tool, but rather to recognize that there is room for improvement.

Any "improvement" can only be recognized in some context... that's why you keep getting asked. Nobody wants a change for change's sake - every change we make is targeted at satisfying particular use-cases or issues. I don't see us adding some new type variant just for tracking the suppression of the noUnusedParameter warning - it doesn't even behave like a real type. Types flow from point A to point B in a program along with values assigned that type - this marker would do... nothing... other than suppress an error... which is something we already have the ability to do. You can use the convention-based suppression (_ name prefix) or the comment-based suppression (@ts-ignore).

Unless you can present an actual reason why the other options available fail in some context and why adding behavior to TS is the best way to improve this scenario in that context, I don't see us doing anything like this in the near future. Plus, enumerating that context lets us explore likely more fruitful alternatives to alleviate whatever inconvenience prompted this.

@Qix-
Copy link

Qix- commented Jan 7, 2019

At the very least, allow us to use _ more than once, or some way to indicate that a parameter is unused, similar to C's (void) foo;.

I run into this all the time - where I'm adhering to some interface or abstract method and I just don't need one of the parameters that is being passed down.

Linters for Javascript have the logic that reports any unused parameters after the last used parameter. For example, if function (a, b, c, d, e) uses a and c, then only d and e are reported as unused since b is required to be there in order to have proper parameter ordering.

It's not clear how to work around this in Typescript. While I agree that unused as a type isn't the best way, having some means to signify that a parameter is unused would be nice.

@pronebird
Copy link

pronebird commented Feb 4, 2019

@aryzing in Electron, event handles are the case where the Event itself can be useless if you don't wish to send something back. I believe same applies to DOM events in many cases.

const handleEvent = (_ipcEvent: Electron.Event, result: string) => { // tslint error
 /// code
}

@RyanCavanaugh
Copy link
Member

?

type unused = unknown;

const x = (_event: unused, result: string) { 

@Qix-
Copy link

Qix- commented Feb 4, 2019

After revisiting this I still think allowing multiple underscores is a good thing. It cuts down on noise and immediately indicates intent to ignore while keeping types intact, which is important for a variety of reasons.

@innanyun
Copy link

innanyun commented Apr 12, 2020

My solution: prefix unused parameter name with "_unused" (and optionally with camelCasing).

function handleEvent(_unusedEvent: Event, result: string) { ... }

Two rationales behind this:

  1. Follow "explicit is better than implicit" rule from Python Zen.
  2. No changes needed in existing language or compiler features. Existing "_" prefixing solution is enough.

@mjackson
Copy link

At the very least, a function parameter should not be considered to be "unused" if a parameter that comes after it is used. In that case, the "unused" parameter is actually serving a purpose by taking up a slot in the argument list.

function fun(a, b) {
  // The `a` parameter should not be considered "unused"
  console.log(b);
}

@infacto
Copy link

infacto commented Aug 19, 2020

Why not just detect that b is used and don't claim a as unused. (Without any other changes line underscore or special unused type.) Because that's just logical. This should be automatically detected by TypeScript (which is already done. But don't claim it. Just inform on hover.) A "unused" type is not a good idea in my opinion. And underscore may be left by humans. So also not the best solution. But if I see that correctly, it's currently not a warning or error. So it's ok, isn't? It's just grayed-out (not underlined / marked) in vscode with a information about unused variable on hover. Which is a correct message. But neither the compiler nor the linter must fail here. It works correctly at this moment. So no need for changes. Or did I miss something?
At most maybe just don't consider the parameter a as unused if b is used. No message about, not grayed-out (maybe a vscode issue). But nothing critical I think.

@aryzing
Copy link
Author

aryzing commented Aug 20, 2020

Seems @mjackson hit the nail on the head -- I take my suggestion back. If the compiler can be tweaked to work as described above, that may be all we need.

@dsogari
Copy link

dsogari commented Mar 25, 2024

You could declare a type alias for this:

type unused = unknown;

And tweak the linter to understand that it means "unused parameter" and should only be used in function parameters.

@RyanCavanaugh RyanCavanaugh added Declined The issue was declined as something which matches the TypeScript vision and removed Awaiting More Feedback This means we'd like to hear from more people who would be helped by this feature labels Mar 25, 2024
@RyanCavanaugh
Copy link
Member

This has a very high ratio of downvotes and doesn't seem like a good solution to the problem.

If you need more advanced configuration (only allow me to suppress with a prime number of underscores, etc), there are widely-available highly-configurable linters for the purpose. The check as exists in TS is straightforward and we don't really want to add more lint-like checks.

At the very least, a function parameter should not be considered to be "unused" if a parameter that comes after it is used. In that case, the "unused" parameter is actually serving a purpose by taking up a slot in the argument list.

function fun(a, b) {
  // The `a` parameter should not be considered "unused"
  console.log(b);
}

I feel like this misapprehends the point of a "no unused" check. It's a useful lint because it detects straightforward mistakes like this

function fun(a, b) {
  alert(`Welcome to ${b}, ${b}, I hope you have a nice day`);
}

where you meant to write

function fun(a, b) {
  alert(`Welcome to ${a}, ${b}, I hope you have a nice day`);
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Declined The issue was declined as something which matches the TypeScript vision Suggestion An idea for TypeScript
Projects
None yet
Development

No branches or pull requests