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

feat(58561): Allow leading underscore for types to bypass noUnusedLocals warning #58884

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions src/compiler/checker.ts
Original file line number Diff line number Diff line change
Expand Up @@ -43174,6 +43174,7 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
}
}
}

function isTypeParameterUnused(typeParameter: TypeParameterDeclaration): boolean {
return !(getMergedSymbol(typeParameter.symbol).isReferenced! & SymbolFlags.TypeParameter) && !isIdentifierThatStartsWithUnderscore(typeParameter.name);
}
Expand All @@ -43194,6 +43195,13 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
}

function isValidUnusedLocalDeclaration(declaration: Declaration): boolean {
if (isTypeAliasDeclaration(declaration)) {
Copy link
Member

Choose a reason for hiding this comment

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

At some level, I wonder if we should just be checking isIdentifierThatStartsWithUnderscore for all declarations, rather than special casing a few kinds. What declarations are we actually missing here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this is open to discussion. The fix shouldn't require significant work... What do you think, @RyanCavanaugh?

Copy link
Member

Choose a reason for hiding this comment

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

I guess it's sort of nonsensical to do this for enum members and properties and so on; after all, their names are important.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jakebailey What are your thoughts on the following list? Should we add or exclude any additional declarations?

  1. Interface Declaration
  2. Type Alias Declaration
  3. Enum Declaration
  4. Class Declaration

Copy link
Member

Choose a reason for hiding this comment

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

I would say any declaration that can be a child of a Block or SourceFile or Module, etc, if that makes sense. So basically not enum members or properties. Not sure we have a list of that variety, though...

/**
* ignore starts with underscore names _
* type _T = number;
*/
return isIdentifierThatStartsWithUnderscore(declaration.name);
}
if (isBindingElement(declaration)) {
if (isObjectBindingPattern(declaration.parent)) {
/**
Expand Down
11 changes: 11 additions & 0 deletions tests/baselines/reference/unusedTypeDeclarations.errors.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
unusedTypeDeclarations.ts(1,6): error TS6196: 'T1' is declared but never used.


==== unusedTypeDeclarations.ts (1 errors) ====
type T1 = number; // error
~~
!!! error TS6196: 'T1' is declared but never used.
type _T2 = number; // ok

export {};

12 changes: 12 additions & 0 deletions tests/baselines/reference/unusedTypeDeclarations.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
//// [tests/cases/compiler/unusedTypeDeclarations.ts] ////

//// [unusedTypeDeclarations.ts]
type T1 = number; // error
type _T2 = number; // ok

export {};


//// [unusedTypeDeclarations.js]
"use strict";
Object.defineProperty(exports, "__esModule", { value: true });
11 changes: 11 additions & 0 deletions tests/baselines/reference/unusedTypeDeclarations.symbols
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
//// [tests/cases/compiler/unusedTypeDeclarations.ts] ////

=== unusedTypeDeclarations.ts ===
type T1 = number; // error
>T1 : Symbol(T1, Decl(unusedTypeDeclarations.ts, 0, 0))

type _T2 = number; // ok
>_T2 : Symbol(_T2, Decl(unusedTypeDeclarations.ts, 0, 17))

export {};

13 changes: 13 additions & 0 deletions tests/baselines/reference/unusedTypeDeclarations.types
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
//// [tests/cases/compiler/unusedTypeDeclarations.ts] ////

=== unusedTypeDeclarations.ts ===
type T1 = number; // error
>T1 : number
> : ^^^^^^

type _T2 = number; // ok
>_T2 : number
> : ^^^^^^

export {};

6 changes: 6 additions & 0 deletions tests/cases/compiler/unusedTypeDeclarations.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
// @noUnusedLocals: true

type T1 = number; // error
type _T2 = number; // ok

export {};