Fix some false positives with ternary operators and cssMap (#1781)

* Fix some false positives with ternary operators and cssMap

* Add changeset

* Remove unnecessary spread operator

* Improve naming in union function

.changeset/few-camels-compare.md

@@ -0,0 +1,5 @@
+---
+'@compiled/eslint-plugin': patch
+---
+
+Fix some false positives in `shorthand-property-sorting` with css and cssMap

packages/eslint-plugin/src/rules/shorthand-property-sorting/__tests__/rule.test.ts

@@ -270,6 +270,88 @@ tester.run('shorthand-property-sorting', shorthandFirst, {
       `,
     },
 
+    //
+    // styles from ternary operators should not cause false positives, if the same properties
+    // are defined in each style object
+    //
+
+    {
+      name: `styles from ternary operators should not cause false positive, if the same properties are defined (css, ${imp})`,
+      code: `
+        import { css } from '${imp}';
+        const oldStyles = css({
+          font: '...',
+          fontWeight: '...',
+        });
+        const newStyles = css({
+          font: '...',
+          fontWeight: '...',
+        });
+        const someCondition = true;
+        export const EmphasisText = ({ children }) => <span css={[someCondition ? oldStyles : newStyles]}>{children}</span>;
+      `,
+    },
+    {
+      name: `pseudo-classes from ternary operators should not cause false positive, if the same properties are defined (css, ${imp})`,
+      code: `
+        import { css } from '${imp}';
+        const oldStyles = css({
+          '&:hover': {
+            font: '...',
+            fontWeight: '...',
+          }
+        });
+        const newStyles = css({
+          '&:hover': {
+            font: '...',
+            fontWeight: '...',
+          },
+        });
+        const someCondition = true;
+        export const EmphasisText = ({ children }) => <span css={[someCondition ? oldStyles : newStyles]}>{children}</span>;
+      `,
+    },
+    {
+      name: `styles from ternary operators should not cause false positive, if the same properties are defined (cssMap, ${imp})`,
+      code: `
+        import { css } from '${imp}';
+        const myMap = cssMap({
+          warning: {
+            font: '...',
+            fontWeight: '...',
+          },
+          normal: {
+            font: '...',
+            fontWeight: '...',
+          }
+        });
+        const someCondition = true;
+        export const EmphasisText = ({ appearance, children }) => <span css={myMap[apperance]}>{children}</span>;
+      `,
+    },
+    {
+      name: `pseudo-classes from ternary operators should not cause false positive, if the same properties are defined (cssMap, ${imp})`,
+      code: `
+        import { cssMap } from '${imp}';
+        const myMap = cssMap({
+          warning: {
+            '&:hover': {
+              font: '...',
+              fontWeight: '...',
+            },
+          },
+          normal: {
+            '&:hover': {
+              font: '...',
+              fontWeight: '...',
+            },
+          }
+        });
+        const someCondition = true;
+        export const EmphasisText = ({ appearance, children }) => <span css={myMap[apperance]}>{children}</span>;
+      `,
+    },
+
     //
     // has a valid sorting for borderInlineStart and borderInlineEnd
     //
@@ -310,6 +392,54 @@ tester.run('shorthand-property-sorting', shorthandFirst, {
         export const EmphasisText = ({ children }) => <span css={styles}>{children}</span>;
       `,
     },
+
+    // false negative: styles in pseudo-classes are not checked when using style composition
+
+    {
+      // Currently not handled due to the added complexity of handling selectors.
+      // Feel free to update the rule to handle this, if it becomes a problem
+      name: `false negative: styles in pseudo-classes are not checked when using style composition (css, ${imp})`,
+      code: `
+        import { css } from '${imp}';
+        const baseStyles = css({
+          '&:hover': {
+            paddingLeft: '...',
+          }
+        });
+        const newStyles = css({
+          '&:hover': {
+            padding: '...',
+          }
+        });
+        export const EmphasisText = ({ children }) => <span css={[baseStyles, newStyles]}>{children}</span>;
+      `,
+    },
+
+    //
+    // depth in correct order for shorthand properties in the same bucket for cssMap AND if key not static
+    //
+
+    {
+      name: `depth in correct order for shorthand properties in the same bucket for cssMap AND if key not static (${imp})`,
+      code: outdent`
+        import { cssMap } from '${imp}';
+        const styles = cssMap({
+          warning: {
+            borderColor: '...', // 2
+            borderTop: '...', // 1
+          },
+          debug: {
+            borderColor: '...', // 2
+            borderTop: '...', // 1
+          },
+          normal: {
+            borderColor: '...', // 2
+            borderTop: '...', // 1
+          },
+        });
+        export const EmphasisText = ({ children, appearance }) => <span css={styles[appearance]}>{children}</span>;
+      `,
+    },
   ]),
 
   invalid: includedImports.flatMap((imp) => [
@@ -796,39 +926,42 @@ tester.run('shorthand-property-sorting', shorthandFirst, {
     },
 
     //
-    // false positives for cssMap
+    // known false positives for css
     //
 
     {
-      name: `false positive: shorthands in different selectors for cssMap if key not static (${imp})`,
-      code: outdent`
-        import { cssMap } from '${imp}';
-        const styles = cssMap({
-          warning: {
-            borderTop: '...',
-          },
-          normal: {
-            border: '...',
-          }
+      // I don't see a good way for the ESLint rule to handle this, without running the
+      // rule multiple times to handle each branch of the ternary operator (which can be
+      // exponentially expensive the more ternary operators we have)
+      name: 'false positive: styles from ternary operators, if different properties are defined',
+      code: `
+        import { css } from '${imp}';
+        const oldStyles = css({
+          fontWeight: '...',
         });
-        export const EmphasisText = ({ children, appearance }) => <span css={styles[appearance]}>{children}</span>;
+        const newStyles = css({
+          font: '...',
+        });
+        const someCondition = true;
+        export const EmphasisText = ({ children }) => <span css={[someCondition ? oldStyles : newStyles]}>{children}</span>;
       `,
       errors: [{ messageId: 'shorthand-first' }, { messageId: 'shorthand-first' }],
     },
+
+    //
+    // known false positives for cssMap
+    //
+
     {
-      // this is a false positive, but making an exception for this would involve
-      // some extra logic... maybe we can revisit this if it becomes a common situation.
-      name: `false positive, if depth in correct order for shorthand properties in the same bucket for cssMap AND if key not static (${imp})`,
+      name: `false positive: shorthands in different selectors for cssMap if key not static (${imp})`,
       code: outdent`
         import { cssMap } from '${imp}';
         const styles = cssMap({
           warning: {
-            borderColor: '...', // 2
-            borderTop: '...', // 1
+            borderTop: '...',
           },
           normal: {
-            borderColor: '...', // 2
-            borderTop: '...', // 1
+            border: '...',
           }
         });
         export const EmphasisText = ({ children, appearance }) => <span css={styles[appearance]}>{children}</span>;

packages/eslint-plugin/src/rules/shorthand-property-sorting/index.ts

@@ -116,6 +116,45 @@ const getObjectCSSProperties = (
   return properties;
 };
 
+// Given two (or more) arrays of properties, concatenate them in such a way that any
+// repeated properties are de-duplicated.
+//
+// Nested arrays (nested selectors, pseudo-selectors, etc.) are not de-duplicated.
+const union = (...arrays: PropertyArray[]): PropertyArray => {
+  const newArray = [];
+
+  if (arrays.length === 0) {
+    return [];
+  }
+
+  const arrayA = arrays[0];
+  const propertiesInArrayA = new Set<string>();
+
+  for (const elementA of arrayA) {
+    newArray.push(elementA);
+    if (!Array.isArray(elementA)) {
+      propertiesInArrayA.add(elementA.name);
+    }
+  }
+
+  for (const arrayB of arrays.slice(1)) {
+    for (const elementB of arrayB) {
+      if (Array.isArray(elementB)) {
+        newArray.push(elementB);
+        continue;
+      }
+
+      if (propertiesInArrayA.has(elementB.name)) {
+        continue;
+      }
+
+      newArray.push(elementB);
+    }
+  }
+
+  return newArray;
+};
+
 const parseCssArrayElement = (
   context: Rule.RuleContext,
   element: ArrayExpression['elements'][number]
@@ -135,10 +174,10 @@ const parseCssArrayElement = (
   } else if (element.type === 'ConditionalExpression') {
     // Covers the case:
     //     someCondition ? someStyles : someOtherStyles
-    return [
-      ...parseCssArrayElement(context, element.consequent),
-      ...parseCssArrayElement(context, element.alternate),
-    ];
+    return union(
+      parseCssArrayElement(context, element.consequent),
+      parseCssArrayElement(context, element.alternate)
+    );
   } else if (element.type === 'MemberExpression' && element.object.type === 'Identifier') {
     // Covers cssMap usages
     functionCall = getVariableDefinition(context, element.object);
@@ -275,7 +314,7 @@ const parseCssMap = (
   context: Rule.RuleContext,
   { node, key }: { node: CallExpression; key?: string | Literal['value'] }
 ): PropertyArray => {
-  const properties: PropertyArray = [];
+  const properties: PropertyArray[] = [];
   const { references } = context.sourceCode.getScope(node);
   if (!isCssMap(node.callee as Rule.Node, references)) {
     return [];
@@ -303,26 +342,24 @@ const parseCssMap = (
       // If we know what key in the cssMap function call to traverse,
       // we can make sure we only traverse that.
       if (property.key.type === 'Literal' && key === property.key.value) {
-        properties.push(...getObjectCSSProperties(context, property.value));
-        break;
+        return getObjectCSSProperties(context, property.value);
       } else if (property.key.type === 'Identifier' && key === property.key.name) {
-        properties.push(...getObjectCSSProperties(context, property.value));
-        break;
+        return getObjectCSSProperties(context, property.value);
       }
-    } else {
-      // We cannot determine which key in the cssMap function call to traverse,
-      // so we have no choice but to unconditionally traverse through the whole
-      // cssMap object.
-      //
-      // Not very performant and can give false positives, but considering that
-      // the cssMap key can be dynamic, we at least avoid any false negatives.
-      //
-      // (https://compiledcssinjs.com/docs/api-cssmap#dynamic-declarations)
-      properties.push(...getObjectCSSProperties(context, property.value));
     }
+
+    // We cannot determine which key in the cssMap function call to traverse,
+    // so we have no choice but to unconditionally traverse through the whole
+    // cssMap object.
+    //
+    // Not very performant and can give false positives, but considering that
+    // the cssMap key can be dynamic, we at least avoid any false negatives.
+    //
+    // (https://compiledcssinjs.com/docs/api-cssmap#dynamic-declarations)
+    properties.push(getObjectCSSProperties(context, property.value));
   }
 
-  return properties;
+  return union(...properties);
 };
 
 const parseStyled = (context: Rule.RuleContext, node: CallExpression): PropertyArray => {