no-unused-variable TypeError: Cannot read property '1' of null

[hotforfeature]

Bug Report

• TSLint version: 5.10.0
• TypeScript version: 2.8.1
• Running TSLint via: CLI

TypeScript code being linted

import { A, B, C } from './file';

console.log('No imports used');

export const D = 4;

with tslint.json configuration:

{
  "rules": {
    "no-unused-variable": [true, {"ignore-pattern": "^_"}]
  }
}

Actual behavior

The 'no-unused-variable' rule threw an error in '/Users/andrew.mitchell/Documents/Projects/test/test.ts':
TypeError: Cannot read property '1' of null
    at walk (/Users/andrew.mitchell/Documents/test/node_modules/tslint/lib/rules/noUnusedVariableRule.js:105:54)
    at Rule.AbstractRule.applyWithFunction (/Users/andrew.mitchell/Documents/test/node_modules/tslint/lib/language/rule/abstractRule.js:39:9)
    at Rule.applyWithProgram (/Users/andrew.mitchell/Documents/test/node_modules/tslint/lib/rules/noUnusedVariableRule.js:32:21)
    at Linter.applyRule (/Users/andrew.mitchell/Documents/test/node_modules/tslint/lib/linter.js:194:29)
    at /Users/andrew.mitchell/Documents/test/node_modules/tslint/lib/linter.js:139:85
    at Object.flatMap (/Users/andrew.mitchell/Documents/test/node_modules/tslint/lib/utils.js:151:29)
    at Linter.getAllFailures (/Users/andrew.mitchell/Documents/test/node_modules/tslint/lib/linter.js:139:32)
    at Linter.lint (/Users/andrew.mitchell/Documents/test/node_modules/tslint/lib/linter.js:99:33)
    at /Users/andrew.mitchell/Documents/test/node_modules/tslint/lib/runner.js:209:32
    at step (/Users/andrew.mitchell/Documents/test/node_modules/tslint/node_modules/tslib/tslib.js:133:27)

Expected behavior

Rule should warn All imports in import declaration are unused. for line 1.

This is caused by noUnusedVariableRule.ts#L123 because the failure message is All imports in import declaration are unused., which does not have a variable name in it for the regex to find.

This rule works correctly if the ignore-pattern option is not specified since it won't try to check the variable name.

[jkillian]

Good find @hotforfeature. I started addressing this over in #3919 because it related to some changes coming up in TS 2.9 which will only exacerbate this issue.

My PR prevent an exception from being thrown, however, it simply ignores ignorePattern in the cases you mentioned which isn't great. I believe we'll need to add some complex code sort of like what we do for import autofixes right now :/

[jkillian]

I'm wondering if we should thing about deprecating this rule. tsc now provides a way to ignore its warnings, and it provides support for automatic fixes for its warnings through an IDE.

As far as I can see, the main benefit to this rule is ignore-pattern, but it's difficult to implement that correctly the way the rule is now written to depend off tsc diagnostics. TSLint is also a little more flexible than tsc w/ regards to disabling rules in certain files. @suchanlee any thoughts on all this? I know you have fixed this rule recently

[hotforfeature]

My primary use case for this rule is to autofix problems on commit in a git hook when linting. So I still think there's great value here even if tsc offers something via IDE integrations.

[suchanlee]

@jkillian i think that the rule is starting to become increasingly expensive to support with TSLint's support for multiple versions of Typescript and continued changes in Typescript behavior. And since Typescript supports the feature, we should lean towards using that. But since earlier versions of TS doesn't support it, I dont think that we should delete it. However i think we should start to think about deprecating the rule and work with the TS folks to better support the current TSLint workflows with TS.

[aervin]

I would second @suchanlee 's proposal. This rule comes up in issues a lot, it seems. Too many considering that recent TS versions warn about the problem adequately. Let's phase it out!

[jkillian]

@hotforfeature - acknowledge your usecase and find it reasonable. However, due to the complexity of the rule, I think we're going to deprecate it (see #3919).

As always though, anyone is welcome to copy the source code of the rule and move it to an external package and keep using/improving it that way!

[killtheliterate]

For what it's worth:

{
  "compilerOptions":  {
    "noUnusedLocals": true,
    "noUnusedParameters": true,
  }
}

Using the above in tsconfig allows for the no-unused-variable lint rule to be disabled.

[giladgray]

no-unused-variable is now deprecated, and the compilerOptions above is the official solution now.

[kachkaev]

Defining noUnusedLocals and noUnusedParameters in compilerOptions is not quite the same as tslint's no-unused-variable. As of now, unused vars either crash the build or remain unnoticed and there is no warning mode any more 😞

[atsu85]

@giladgray, @killtheliterate wrote:

Using the above in tsconfig allows for the no-unused-variable lint rule to be disabled.

I don't want to disable this rule, because as @kachkaev wrote

Defining noUnusedLocals and noUnusedParameters in compilerOptions is not quite the same as tslint's no-unused-variable

My use-case is generated code. I want my manually written code to follow this rule, but to avoid making code-generator enormously complex, i'd like to disable this rule in generated code (or at least in some sections of generated code) - that is smth that can't be done with TypeScript compiler options 😞

[aleclarson]

At the very least, this deprecation should be documented on the TSLint website, but I strongly agree that this deprecation was premature. Any plans to re-consider? Would a community contribution be accepted?

[maxbogue]

This deprecation is ridiculous. The compiler flags are a TERRIBLE replacement. They break the build and don't fix themselves. How is that a reasonable solution to point people to? If the ignore-pattern option is too hard to make work now, deprecate that instead.

[JoshuaKGoldberg]

👋 folks - just linking this to #4232. It's well understood & agreed that the built-in compiler options are not a sufficient replacement for no-unused-variable. The rule's original implementation didn't play well with other TS tooling and had to be disabled. #4232 tracks finding a way to enable it again.

In the meantime, you could use something like tsc --noEmit --noUnusedLocals --noUnusedParameters as a separate tool to use the built-in checks. Yes, that's not as good as a configurable no-unused-variables.

[adidahiya]

continue discussion in #4100 & #4232