Tslint: [Suggestion] Add rule "no-static-members" to disallow usage of static class members

[avchugaev]

Please add rule like "no-static-members: true" to disallow usage of static class members.
This restriction will be very useful for teams/developers who tries to avoid static methods in their code.
I think it should work for public static and static protected members. Probably it should allow private static members.

Allow

Class-level constants (static properties marked as readonly):

public static readonly DEFAULT_CAPACITY: number = 100;

protected static readonly PROTECTED_PROPERTY_KEY: symbol = Symbol();

private static readonly PRIVATE_PROPERTY_KEY: symbol = Symbol();

Also there are specific static methods that returns instances of class they defined on:

• named constructors
• factory methods

I think such methods should also be allowed because:

• there is not support of multiple constructors
• static methods that creates class instances allows to keep incapsulation

To detect such methods it's enough to check type of method's return value.

export class SomeService {
    private static _instance: SomeService;

    public static getInstance(): SomeService {
         // get or create singleton
    }
}

const klass: Class = Class.of(SomeService);

Disallow

All other static class members - methods, accessors, properties without readonly modifier etc

[JoshuaKGoldberg]

Ha, this seems like it would be a very contentious rule. Some folks would argue that, under some codebases, it's preferable to group constructs directly onto the classes they're logically associated with. Others would argue that you shouldn't ever have classes in the first place.

Maybe this should go into tslint-microsoft-contrib instead?

/cc @adamtreineke because we've spent a few afternoons screaming at each other over this.

[avchugaev]

Hi @JoshuaKGoldberg!

this seems like it would be a very contentious rule

I cannot agree with you in this. In object-oriented programming static method considered bad practice:

• Static methods represents global shared state
• Static methods are hard to mock and test
• Static methods breaks encapsulation

Keeping this in mind developers should avoid creating and using static methods.
Here is nice article explaining why developers should avoid static methods.

Some folks would argue that, under some codebases, it's preferable to group constructs directly onto the classes they're logically associated with.

You're right. As I know, all rules can be enabled or disabled. I think those guys may ignore this rule if they have good reason. But again, in true object-oriented programming static methods are source of many problems. There are only few reasons to use static members: for example, to create util class which extends some of default types for example, some kind of StringUtils with additional methods not present in language specification, or Assert with assertion methods (they never encapsulate any state so it's okay to make them static).

[JoshuaKGoldberg]

Oh, I personally agree with your points and would absolutely push to enable this rule in my projects. The issue is not whether this is a _good_ suggestion, but rather whether it's too opinionated or tied into a particular line of thought. Can it be made generic enough to justify being in TSLint core? More specifically: how can this rule be constructed to _(mostly)_ flag the really bad uses of statics while _(mostly)_ allowing the reasonable ones? If this is to get into TSLint core without first being in tslint-microsoft-contrib or another repository, it should safely fall into that safe space.

As prior art, no-unnecessary-class exists in TSLint core because the extra class it guards against is purely bloat with approximately no benefit. One could just use exported functions or an {} object literal in pretty much all cases _(oversimplified, but vaguely accurate)_.

[avchugaev]

Thank you for more detailed explanation, @JoshuaKGoldberg.
For me, the proposed rule is "on the same level of abstraction" as member-access, prefer-const and prefer-readonly. They all designed to make code more predictable and safe.

If you say that it's good practice to put new rules into separate project, I will trust your expirience.

Here are code samples to look at the whole picture:

export class Demo {
  // ALLOW static read-only property (aka constant)
  public static readonly VALUE: number = 0;
  protected static readonly VALUE: number = 0;
  private static readonly VALUE: number = 0;

  // FORBID static getters and setters
  public static get someProperty() { /* return value */ }
  public static set someProperty(value) { /* save value */ }
  protected static get someProperty() { /* return value */ }
  protected static set someProperty(value) { /* save value */ }
  private static get someProperty() { /* return value */ }
  private static set someProperty(value) { /* save value */ }
}

export class Class {
  // ALLOW - named constructor
  public static of(type: Type): Class { /* get class reflection */ }
  // FORBID - method without encapsulation; not a named constructor
  public static getName(klass: Class): string { /* get class name */ }
  // FORBID - procedural programming approach, no encapsulation
  private static someReusablePieceOfLogic(...args) {}
}

[avchugaev]

Rule configuration proposal

As for rule configuration, I see it like this:

Enabled rule

In this case linter should emit errors for static getters, setters and methods whose are not a named constructors.
This rule doesn't have automatic fixer - all changes should be performed manually by developer.

{
  "no-static-members": true
}

Disables rule (default)

{
  "no-static-members": false
}

[avchugaev]

Two additional thoughts came to my mind:

1. This rule requires type information to check static method return type
2. Allowed return type of static method should be equal to current class and not equal to any subclass or superclass.

Examples of named constructors:

class Promise {
  public static all(promises: Array<Promise>): Promise<any[]> {}
  public static resolve<V>(value: V): Promise<V> {}
  public static reject(error: any): Promise<never> {}
}

class Class {
  public static of(type: Type): Class {}
}

[JoshuaKGoldberg]

Note: per #4534, this issue will be closed in less than a month if no PR is sent to add the rule. If you _really_ need the rule, custom rules are always an option and can be maintained outside this repo!

[JoshuaKGoldberg]

💀 _It's time!_ 💀

TSLint is being deprecated and no longer accepting pull requests for new rules. See #4534. 😱

If you'd like to see this rule implemented, you have two choices:

• Recommended: Use typescript-eslint to lint your TypeScript code with ESLint ✅
• Legacy: Write it as a custom TSLint rule 🤷‍♂️

👋 It was a pleasure open sourcing with you!

_If you believe this message was posted here in error, please comment so we can re-open the issue!_

[JoshuaKGoldberg]

🤖 Beep boop! 👉 TSLint is deprecated 👈 _(#4534)_ and you should switch to typescript-eslint! 🤖

🔒 This issue is being locked to prevent further unnecessary discussions. Thank you! 👋