Enhancement: [parameter-properties] Support unnecessary-check under derived classes

[thisrabbit]

Before You File a Proposal Please Confirm You Have Done The Following...

• I have searched for related issues and found none that match my proposal.
• I have searched the current rule list and found no rules that match my proposal.
• I have read the FAQ and my problem is not listed.

My proposal is suitable for this project

• I believe my proposal would be useful to the broader TypeScript community (meaning it is not a niche proposal).

Link to the rule's documentation

https://typescript-eslint.io/rules/parameter-properties/

Description

TL;DR

This issue proposes the following changes to the rule parameter-properties:

• [Primary] Additionally check the error-prone usage of the feature parameter-property under the class inheritance:

  class Foo {
    constructor(public a: string) { /* Empty */ }
  }
  
  class Bar extends Foo {
    //          vvvvvv Error: Unnecessary TS visibility modifier for parameter property since the variable `a` is initialized by its parent class
    constructor(public a: string, public b: number) {
      super(a);
    }
  }

• [Optional] Merge the check by the rule no-unnecessary-parameter-property-assignment to this rule.

Background

Currently, the rule parameter-properties strictly reports the following TS parameter property usage when Options.prefer is set to parameter-property and conditions marked in comments are met:

class Foo {
  // vvv Any TS visibility modifier
  public a: string;
  //          v No TS visibility modifier
  constructor(a: string) {
  //          ^  ^^^^^^ The same variable name and type as defined in line 2
    this.a = a;
    // ^^^^^^^ The assignment expression AT THE BEGINNING of the constructor
  }
}

which expects it to be refactored as the following (Though no auto-fix was provided) to prefer parameter-property:

class Foo {
  constructor(public a: string) { /* Empty */ }
}

However, the TS feature parameter-property comes with an error-prone usage (duplication):

constructor(public a: string) {
  this.a = a;  // Unnecessary assignment expression since TS will generate this line while compiling to JS
}

A developer would not intentionally write code like this, so the rule no-unnecessary-parameter-property-assignment detects it.

Motivation

During our feature usage analysis, we found another error-prone usage involving the super call (that is, the class inheritance):

class Foo {
  //          vvvvvv This makes parameter `a` a property of class `Foo`
  constructor(public a: string) { /* Empty */ }
}

class Bar extends Foo {
  //          vvvvvv This also makes parameter `a` a property of class `Bar`
  constructor(public a: string, public b: number) {
    super(a);
  }
}

causing the assignment expression this.a = a appears twice in class Foo and class Bar respectively (See JS code in this playground).

This is not a niche proposal since a real-world case can be found in a famous repository BabylonJS/Babylon.js, where

• defaultViewer.ts declares a class DefaultViewer that extends from a parent class AbstractViewerWithTemplate.
• The ultimate parent class of DefaultViewer can be traced back to class AbstractViewer in viewer.ts.
• The parent class AbstractViewer declares a parameter property with public containerElement: Element.
• The child class DefaultViewer also DECLARES a parameter property with public containerElement: Element and sends it to the super() call.
• As demonstrated previously, this duplicates the assignment expression, causing similar problems that the rule no-unnecessary-parameter-property-assignment detects.

This case is considered a bad practice by us because, on the contrary, we also found the following best practice as demonstrated by the repository glideapps/quicktype, where

• Dart.ts class DartRenderer -extends-> ConvenienceRenderer.ts class ConvenienceRenderer -extends-> Renderer.ts class Renderer.
• The first constructor parameter targetLanguage is marked protected readonly in the parent class and not marked in the following derived classes.
• This only initializes the class property once at the parent class, preventing it from accidentally undoing the parent's initialization.

Proposal

Given the motivation described above, we would like to (mainly) propose the enhancement of the unnecessary-check under the class inheritance:

• Ideally, a cross-file inheritance chain analysis is desired, given the proposed scenario may involve two classes in different files.
• A loose constraint may simplify the implementation in case the precise cross-file type analysis is currently not available or time-consuming:
  • In a derived class's constructor, a parameter is marked with TS visibility modifier.
  • That parameter is sent to the super call.

This check benefits a better use of the feature parameter-property.

Optional Goal

Like the rule no-unnecessary-parameter-property-assignment, a developer would not intentionally write duplicated code once they determined to prefer: 'parameter-property'. We consider violations of best practices like these should be implemented seamlessly and automatically without another rule to turn on.

The base rule parameter-properties was discussed in 2019 and implemented in 2022, and the rule no-unnecessary-parameter-property-assignment was discussed in 2023 and introduced later in 2024. We have read all discussions in PRs and issues and found that merging them into one rule was not discussed previously. By the chance of enhancing the base rule, we would like to pose the discussion for a more concise and compact rule set.

Fail

class Foo {
  constructor(public a: string) { /* Empty */ }
}

class Bar extends Foo {
  //          vvvvvv Error: Unnecessary modifier, has already been initialized in the parent class.
  constructor(public a: string, public b: string) {
    super(a);
  }
}

Pass

class Foo {
  constructor(public a: string) { /* Empty */ }
}

class Bar extends Foo {
  constructor(a: string, public b: string) {
    //    v   ^ If the parameter is passed to the `super` call, it would most likely unnecessary to be a parameter property.
    super(a);
  }
}

Additional Info

Current rules can not detect the bad practice discussed in this proposal.

[Josh-Cena]

First, this rule is not type-aware and your proposal does not seem to address a core problem that's worth making the rule type-aware, so we are discussing a new rule, not an existing rule extension.

Second, how would that be different from this?

class A {
  a = 1;
}

class B {
  a = 2;
}

Or this?

class A {
  a;
  constructor(a) {
    this.a = a;
  }
}

class B {
  a = 2;
}

Or any other combination of these features? Why is this proposal specific to parameter properties, and not reinitialization of inherited class fields at large? Should we just have a rule that reports all kinds of class field overwriting?

[thisrabbit]

Thanks @Josh-Cena , and I assume in your code snippets, all class B extends class A since currently it's confusing that non-inherited classes are irrelevant to this proposal.

First, I agree with you to treat this proposal as a new rule rather than a rule extension.

Second, IMO rules are about warning developers of potentially unclear and unintentional code. In both of the code snippets you provided:

• It is explicit and clear that field a is overwritten.
• The context is insufficient to determine whether it is intentional by the developer.

However, in the context of this proposal, that is, in the following code:

class A {
  constructor(public a: string) {}
}

class B extends A {
  constructor(public a: string, public b: any) {
    super(a);
  }
}

how would that be different from this?

• The use of parameter-property on class B's public a is proved to be redundant since we see other developers (correctly) omit the modifier public (Please refer to the example we provided in the proposal).
• This specific case suggests an implicit and (most likely) unintentional use and can be fixed without any side-effect.

Should we just have a rule that reports all kinds of class field overwriting?

This proposal is not about class field overwriting. This proposal intends to report an implicitly duplicated re-assignment due to the misuse of the TS feature parameter-property.

Why is this proposal specific to parameter properties, and not reinitialization of inherited class fields at large?

This proposal was inspired by an existing rule no-unnecessary-parameter-property-assignment and intended to cooperate with it to make the guidance on this feature more complete. Retargeting this proposal to general reinitialization of inherited class may be a good idea but developers' intention behind the code and what patterns can be observed are not well-studied as what is in this proposal.

Hope this makes things more clear :)