Skip to content
This repository has been archived by the owner on Mar 25, 2021. It is now read-only.

Check for newly introduced diagnostics before reporting failures (for unused import) #1608

Merged
merged 10 commits into from
Nov 23, 2016

Conversation

alexeagle
Copy link
Contributor

When a program is available, verify that fixes for no-unused-variable don't introduce new diagnostics. If a fix breaks the program, don't report that failure at all.

This re-uses TypeScript to verify that our failures are legitimate, without having to re-implement the TS logic for whether an import is actually unused. This capability can be used in other rules as well, ensuring that accepting tslint fixes won't break type-checking.

The downside is that it makes the check slower, especially in a codebase with many unused imports, since each file must be checked (and those with spurious failures must have each unused import checked).

Fixes #1569

@palantirtech
Copy link
Member

Thanks for your interest in palantir/tslint, @alexeagle! Before we can accept your pull request, you need to sign our contributor license agreement - just visit https://cla.palantir.com/ and follow the instructions. Once you sign, I'll automatically update this pull request.

@alexeagle
Copy link
Contributor Author

See also discussion in the linked issue. TypeScript can fix this particular issue in the future, but tslint might still benefit from this capability in other cases.

@alexeagle
Copy link
Contributor Author

Ping!

Copy link
Contributor

@jkillian jkillian left a comment

Choose a reason for hiding this comment

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

Looks good to me overall 👍 I think there may be merge conflicts now though, sorry!

}

export function checkEdit(ls: ts.LanguageService, sf: ts.SourceFile, newText: string) {
if (ls.hasOwnProperty("editFile")) {
Copy link
Contributor

Choose a reason for hiding this comment

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

typeguard function would be nice here

@@ -74,6 +81,9 @@ export function runTest(testDirectory: string, rulesDirectory?: string | string[
return ts.createSourceFile(filenameToGet, fileText, compilerOptions.target);
} else if (filenameToGet === fileCompileName) {
return ts.createSourceFile(fileBasename, fileTextWithoutMarkup, compilerOptions.target, true);
} else if (fs.existsSync(path.resolve(path.dirname(fileToLint), filenameToGet))) {
Copy link
Contributor

Choose a reason for hiding this comment

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

just curious, what's this case for?

Noah Chen added 2 commits November 23, 2016 17:20
# Conflicts:
#	src/language/rule/typedRule.ts
#	src/rules/preferForOfRule.ts
#	src/test.ts
#	src/tslintMulti.ts
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.

no-unused-variable rules incorrectly marks required import
5 participants