noUnusedLocals doesn't error when using a ...rest declaration

[scamden]

🔎 Search Terms

desctructure noUnusedLocals rest

🕗 Version & Regression Information

• This is the behavior in every version I tried, and I reviewed the FAQ for entries about noUnusedLocals

⏯ Playground Link

https://www.typescriptlang.org/play/?noUncheckedIndexedAccess=true&noUnusedLocals=true&noUnusedParameters=true&target=99#code/GYVwdgxgLglg9mABAWznAFASkQbwFCKGIQIDOUiAhogLy7UBciAjADSIBGiTATAL4EiJMOVyd2AOikAnAKai+tKgG5BhNcTIUIS5qoFA

💻 Code

function moo() {
    const A = {a : 1, b : 2}
    // no error on b
    const { b, ...rest } = A;
    // error on a as expected
    const { a } = A
    const c = 1;
}

🙁 Actual behavior

no error on an unused local

🙂 Expected behavior

error on any unused local whether a ...rest param is used or not

Additional information about the issue

No response

[MartinJohns]

This is working as intended. It's indirectly used by removing the variable from the rest. If you remove it, you will get different behavior (it's suddenly part of the rest again).

[scamden]

Hmm yes but the variable itself is unused and I expected an error there.. I can always rename it with an underscore prefix to avoid putting it back in the rest.

I’ve since enabled the typescript-eslint version of this rule which lets me error for this so I’m unblocked personally but this seems pretty unexpected

[MartinJohns]

Completely expected to me. :-) And intentional, see #12778. Having to write b: _b for every property would really be a bloated bummer.

[scamden]

Yes but forgetting to use a variable you intended to use is a correctness bummer which bothers me more than typing a few more characters.

The linked issue seems to assume that destructured properties with a rest sibling are only intended to remove properties from the rest element but in my experience that’s actually the less common intention. Usually, I intend to use both the destructured properties and the rest element.

I can see how this is convenient when your intention is to use destructuring as a poor man’s omit but I would argue that using an actual omit utility would be more clear for that whereas it’s more clear and expected to use all local declarations.

Regardless as I said I’m unblocked but I do feel strongly that trading correctness for convenience is a poor trade off.

[nmain]

correctness

Having an unused variable is not a lapse in correctness, and warnings or errors for such situations are closer to lints than true compiler errors. Given that, exactly what to error on is more of a judgment call and convenience is important.

[snarbles2]

It is inconsistent and would have confused me just the same to discover that noUnusedLocals acts more like someUnusedLocals.

[RyanCavanaugh]

We shipped it the other way and got several bug reports saying that there "are used", so changed it. Then it was that way for ~nine years without anyone else saying anything. People are entitled to their own opinions but the balance of evidence seems to be that the current behavior is more likely to be what someone expects.

[scamden]

People are entitled to their own opinions but the balance of evidence seems to be that the current behavior is more likely to be what someone expects.

Fair enough though it seems opinion is fairly split in the tiny sample size of comments on this issue :)

I don't understand how you can possibly be "blocked" on what is effectively a lint warning not showing up.

I'm leaning on typescript here to ensure correctness of some methods (e.g. make sure I used every property of a given object). I was only blocked in the sense that it wasn't ensuring that correctness. Using the term somewhat loosely. (Update: just noticed you removed this line since the email notification i got. But leaving my response to clarify all i meant by blocked )

[scamden]

It's pretty straightforward to show why this is in fact a correctness issue in some cases:

function transformObject(object : {name : string, name2? : number}) : {translatedName : string, translatedName2? : number} {
  const {name, name2, ...rest} = object;

  return {
    translatedName : name,
    // whoops! we're dropping name2 without warning
   ...rest,
  }

Update: added the rest parameter so it actually applies to the filed bug

(In my case I'm actually doing assertEmptyObject(rest) to ensure I've really translated and dealt with every property)

[typescript-bot]

This issue has been marked as "Working as Intended" and has seen no recent activity. It has been automatically closed for house-keeping purposes.