Rule proposal: no-unused-private-class-members

[alumni]

Description

To complement the original no-unused-private-class-members rule which works with typescript but doesn't detect the private keyword:

Private class members that are declared and not used anywhere in the code are most likely an error due to incomplete refactoring. Such class members take up space in the code and can lead to confusion by readers.

— From: https://eslint.org/docs/rules/no-unused-private-class-members#no-unused-private-class-members

See also: eslint/eslint#14859.

Fail

class Foo {
    #unusedMember: number;
    private anotherUnusedMember: number;
}

class Foo {
    #usedOnlyInWrite: number;

    constructor(private anotherUsedOnlyInWrite: number) {
        // noop
    }

    method() {
        this.#usedOnlyInWrite = 42;
    }
}

class Foo {
    #usedOnlyToUpdateItself = 5;

    method() {
        this.#usedOnlyToUpdateItself++;
    }
}

class Foo {
    #unusedMethod() {}
    private anotherUnusedMethod() {}
}

class Foo {
    get #unusedAccessor() {}
    set #unusedAccessor(value) {}
    private get anotherUnusedAccessor() {}
    private set anotherUnusedAccessor(value) {}
}

Pass

class Foo {
    #usedMember = 35;
    private anotherUsedMember = 7;

    method() {
        return this.#usedMember + this.anotherUsedMember;
    }
}

class Foo {
    constructor(private readonly usedMember: number) {
        // noop
    }

    doSomething() {
        return this.usedMember / 42;
    }
}


class Foo {
    #usedMethod() {
        return 35;
    }

    private anotherUsedMethod() {
        return 7;
    }

    anotherMethod() {
        return this.#usedMethod() + this.anotherUsedMethod();
    }
}

class Foo {
    get #usedAccessor() {}
    set #usedAccessor(value) {}
    private get anotherUsedAccessor() {}
    private set anotherUsedAccessor(value) {}
    
    method() {
        this.#usedAccessor = 42;
        this.anotherUsedAccessor = 42;
    }
}

[bradzacher]

#private members are completely safe to mark as unused because they are 100%, runtime enforced to be only accessible within the class they are defined.

However private members are not. They are accessible outside the class via instance['member'] (yes, that's valid and safe TS). And are ultimately just type-only sugar for a normal class member.

Which means we cannot statically detect the valid usages across module boundaries.

This is generally I would recommend using #private instead of private in all modern code.

[alumni]

Actually, TypeScript is already complaining about private members not being used. I could enable the noUnusedLocals option to make that an error, I would just be using tsc instead of eslint.

[bradzacher]

Yes, but I don't know if the compiler option actually detects the indexed access across files.

[alumni]

Actually the compiler detects indexed access, but not across files. See Car.wheels in codesandbox.io/s/agitated-ramanujan-vwpyxv - I set it up locally and ran tsc --noUnusedLocals --noEmit src/*.ts (the editor is a bit confused).

Which is fine for me because I don't ever want to access private members outside of a class. The problem is that inside the same file it doesn't complain. Not that big of a deal though, since even without an eslint rule for it, I haven't seen anyone using indexed access - and it would much easier to spot during code review than unused code.

It would be nice to get private to behave like # during linting since most of the people prefer TS access modifiers to ES access modifiers. Or to have a rule that automatically converts private to # until people get used to it (probably not possible since # doesn't work in the constructor signature).

[bradzacher]

The problem is that inside the same file it doesn't complain

our dot-notation rule will let you enforce people don't do this silly stuff.

Or to have a rule that automatically converts private to # until people get used to it

An autofixer would not be possible for this because it wouldn't be able to fix cross-module access!

In general we don't like writing rules that just ban a feature unless we can add real value with a standalone rule.
https://typescript-eslint.io/docs/linting/troubleshooting/#how-can-i-ban-specific-language-feature

Here's an example config which bans private members

[snewcomer]

@bradzacher Thanks for the detailed reply here. How do you think we can get a fixer in to ban and autofix the no-restricted-syntax for private members to #private? Can we create a rule that extends from no-restricted-syntax that just includes the fixer? If you have a sol'n you would be happy with, I'll open a new issue.

---- EDIT ----

I reread and noticed you already said a fixer is not possible. Ignore me!

[snewcomer]

Also, unit tests relying on any private methods would also start failing if they weren't treated as block boxes. Wow, much harder problem than I thought!

[Zamiell]

Using # versus private

This is generally [why] I would recommend using #private instead of private in all modern code.

This was discussed in the TypeScript Discord a few times and I do not think there is a consensus on this point. I think there are some valid reasons why "modern code" might want to prefer the private keyword over #. Consider the following arguments.

# Run-Time Safeguards Are Superfluous - ✅ Win for private

Run-time safeguards seem mostly useful for the situation where you don't want JavaScript consumers to do unsafe things. For example, consider the discord.js library: it is written in TypeScript, and expects both JavaScript and TypeScript users to consume the library.

However, I'd argue that the discord.js situation is not the default situation. In many cases, we are working on an application that is completely written in TypeScript, or a library that is written in TypeScript and has only TypeScript consumers.

In these situations, we don't need the run-time safeguards - the TypeScript compiler will enforce that it is impossible for any private method to be improperly called at compile-time. And other lint rules, such as dot-notation, can forbid the escape hatch (if needed).

private is Easier to Troubleshoot - ✅ Win for private

So if the extra run-time functionality of the # annotation is not needed for some (or most?) applications, then why not just use it anyway?

By allowing access at run-time, troubleshooting your app becomes a lot easier - you can simply type in the names of the private variables into the console.

private is More Testable - ✅ Win for private

Furthermore, you can explicitly use the intended TypeScript escape hatch in your test files, as recommended by mhegazy in the aforementioned thread.

private is Less Verbose - ✅ Win for private

One notable difference between private and # is that the former is declarative, and the latter is not. In other words, with private, you only have to specify it once. With #, you have to repeat it all over your code base and type it every single time you access the variable or method.

So, private better satisfies the DRY principle. As for whether or not scattering the # symbol all over a class makes it easier or harder to read, that is debatable. But I'm in the "harder" camp, since having # all over the place is a bit noisy.

private is Easier to Understand - ✅ Win for private

A primary goal of code is that it should be as easy to understand as possible. (e.g. Guido's quote: "Code is read more often than it is written.", etc.)

If you agree with that premise, then that's a (slight) reason for preferring private.

On the one hand, someone onboarding with a codebase will quickly learn that "Oh, # means a private variable, got it."

But on the other hand, there's no need to have this friction at all. Using public and private are really intuitive for most programmers, as there are no arcane symbols to decode.

private is More Explicit - ✅ Win for private

In one sense, using # is more explicit, because you are communicating to the reader at every point along the line, "this is private!"

However, in other ways, it is less explicit. Anything without the # annotation is public by default. So, in a class that has a mixture of public and private methods, a programmer may accidently forget to specify the # annotation on a new method. In this situation, there isn't really a lint rule that can save us.

On the other hand, the @typescript-eslint/explicit-member-accessibility rule is an excellent fit for a codebase that has classes with a good mix of public and private methods. The rule forces the programmer to write public or private on every single method, which is very explicit. But by forcing them to choose public or private at declaration time, it forces them to explicitly think about the proper scope of the method, which is a useful pattern to prevent bugs.

In the world where we only use #, there does not seem to be an analogous pattern.

private is Further Away from JavaScript - ❌ Fail for private

Of course, there is one other main reason to prefer # over private: it's closer to JavaScript. Writing code with less TypeScript-specific features can be desirable as a way to future-proof the code.

There has been a movement in the TypeScript ecosystem to prefer JavaScript-native code over TypeScript-specific code, as we envision that in the long-term, all of the good TypeScript features will become ECMA proposals and be integrated into the language for real, and then we can drop TypeScript completely.

So, if you care about that kind of thing, then you would want to embrace # now, despite its other shortcomings.

Personally, I'm not sold that private will ever become deprecated. And I don't think that this "pro" outweighs the other downfalls established above.

How to Lint Unused private Members

noUnusedLocals

As previously mentioned in this thread, the TypeScript compiler can enforce unused private members with the noUnusedLocals flag. However, it seems to be best practice in the ecosystem to turn this flag off in favor of the @typescript-eslint/no-unused-vars rule. (This is also a practice followed by this repository.)

The reasons for this are two-fold:

1. It is desirable for "warnings" to come from ESLint and type errors to come from TypeScript, which allows for a clearer delineation of the severity of a given problem.

2. The @typescript-eslint/no-unused-vars rule has a handy feature where you can quickly make a variable exempt from the rule by prefacing it with an underscore (or some other arbitrary pattern, but conventionally an underscore is used). This is useful because during development, you might want to quickly silence the unused variable warning so that it no longer obfuscates the other errors/warnings that you care about. The TypeScript compiler does not have an analogous feature.

no-unused-private-class-members

Obviously, the @typescript-eslint/no-unused-vars rule does not account for unused private methods, but this is where the @typescript-eslint/no-unused-private-class-members rule comes in. Or at least, where it is supposed to come in, as the rule logic needs to be updated to account for private.

The Gap

Thus, for these two reasons (desirability of private over # + desirability of no-unused-private-class-members over noUnusedLocals), a PR for this functionality is still needed. I'll see about doing one in the future if no-one else wants to.

[JoshuaKGoldberg]

@PhilippeMorier please see our contributing guide's section on issue comments: https://typescript-eslint.io/contributing/issues#commenting. We ask that folks don't post "is there an update?" messages on issues. They don't add to the discussion. If nothing is visible on GitHub, then nothing has happened.

Zamiell is waiting for our re-review on some other PRs (sorry for the delay 😅) as it is.