Enhancement: [no-redundant-property-definitions] Rule to detect redundant visibility definitions in 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 extends A {
  a = 2;
}

Or this?

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

class B extends A {
  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 :)

[JoshuaKGoldberg]

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.

Right, and I think what we're saying is:

• The root issue around accidental redundant property re-defines is common to both parameter properties and traditional properties
• There's no need to limit detection for this issue to just one of those two types of properties

I'm mildly in favor of a generalized rule, like no-redundant-property-definitions, for the cases of declaring a property on a derived class with an identical signature to a parent. Slightly reducing one of the original examples:

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

class Derived extends Base {
    constructor(public a: string) {
        super(a);
    }
}

• Playground with Derived extends Base
• Playground with Derived extends Base and useDefineForClassFields

Agreed this new rule would need type information. There are subtle runtime differences, so I don't think the rule couldn't auto-fix. It could only report with suggestions.

P.S. @Josh-Cena I edited in extends A to your class Bs, was that intended?

[thisrabbit]

I'm mildly in favor of a generalized rule, like no-redundant-property-definitions, for the cases of declaring a property on a derived class with an identical signature to a parent.

@JoshuaKGoldberg Did you mean code like:

class A {
  public a: string;
}

class B extends A {
  public a: string;
}

I think this is a good idea that expands the original use case and strengthens the implementation value.

Playground with Derived extends Base and useDefineForClassFields

Thank you for your link, from which I learned that TypeScript can compile a class property declaration to Object.defineproperty.

I also attached a playground link that demonstrates under both compile options, any further audition to class property in the parent class will be reset by the derived class.

And if this rule is going to be expanded to no-redundant-property-definitions, the mixed-use of traditional class property declaration and ts parameter-property should also be detected (The link above demonstrates this).

[JoshuaKGoldberg]

Coming back to this, we chatted a bit as a team and think:

• This would be valuable in some cases 👍
• Those cases are not very frequent:
  • Although class inheritance is still very much a real pattern in JS/TS, it's slowly becoming less and less popular over time
  • The lack of previously filed issues over the years, or engagement from other community members over this issue's ~2 months, indicates it's not an often-thought-of need

We don't have the maintenance budget to work on every useful-in-some-cases rule proposed. If a lot of community members pitch in over the next few months expressing then that would be a signal we should put this one in scope. If not it might make more sense to tackle as a third-party plugin. I've added the evaluating community engagement label to indicate all that.