Internal ESLint Rule to prevent unnecessary group

[seiyab]

Isn't it helpful if #15795 and similar problems can be detected by defining internal ESLint rule?

group containing hardline or breakParent only make sense if the rest of the code relies on the Group type to make a different decision.

I might try to implement it if members agree.

[fisker]

Go!

[fisker]

Reminder, this may only able to detect simple array?

group([hardline, ...])

The following cases are all not safe.

group(foo ? [...], [hardline, ...])

group(join(hardline, ...)); // the second argument can be single

[fisker]

Do you have other cases we can check?

[seiyab]

Thank you for your reminder.
I considered simple array + some simple ones. I missed simple rule won't detect #15795 itself as you mentioned "the second argument can be single".

I have considered:

• group([hardline, ...])
• group([foo ? [..., hardline, ...] : [..., hardline, ...], ...])
• group(join(hardline, [foo, ...rest]))
• group([ifBreak([..., hardline, ...], [..., hardline, ...], { groupId })

However, it can't detect #15795 and might not be helpful enough... I might submit PR only if I find even simple rule can detect existing mistakes or come up with better logic.

[seiyab]

Oh I've missed #15795 contains not only join(hardline, docs) but also simple hardline. A simple logic might help. I'll try it anyway. I don't hesitate to be rejected at all.

[seiyab]

I'm wondering why following one can change format...

diff --git a/src/language-js/print/jsx.js b/src/language-js/print/jsx.js
index 424756498..e78915c65 100644
--- a/src/language-js/print/jsx.js
+++ b/src/language-js/print/jsx.js
@@ -258,12 +258,12 @@ function printJsxElementInternal(path, options, print) {
     return content;
   }

-  const multiLineElem = group([
+  const multiLineElem = [
     openingLines,
     indent([hardline, content]),
     hardline,
     closingLines,
-  ]);
+  ];

   if (forcedBreak) {
     return multiLineElem;

↓test result

  x = (
    <div>
      <div className="first" tabIndex="1">
        First
-     </div>
-     -
-     <div className="second" tabIndex="2">
+     </div>-<div className="second" tabIndex="2">
        Second
      </div>
    </div>
  );

  x = (
    <div>
      <div className="first" tabIndex="1">
        First
-     </div>
-     -
-     <div className="second" tabIndex="2">
+     </div>-<div className="second" tabIndex="2">
        Second
      </div>
    </div>
  );

  x = (
    <div>
      <div className="first" tabIndex="1">
        First
-     </div>
-     -
-     <div className="second" tabIndex="2">
+     </div>-<div className="second" tabIndex="2">
        Second
      </div>
    </div>
  );

  <Hello>
    test <World /> test
- </Hello>
- 123
+ </Hello>123

[seiyab]

EDIT: simplified.

describe("fill", () => {
  test("group", () => {
    const doc = fill([
      group(["{", breakParent]),
      softline,
      "-",
      softline,
      group(["}", breakParent]),
    ]);

    expect(docToString(doc, { printWidth: 100 })).toMatchInlineSnapshot(`
      "{
      -
      }"
    `);
  });

  test("array", () => {
    const doc = fill([
      ["{", breakParent],
      softline,
      "-",
      softline,
      ["}", breakParent],
    ]);

    expect(docToString(doc, { printWidth: 100 })).toMatchInlineSnapshot(
      `"{-}"`,
    );
  });
});

older example

describe("fill", () => {
  test("group", () => {
    const doc = fill([
      group(["[", indent([hardline, "aaaaa"]), hardline, group("]")]),
      softline,
      "-",
      softline,
      group(["[", indent([hardline, "bbbbb"]), hardline, group("]")]),
    ]);
    expect(docToString(doc, { printWidth: 100 })).toMatchInlineSnapshot(`
      "[
      aaaaa
      ]
      -
      [
      bbbbb
      ]"
    `);
  });

  test("array", () => {
    const doc = fill([
      ["[", indent([hardline, "aaaaa"]), hardline, group("]")],
      softline,
      "-",
      softline,
      ["[", indent([hardline, "bbbbb"]), hardline, group("]")],
    ]);
    expect(docToString(doc, { printWidth: 100 })).toMatchInlineSnapshot(`
      "[
      aaaaa
      ]-[
      bbbbb
      ]"
    `);
  });
});

[fisker]

Maybe @thorn0 can help?

[seiyab]

In block for DOC_TYPE_FILL,

const doc = fill([
  group(["{", breakParent]),
  softline,
  "-",
  softline,
  group(["}", breakParent]),
]);

goes

length = 3, firstAndSecondContentFits = false, contentFits = false
length = 1, firstAndSecondContentFits = false, contentFits = true
length = 1, contentFits = false

On the other hand,

const doc = fill([
  ["{", breakParent],
  softline,
  "-",
  softline,
  ["}", breakParent],
]);

yields

length = 3, firstAndSecondContentFits = true
length = 1, firstAndSecondContentFits = true
length = 1, contentFits = true

.

So fits() yields different result between breaking group() and breaking [] maybe?

[seiyab]

Is it?

diff --git a/src/document/printer.js b/src/document/printer.js
index a12c3a1ec..0fd704f75 100644
--- a/src/document/printer.js
+++ b/src/document/printer.js
@@ -279,6 +279,9 @@ function fits(
         if (mode === MODE_BREAK || doc.hard) {
           return true;
         }
+        if (mustBeFlat && doc.hard) {
+          return false;
+        }
         if (!doc.soft) {
           out.push(" ");
           width--;
@@ -294,6 +297,11 @@ function fits(
           return false;
         }
         break;
+
+      case DOC_TYPE_BREAK_PARENT:
+        if (mustBeFlat) {
+          return false;
+        }
     }
   }
   return false;

This change make just one test case failed.

 FAIL  tests/format/markdown/spec/jsfmt.spec.js
example-601.md - {"proseWrap":"always"} > format
    Error: expect(received).toMatchSnapshot()

    Snapshot name: `example-601.md - {"proseWrap":"always"} format 1`

    - Snapshot  - 1
    + Received  + 2

    @@ -7,8 +7,9 @@
      foo\
           bar

      =====================================output=====================================
      foo\
    -  bar
    +
    + bar

      ================================================================================
        at runTest (file:///Users/seiya/Projects/oss/prettier/tests/config/format-test.js:372:5)
        at file:///Users/seiya/Projects/oss/prettier/tests/config/format-test.js:305:17
        at async Promise.all (index 63)
        at async TestScheduler.scheduleTests (/Users/seiya/Projects/oss/prettier/node_modules/@jest/core/build/index.js:1128:15)
        at async runJest (/Users/seiya/Projects/oss/prettier/node_modules/@jest/core/build/index.js:3390:19)
        at async _run10000 (/Users/seiya/Projects/oss/prettier/node_modules/@jest/core/build/index.js:1513:164)

[seiyab]

I'll submit a PR for this potential "bug" first.