Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

Enhancement: [no-redundant-property-definitions] Rule to detect redundant visibility definitions in derived classes #10825

Open
Labels
enhancement: plugin rule optionNew rule option for an existing eslint-plugin ruleevaluating community engagementwe're looking for community engagement on this issue to show that this problem is widely importantpackage: eslint-pluginIssues related to @typescript-eslint/eslint-plugin
@thisrabbit

Description

@thisrabbit

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

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 ruleparameter-properties:

  • [Primary] Additionally check the error-prone usage of the featureparameter-property under the class inheritance:
    classFoo{constructor(publica:string){/* Empty */}}classBarextendsFoo{//          vvvvvv Error: Unnecessary TS visibility modifier for parameter property since the variable `a` is initialized by its parent classconstructor(publica:string,publicb:number){super(a);}}
  • [Optional] Merge the check by the ruleno-unnecessary-parameter-property-assignment to this rule.

Background

Currently, the ruleparameter-properties strictly reports the following TS parameter property usage whenOptions.prefer is set toparameter-property and conditions marked in comments are met:

classFoo{// vvv Any TS visibility modifierpublica:string;//          v No TS visibility modifierconstructor(a:string){//          ^  ^^^^^^ The same variable name and type as defined in line 2this.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) topreferparameter-property:

classFoo{constructor(publica:string){/* Empty */}}

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

constructor(publica: 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 ruleno-unnecessary-parameter-property-assignment detects it.

Motivation

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

classFoo{//          vvvvvv This makes parameter `a` a property of class `Foo`constructor(publica:string){/* Empty */}}classBarextendsFoo{//          vvvvvv This also makes parameter `a` a property of class `Bar`constructor(publica:string,publicb:number){super(a);}}

causing the assignment expressionthis.a = a appears twice in classFoo and classBar respectively (See JS code inthis playground).

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

  • defaultViewer.ts declares a classDefaultViewer that extends from a parent classAbstractViewerWithTemplate.
  • The ultimate parent class ofDefaultViewer can be traced back to classAbstractViewer inviewer.ts.
  • The parent classAbstractViewer declares a parameter property withpublic containerElement: Element.
  • The child classDefaultViewer also DECLARES a parameter property withpublic containerElement: Element and sends it to thesuper() call.
  • As demonstrated previously, this duplicates the assignment expression, causing similar problems that the ruleno-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 repositoryglideapps/quicktype, where

  • Dart.ts classDartRenderer -extends->ConvenienceRenderer.ts classConvenienceRenderer -extends->Renderer.ts classRenderer.
  • The first constructor parametertargetLanguage is markedprotected 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 thesuper call.

This check benefits a better use of the featureparameter-property.

Optional Goal

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

The base ruleparameter-properties was discussed in2019 and implemented in2022, and the ruleno-unnecessary-parameter-property-assignment was discussed in2023 and introduced later in2024. 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

classFoo{constructor(publica:string){/* Empty */}}classBarextendsFoo{//          vvvvvv Error: Unnecessary modifier, has already been initialized in the parent class.constructor(publica:string,publicb:string){super(a);}}

Pass

classFoo{constructor(publica:string){/* Empty */}}classBarextendsFoo{constructor(a:string,publicb: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.

Metadata

Metadata

Assignees

No one assigned

    Labels

    enhancement: plugin rule optionNew rule option for an existing eslint-plugin ruleevaluating community engagementwe're looking for community engagement on this issue to show that this problem is widely importantpackage: eslint-pluginIssues related to @typescript-eslint/eslint-plugin

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions


      [8]ページ先頭

      ©2009-2025 Movatter.jp