eslint / eslint

Find and fix problems in your JavaScript code.
https://eslint.org
MIT License
24.44k stars 4.41k forks source link

Rule Change: Ignore class with Static initialization blocks in no-unused-vars #17764

Closed erosman closed 6 months ago

erosman commented 6 months ago

What rule do you want to change?

no-unused-vars

What change to do you want to make?

Generate fewer warnings

How do you think the change should be implemented?

A new default behavior

Example code

/* eslint no-unused-vars: "error" */

class ClassWithSIB {
  static {
    // …
  }
}

What does the rule currently do for this code?

'ClassWithSIB' is defined but never used. (no-unused-vars)

What will the rule do after it's changed?

Ignore classes with Static initialization blocks in no-unused-vars

Participation

Additional comments

In a function, auto-run is achieved using IIFE (Immediately Invoked Function Expression).

In a class, auto-run is achieved using static initialization blocks.

Static initialization blocks

Static initialization blocks are declared within a class. It contains statements to be evaluated during class initialization. This permits more flexible initialization logic than static properties, such as using try...catch or setting multiple fields from a single value. Initialization is performed in the context of the current class declaration, with access to private state, which allows the class to share information of its private properties with other classes or functions declared in the same scope (analogous to "friend" classes in C++).

A self-contained class with a "static initialization block" is a valid OOP programming method and thus should be ignored in "no-unused-vars".

footnote: This is a clarified, better-targeted request based on #17757

fasttime commented 6 months ago

Hi @erosman, thanks for the issue. Could you explain what the class name is doing in your example?

Most typically, a declaration that is never referenced can be replaced with an expression regardless of the logic inside.

/* eslint no-unused-vars: "error" */

// Incorrect:

class SomeClass {
  static {
    // …
  }
}

const someVar = foo();

// Correct:

(class {
  static {
    // …
  }
});

(class SomeClass {
  static {
    // …
  }
});

foo();
erosman commented 6 months ago

Classes in JavaScript

In the last article, we introduced some basic concepts of object-oriented programming (OOP), and discussed an example where we used OOP principles to model professors and students in a school.

We also talked about how it's possible to use prototypes and constructors to implement a model like this, and that JavaScript also provides features that map more closely to classical OOP concepts.

It is my understanding that ...

In this topic, we are concentrating on the static class type. Once a static class in OOP is created, it can be used and reused in various programs and files.

Let use consider a typical utility type static class.

class Util {

  static {
    // run some common preparations & processes
    // run the primary method
    this.primary();
  }

  static primary() {}

  static secondary1() {}

  static secondary2() {}

  static secondary3() {}
}

Above OOP object can be included in various JS files (directly or as a module), however, they may not all need all its methods. It would be contrary to the concept of OOP to create multiple versions of that object, each with the methods that each file needs. Therefore, the same object is used for all, and each file would use what it needs.

If the JS files needs a methods, it can access it directly e.g. Util.secondary1()

What about JS files that ONLY need the preparatory processes and the the primary() method, and not any of the secondary methods?

In this case, the JS file will have a included (or imported) named class object but the class name is not called since its secondary method are not needed in this particular JS file. If imported, the class module would be referred to as a Side effect import.

PS. Sorry for the long post

fasttime commented 6 months ago

I'm still having trouble understanding why the rule is not working well for you. Maybe you could create a small repo to show your use case?

Let use consider a typical utility type static class.

class Util {

  static {
    // run some common preparations & processes
    // run the primary method
    this.primary();
  }

  static primary() {}

  static secondary1() {}

  static secondary2() {}

  static secondary3() {}
}

Above OOP object can be included in various JS files (directly or as a module), however, they may not all need all its methods. It would be contrary to the concept of OOP to create multiple versions of that object, each with the methods that each file needs. Therefore, the same object is used for all, and each file would use what it needs.

If by included you mean imported, then, at least in Node.js, it will be necessary to export that class first with exports.Util = Util; in CommonJS or export { Util }; in ESM. The export syntax marks the class as used, so it won't be reported by no-unused-vars.

If the JS files needs a methods, it can access it directly e.g. Util.secondary1()

What about JS files that ONLY need the preparatory processes and the the primary() method, and not any of the secondary methods?

It doesn't matter how that class is used in other files that import it. The rule only reports on the class declaration if it is not exported or otherwise used in the declaring file. Given your use case for calling Util.secondary1() in other places, you will need to export that class in the module where it is declared. Again, it is irrelevant whether that method or any other feature of the class will be actually used outside the declaring module.

In this case, the JS file will have a included (or imported) named class object but the class name is not called since its secondary method are not needed in this particular JS file. If imported, the class module would be referred to as a Side effect import.

Once again, it doesn't matter if the class is used in another module or not. If it's exported, then it won't be flagged by the rule. And if it's not exported because it's only used for side effects, then it doesn't need a name.

PS. Sorry for the long post

Never mind, it's always a good idea to add details.

erosman commented 6 months ago

I'm still having trouble understanding why the rule is not working well for you. Maybe you could create a small repo to show your use case?

Please note https://github.com/eslint/eslint/issues/17757#issuecomment-1810827895

If by included you mean imported, then, at least in Node.js, it will be necessary to export that class first with exports.Util = Util; in CommonJS or export { Util }; in ESM. The export syntax marks the class as used, so it won't be reported by no-unused-vars.

Included as an inline class OR imported as an ESM module.

Once again, it doesn't matter if the class is used in another module or not. If it's exported, then it won't be flagged by the rule. And if it's not exported because it's only used for side effects, then it doesn't need a name.

Please note Side effect import. A module can be imported without having its class imported. In this case, the auto-run class runs its course when imported but never needs to be referenced in the importer.

In the aforementioned repo (https://github.com/eslint/eslint/issues/17757#issuecomment-1810827895) you will find:

why not move the code out of the class?

It would be contrary to the concept of OOP and especially "encapsulation" in OOP (besides other reasons). class was introduced in ES6 for this reason.

See also:

In conclusion...

A static class with a static initialization block, that is not called again in the code, is a valid JavaScript OOP method, therefore is should not generate any error, stylistically or programmatically.

Example

The following are all valid and do the same job.

function run() {
  //do something
}
run();
class A {

  run() {
    //do something
  }
}
const a = new A();
a.run();
class A {

  static run() {
    //do something
  }
}
A.run();
class A {

  static {
    this.run();
  }

  static run() {
    //do something
  }
}

Proposal

A class with a "static initialization block" should not be marked as an error in no-unused-vars

fasttime commented 6 months ago

If by included you mean imported, then, at least in Node.js, it will be necessary to export that class first with exports.Util = Util; in CommonJS or export { Util }; in ESM. The export syntax marks the class as used, so it won't be reported by no-unused-vars.

Included as an inline class OR imported as an ESM module.

As explained previously, in the case of (ESM) module imports, the rule already behaves well, because it only reports on declarations that are not exported or otherwise used. As for the "included as an inline class", I was asking for an example because I'm not sure how that's supposed to work. If the inlining is done with some kind of tool, could you show the code? Or do you mean manually copy-pasting the Util class into another file?

Please note Side effect import. A module can be imported without having its class imported. In this case, the auto-run class runs its course when imported but never needs to be referenced in the importer.

In the aforementioned repo (#17757 (comment)) you will find:

* `options.js` contains 8 classes that are not referenced elsewhere in the code

* `commands.js` & `i18n.js` which are imported for side effect only

* `authentication.js` which here imports `app.js` for side effect only

* `background.js` which imports  `authentication.js` for side effect

* `options.js` and `popup.js` which import `i18n.js` for side effect

Since those classes are not exported, they don't need a declaration. Note that this use case is not the same as the Util class you mentioned in the previous comment, which does need a declaration. For example, the Incognito class in options.js in that repo could be a class expression, and the rule would not report it:

void class IncognitoAccess {

  static {
    // https://developer.mozilla.org/docs/Mozilla/Add-ons/WebExtensions/API/proxy/settings
    // Changing proxy settings requires private browsing window access because proxy settings affect private and non-private windows.
    // https://github.com/w3c/webextensions/issues/429
    // Inconsistency: incognito in proxy.settings
    // https://bugzilla.mozilla.org/show_bug.cgi?id=1725981
    // proxy.settings is not supported on Android
    App.firefox && browser.proxy.settings && browser.extension.isAllowedIncognitoAccess()
    .then(response => !response && alert(browser.i18n.getMessage('incognitoAccessError')));
  }
};

why not move the code out of the class?

It would be contrary to the concept of OOP and especially "encapsulation" in OOP (besides other reasons). class was introduced in ES6 for this reason.

I have no idea what this is talking about. If you think that the suggestion to "move the code out of the class" is coming from ESLint, then please, explain.

See also:

* [Best Practices for Object-Oriented Programming in JavaScript](https://codedamn.com/news/javascript/oop-best-practices)

* [Mastering Object-Oriented Programming in JavaScript: Best Practices and Examples](https://dev.to/hasidicdevs/mastering-object-oriented-programming-in-javascript-best-practices-and-examples-31mc)

In conclusion...

A static class with a static initialization block, that is not called again in the code, is a valid JavaScript OOP method, therefore is should not generate any error, stylistically or programmatically.

Example

The following are all valid and do the same job.

* The 3rd and 4th are almost identical in processing

* The 4th one is actually a new way of doing the 3rd since ECMAScript 2022 (Firefox 93, Chrome 94)

* Currently, `no-unused-vars` does not tolerate the 4th one
function run() {
  //do something
}
run();
class A {

  run() {
    //do something
  }
}
const a = new A();
a.run();
class A {

  static run() {
    //do something
  }
}
A.run();
class A {

  static {
    this.run();
  }

  static run() {
    //do something
  }
}

Maybe you are misunderstanding the purpose of the no-unused-vars rule. That rule is not about enforcing a particular style over another one. It won't stop you from creating classes with static initializers. The rule only reports on unused declarations because they create a variable that is not referenced in the containing scope, which could be hiding an error.

erosman commented 6 months ago

Let us simplify the situation by concentrating on the class (and not mixing import into the discussion).

Introduction

SO-SIB-Class (definition for convenience): A stand-alone class with a static initialization block that is not referenced elsewhere

no-unused-vars

fasttime commented 6 months ago

So this is a proposal to add an option to the rule no-unused-vars to ignore (stand-alone) class declarations with a static block.

In this case, could you please update the description of the issue with the correct information so it's clear to others?

The question "How do you think the change should be implemented?" should be answered with "A new option", not "A new default behavior". Also, it would be helpful to include the new option and its usage in the code example.

If it helps as a reference, here's another issue that also suggests to add a new option: https://github.com/eslint/eslint/issues/17082.

erosman commented 6 months ago

Closed in favour of #17772