Do not use merger if arguments are the same. (#1092)

* Do not use merger if arguments are the same.
Also this adds a new feature: creating a merger is not needed if there are no configuration options for the rule.
Removed unneeded mergers (for rules with no configuration options).

* Fixes.

* Fix docs.

* Small docs touchups to match

Co-authored-by: Josh Goldberg <josh@codecademy.com>

Author: Res42

docs/Architecture/Linters.md

@@ -23,6 +23,8 @@ Those are run by `src/converters/lintConfigs/rules/convertRules.ts`, which takes
     * The output rule name is added to the TSLint rule's equivalency set.
     * The TSLint rule's config severity is mapped to its ESLint equivalent.
     * If this is the first time the output ESLint rule is seen, it's directly marked as converted.
+    * Notices are merged and deduplicated.
+    * If the existing output has the same arguments as the new output, merge lookups are skipped.
     * If not, a rule merger is run to combine it with its existing output settings.
 
 ### Rule Converters
@@ -52,6 +54,8 @@ These are located in `src/rules/mergers/`, and keyed under their names by the ma
 
 For example, `@typescript-eslint/ban-types` spreads both arguments' `types` members into one large `types` object.
 
+> A merger does not need to be created if the rule does not accept any configuration or all converters output exactly the same configuration.
+
 ## Package Summaries
 
 ESLint configurations are summarized based on extended ESLint and TSLint presets.

src/converters/lintConfigs/rules/convertRules.test.ts

@@ -107,41 +107,98 @@ describe("convertRules", () => {
         expect(ruleEquivalents).toEqual(new Map([["tslint-rule-a", ["eslint-rule-a"]]]));
     });
 
-    it("reports a failure when two outputs exist for a converted rule without a merger", () => {
-        // Arrange
-        const conversionResult = {
-            rules: [
-                {
-                    ruleName: "eslint-rule-a",
-                },
-                {
-                    ruleName: "eslint-rule-a",
-                },
-            ],
-        };
-        const { tslintRule, converters, mergers } = setupConversionEnvironment({
-            conversionResult,
-        });
-
-        // Act
-        const { failed } = convertRules(
-            { ruleConverters: converters, ruleMergers: mergers },
-            { [tslintRule.ruleName]: tslintRule },
-            new Map<string, string[]>(),
-        );
-
-        // Assert
-        expect(failed).toEqual([ConversionError.forMerger("eslint-rule-a")]);
-    });
-
-    it("merges rule arguments when two outputs exist for a converted rule with a merger", () => {
+    test.each([[undefined], [[5, "allow-something", { max: 50 }]]])(
+        "runs without an argument merger when both rules have the same arguments",
+        (ruleArguments) => {
+            // Arrange
+            const conversionResult = {
+                rules: [
+                    {
+                        ruleArguments,
+                        ruleName: "eslint-rule-a",
+                    },
+                    {
+                        ruleArguments,
+                        ruleName: "eslint-rule-a",
+                    },
+                ],
+            };
+            const { tslintRule, converters, mergers } = setupConversionEnvironment({
+                conversionResult,
+            });
+
+            // Act
+            const { converted, failed } = convertRules(
+                { ruleConverters: converters, ruleMergers: mergers },
+                { [tslintRule.ruleName]: tslintRule },
+                new Map<string, string[]>(),
+            );
+
+            // Assert
+            expect(converted).toEqual(
+                new Map([
+                    [
+                        "eslint-rule-a",
+                        {
+                            ruleArguments,
+                            ruleName: "eslint-rule-a",
+                            ruleSeverity: "error",
+                            notices: [],
+                        },
+                    ],
+                ]),
+            );
+            expect(failed).toEqual([]);
+        },
+    );
+
+    test.each([
+        [[[0]], [[1]]],
+        [[[""]], [["allow-something"]]],
+        [[[{ max: 0 }]], [[{ max: 50 }]]],
+        [[[0, "", { max: 0 }]], [[5, "allow-something", { max: 50 }]]],
+    ])(
+        "reports a failure when two outputs with different arguments exist for a converted rule without a merger",
+        ([existingArguments, newArguments]) => {
+            // Arrange
+            const conversionResult = {
+                rules: [
+                    {
+                        ruleArguments: existingArguments,
+                        ruleName: "eslint-rule-a",
+                    },
+                    {
+                        ruleArguments: newArguments,
+                        ruleName: "eslint-rule-a",
+                    },
+                ],
+            };
+            const { tslintRule, converters, mergers } = setupConversionEnvironment({
+                conversionResult,
+            });
+
+            // Act
+            const { failed } = convertRules(
+                { ruleConverters: converters, ruleMergers: mergers },
+                { [tslintRule.ruleName]: tslintRule },
+                new Map<string, string[]>(),
+            );
+
+            // Assert
+            expect(failed).toEqual([ConversionError.forMerger("eslint-rule-a")]);
+        },
+    );
+
+    it("merges rule arguments when two outputs with different arguments exist for a converted rule with a merger", () => {
         // Arrange
         const conversionResult = {
             rules: [
                 {
+                    ruleArguments: [0],
                     ruleName: "eslint-rule-a",
                 },
                 {
+                    ruleArguments: [1],
                     ruleName: "eslint-rule-a",
                 },
             ],
@@ -201,7 +258,7 @@ describe("convertRules", () => {
         );
 
         // Assert
-        expect(converted).toEqual(
+        expect([
             new Map([
                 [
                     "eslint-rule-a",
@@ -213,7 +270,17 @@ describe("convertRules", () => {
                     },
                 ],
             ]),
-        );
+            new Map([
+                [
+                    "eslint-rule-a",
+                    {
+                        ruleName: "eslint-rule-a",
+                        ruleSeverity: "error",
+                        notices: ["notice-1", "notice-2"],
+                    },
+                ],
+            ]),
+        ]).toContainEqual(converted);
     });
 
     it("merges undefined notices", () => {
@@ -243,7 +310,7 @@ describe("convertRules", () => {
         );
 
         // Assert
-        expect(converted).toEqual(
+        expect([
             new Map([
                 [
                     "eslint-rule-a",
@@ -255,7 +322,17 @@ describe("convertRules", () => {
                     },
                 ],
             ]),
-        );
+            new Map([
+                [
+                    "eslint-rule-a",
+                    {
+                        ruleName: "eslint-rule-a",
+                        ruleSeverity: "error",
+                        notices: [],
+                    },
+                ],
+            ]),
+        ]).toContainEqual(converted);
     });
 
     it("marks a new plugin when a conversion has a new plugin", () => {

src/converters/lintConfigs/rules/convertRules.ts

@@ -1,3 +1,5 @@
+import { isEqual } from "lodash";
+
 import { ConversionError } from "../../../errors/conversionError";
 import { ErrorSummary } from "../../../errors/errorSummary";
 import { TSLintConfigurationRules } from "../../../input/findTSLintConfiguration";
@@ -7,7 +9,7 @@ import { formatRawTslintRule } from "./formats/formatRawTslintRule";
 import { RuleMerger } from "./ruleMerger";
 import { RuleConverter } from "./ruleConverter";
 import { TSLintRuleOptions, ESLintRuleOptions } from "./types";
-import { Entries } from "../../../utils";
+import { Entries, uniqueFromSources } from "../../../utils";
 
 export type ConvertRulesDependencies = {
     ruleConverters: Map<string, RuleConverter>;
@@ -78,21 +80,29 @@ export const convertRules = (
                     continue;
                 }
 
-                // 4d. If not, a rule merger is run to combine it with its existing output settings.
+                // 4d. Notices are merged and deduplicated.
+                existingConversion.notices = uniqueFromSources(
+                    existingConversion.notices,
+                    newConversion.notices,
+                );
+                converted.set(changes.ruleName, existingConversion);
+
+                // 4e. If the existing output has the same arguments as the new output, merge lookups are skipped.
+                if (isEqual(existingConversion.ruleArguments, newConversion.ruleArguments)) {
+                    continue;
+                }
+
+                // 4f. If not, a rule merger is run to combine it with its existing output settings.
                 const merger = dependencies.ruleMergers.get(changes.ruleName);
                 if (merger === undefined) {
                     failed.push(ConversionError.forMerger(changes.ruleName));
                 } else {
-                    const existingNotices = existingConversion.notices ?? [];
-                    const newNotices = newConversion.notices ?? [];
-
                     converted.set(changes.ruleName, {
                         ...existingConversion,
                         ruleArguments: merger(
                             existingConversion.ruleArguments,
                             newConversion.ruleArguments,
                         ),
-                        notices: Array.from(new Set([...existingNotices, ...newNotices])),
                     });
                 }
             }

src/converters/lintConfigs/rules/ruleMergers.ts

@@ -2,7 +2,6 @@ import { mergeBanTypes } from "./ruleMergers/ban-types";
 import { mergeConsistentTypeAssertions } from "./ruleMergers/consistent-type-assertions";
 import { mergeIndent } from "./ruleMergers/indent";
 import { mergeNamingConvention } from "./ruleMergers/naming-convention";
-import { mergeNoCaller } from "./ruleMergers/no-caller";
 import { mergeNoEval } from "./ruleMergers/no-eval";
 import { mergeNoMemberDelimiterStyle } from "./ruleMergers/member-delimiter-style";
 import { mergeNoUnnecessaryTypeAssertion } from "./ruleMergers/no-unnecessary-type-assertion";
@@ -16,6 +15,5 @@ export const ruleMergers = new Map([
     ["@typescript-eslint/naming-convention", mergeNamingConvention],
     ["@typescript-eslint/no-unnecessary-type-assertion", mergeNoUnnecessaryTypeAssertion],
     ["@typescript-eslint/triple-slash-reference", mergeTripleSlashReference],
-    ["no-caller", mergeNoCaller],
     ["no-eval", mergeNoEval],
 ]);