ESLINTJS-18 Use ESLint built-in rule no-unreachable-loop instead of our own no-one-iteration-loop (#5576)

Author: vdiez

its/ruling/src/test/expected/jsts/angular.js/javascript-S1751.json

@@ -0,0 +1,6 @@
+{
+"angular.js:src/jqLite.js": [
+199,
+317
+]
+}

its/ruling/src/test/expected/jsts/es5-shim/javascript-S1751.json

@@ -0,0 +1,5 @@
+{
+"es5-shim:es5-shim.js": [
+80
+]
+}

its/ruling/src/test/expected/jsts/jquery/javascript-S1751.json

@@ -0,0 +1,5 @@
+{
+"jquery:src/core.js": [
+228
+]
+}

packages/jsts/src/rules/S1751/index.ts

@@ -14,4 +14,5 @@
  * You should have received a copy of the Sonar Source-Available License
  * along with this program; if not, see https://sonarsource.com/license/ssal/
  */
-export { rule } from './rule.js';
+import { getESLintCoreRule } from '../external/core.js';
+export const rule = getESLintCoreRule('no-unreachable-loop');

packages/jsts/src/rules/S1751/meta.ts

@@ -14,5 +14,6 @@
  * You should have received a copy of the Sonar Source-Available License
  * along with this program; if not, see https://sonarsource.com/license/ssal/
  */
-export const implementation = 'original';
-export const eslintId = 'no-one-iteration-loop';
+export const implementation = 'external';
+export const eslintId = 'no-unreachable-loop';
+export const externalPlugin = 'eslint';

packages/jsts/src/rules/S1751/rule.ts

@@ -14,15 +14,15 @@
  * You should have received a copy of the Sonar Source-Available License
  * along with this program; if not, see https://sonarsource.com/license/ssal/
  */
-import { rule } from './rule.js';
+import { rule } from './index.js';
 import { DefaultParserRuleTester } from '../../../tests/tools/testers/rule-tester.js';
 import { describe, it } from 'node:test';
 
 describe('S1751', () => {
   it('S1751', () => {
     const ruleTester = new DefaultParserRuleTester();
 
-    ruleTester.run('no-one-iteration-loop', rule, {
+    ruleTester.run('no-unreachable-loop', rule, {
       valid: [
         valid(`
     while (cond) {
@@ -86,19 +86,6 @@ describe('S1751', () => {
       continue;
     }`),
 
-        valid(`
-    function foo() {
-      for(p of arr) {
-        return p;  // Compliant: used to return the first element of an array
-      }
-    }`),
-
-        valid(`
-    for (p in obj) {
-      bar();
-      break; // Compliant: often used to check whether an object is "empty"
-    }`),
-
         valid(`
     while(foo()) {
       if (bar()) {
@@ -285,7 +272,7 @@ describe('S1751', () => {
         code,
         errors: [
           {
-            messageId: 'refactorLoop',
+            messageId: 'invalid',
           },
         ],
       };

packages/jsts/src/rules/S1751/unit.test.ts

@@ -1,37 +1,60 @@
 <h2>Why is this an issue?</h2>
 <p>A selector parameter is a <code>boolean</code> parameter that’s used to determine which of two paths to take through a method. Specifying such a
-parameter may seem innocuous, particularly if it’s well named.</p>
+parameter may seem innocuous, particularly if it’s well-named.</p>
 <p>Unfortunately, developers calling the method won’t see the parameter name, only its value. They’ll be forced either to guess at the meaning or to
-take extra time to look the method up.</p>
-<p>This rule finds methods with a <code>boolean</code> that’s used to determine which path to take through the method.</p>
+take extra time to look the method up. Additionally, selector parameters create maintenance challenges: adding a third option requires changing the
+boolean to an enum or similar type, breaking existing code.</p>
+<p>This rule finds methods with a <code>boolean</code> that is used to determine which path to take through the method.</p>
 <h3>Noncompliant code example</h3>
 <pre>
-function tempt(name: string, ofAge: boolean) {
-  if (ofAge) {
-    offerLiquor(name);
+function feed(name: string, isHuman: boolean) {
+  if (isHuman) {
+    // implementation for human
   } else {
-    offerCandy(name);
+    // implementation for animal
   }
 }
 
-// ...
-function corrupt() {
-  tempt("Joe", false); // does this mean not to temp Joe?
-}
+// Intent is not clear at call site
+feed('Max', false); // does this mean not to feed Max?
 </pre>
 <h3>Compliant solution</h3>
-<p>Instead, separate methods should be written.</p>
+<p>Instead, split the method into separate, clearly named methods:</p>
 <pre>
-function temptAdult(name: string) {
-  offerLiquor(name);
+function feedHuman(name: string) {
+  offerSushi(name);
 }
 
-function temptChild(name: string) {
-  offerCandy(name);
+function feedAnimal(name: string) {
+  offerCarrot(name);
 }
 
-// ...
-function corrupt() {
-  age &lt; legalAge ? temptChild("Joe") : temptAdult("Joe");
+// Clear intent at call site
+feedHuman("Joe");
+feedAnimal("Max");
+</pre>
+<p>Alternatively, use descriptive object parameters or types:</p>
+<pre>
+type EntityType = 'human' | 'animal';
+
+function feed(name: string, entityType: EntityType) {
+  if (entityType === 'human') {
+    // implementation for human
+  } else {
+    // implementation for animal
+  }
 }
+
+feed('Max', 'animal');
+</pre>
+<pre>
+function feed(name: string, { human = true } ) {
+  if (human) {
+    // implementation for human
+  } else {
+    // implementation for animal
+  }
+}
+
+feed('Max', { human: false });
 </pre>