-
Notifications
You must be signed in to change notification settings - Fork 13k
Feat/58561 allow leading underscore to bypass no unused locals #62323
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
base: main
Are you sure you want to change the base?
Feat/58561 allow leading underscore to bypass no unused locals #62323
Conversation
@microsoft-github-policy-service agree |
57bfbb5
to
2c295fd
Compare
This commit introduces new tests against the baseline (ie, unchanged from before this PR) `isTypeParameterUnused` implementation.
This PR continues [Allow leading underscore for types...](microsoft#58884), taking into account feedback from https://github.com/microsoft/TypeScript/pull/58884/files#r1643108204. The initial commit on this PR is original work by @a-tarasyuk. The second commit enhances the test suite and reverts the change to checker.ts to get a new baseline on the test suite for easier comparison with the changes. The third commit changes checker.ts to allow any unused local if it is prefixed with an underscore. This has the effect of newly allowing the following unused declarations: ```ts // Variables were previously allowed only in destructuring and for-loops const _unusedVar = 2; let _unusedLet = 4; var _unusedVar2 = 6; // These two no longer raise // error TS6198: All destructured elements are unused const { _a, _b } = { _a: 1, _b: 1 }; function f2({_a, _b}) function _unusedFunc() { } const _unusedArrow = () => { }; class _UnusedClass { } interface _UnusedInterface { } type _UnusedType = string; enum _UnusedEnum { A } namespace _UnusedNamespace { ... } ``` As far as I can tell, enum members and properties are not checked in the checkUnusedLocalsAndParameters function, and we do not need to special-case them. Closes microsoft#58561
2c295fd
to
fdd4051
Compare
@@ -44438,7 +44423,8 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker { | |||
|
|||
if (local.declarations) { | |||
for (const declaration of local.declarations) { | |||
if (isValidUnusedLocalDeclaration(declaration)) { | |||
const name = getNameOfDeclaration(declaration); | |||
if (isAmbientModule(declaration) || name && isIdentifierThatStartsWithUnderscore(name)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I do not include this isAmbientModule(declaration)
guard, then I get failures in two fourslash tests (moduleDeclarationDeprecated_suggestion1.ts
and moduleDeclarationDeprecated_suggestion1.ts
), which both start unexpectedly surfacing "'__global' is declared but its value is never read.",
. I was not able to figure out how to fix those tests.
But I also was not able to figure out any way to exercise this guard in tests/case/compiler
tests. That is, I was unable to come up with a test case that would raise an unused declaration
or declared but its value is never read
, even when I did not include this isAmbientModule(declaration)
. Here’s an example of what I tried.
```ts
// @Filename: node_modules/library/index.d.ts
declare function get(): string;
export { get };
// @Filename: a.ts
declare module "library" { // ok - ambient modules are not reported as unused
export function init(): void;
}
declare global { // ok - global augmentation is not reported as unused
interface Window {
customProperty: number;
}
}
Probably this doesn’t matter. After all, there was no test validating that we don’t error on unused ambient modules even before this change, but I still thought I’d call it out.
This change ends up allowing a whole bunch more types of declarations to be unused without error if they start with an underscore. I summarized the changes in the commit / PR message, and I also made a test case for every change in behavior that I could identify, and you can clearly review the changes here fdd4051#diff-a2225026b5eacf50cf8e3488e7b6e5768cf8952b3e2829cc0589624438397bab . I did my best to implement the @jakebailey’s suggestion on a previous PR. The behavior after this PR is what I would personally want, but I don’t have a ton of TypeScript experience and so I’m not sure what community expectations are. Regardless, I would be happy to iterate if people want anything tweaked. I also wonder if this change warrants an update to the docs to note the new behavior, which I would be happy to include if the maintainers agree. |
The TypeScript team hasn't accepted the linked issue #58561. If you can get it accepted, this PR will have a better chance of being reviewed. |
Allow underscore prefix to bypass noUnusedLocals warning
This PR continues Allow leading underscore for
types...,
taking into account feedback from https://github.com/microsoft/TypeScript/pull/58884/files#r1643108204.
The initial commit on this PR is original work by @a-tarasyuk. The second
commit enhances the test suite and reverts the change to checker.ts to
get a new baseline on the test suite for easier comparison with the
changes. The third commit changes checker.ts to allow any unused local if
it is prefixed with an underscore. This has the effect of newly allowing
the following unused declarations:
As far as I can tell, enum members and properties are not checked in the
checkUnusedLocalsAndParameters function, and we do not need to special-case
them.
Closes #58561