[ESLint] Improve handling of resolving local constants (facebook#6)

* Handle 'as' and 'satisfies' expressions for style values
xonx4l · Nov 22, 2023 · 92b8767 · 92b8767
1 parent 289cb7c
commit 92b8767
Show file tree

Hide file tree

Showing 10 changed files with 120 additions and 36 deletions.
diff --git a/.eslintrc b/.eslintrc
@@ -30,8 +30,7 @@
     "coverage",
     "dist",
     "lib",
-    "node_modules",
-    "nextjs-example"
+    "node_modules"
   ],
   "globals": {
     "$Call": "readonly",

diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml
@@ -36,6 +36,7 @@ jobs:
         with:
           node-version: '18.x'
       - run: npm install
+      - run: npm run build
       - run: npm run lint:report
 
   unit-test:

diff --git a/apps/nextjs-example/.eslintrc.js b/apps/nextjs-example/.eslintrc.js
@@ -5,5 +5,8 @@ module.exports = {
     // The Eslint rule still needs work, but you can
     // enable it to test things out.
     '@stylexjs/valid-styles': 'error',
+    'ft-flow/space-after-type-colon': 0,
+    'ft-flow/no-types-missing-file-annotation': 0,
+    'ft-flow/generic-spacing': 0,
   },
 };
diff --git a/apps/nextjs-example/app/Card.tsx b/apps/nextjs-example/app/Card.tsx
@@ -33,9 +33,13 @@ export default function Card({ title, body, href }: Props) {
   );
 }
 
-const MOBILE = '@media (max-width: 700px)' as const;
+type TMobile = '@media (max-width: 700px)';
+
+const MOBILE: TMobile = '@media (max-width: 700px)' as TMobile;
 const REDUCE_MOTION = '@media (prefers-reduced-motion: reduce)' as const;
 
+const bgDefault = `rgba(${$.cardR}, ${$.cardG}, ${$.cardB}, 0)` as const;
+
 const styles = stylex.create({
   link: {
     display: {
@@ -47,7 +51,7 @@ const styles = stylex.create({
     flexDirection: 'column',
     borderRadius: spacing.xs,
     backgroundColor: {
-      default: `rgba(${$.cardR}, ${$.cardG}, ${$.cardB}, 0)`,
+      default: bgDefault,
       ':hover': `rgba(${$.cardR}, ${$.cardG}, ${$.cardB}, 0.1)`,
     },
     borderWidth: 1,
@@ -93,4 +97,6 @@ const styles = stylex.create({
     lineHeight: 1.5,
     maxWidth: '30ch',
   },
+  color: (color: string) => ({ color }),
+  width: (width: string) => ({ width }),
 });
diff --git a/apps/nextjs-example/typetests/typetests.ts b/apps/nextjs-example/typetests/typetests.ts
@@ -266,6 +266,8 @@ const styles8: Readonly<{
   }>;
 }> = stylex.create({
   foo: {
+    // In a real example `vars` would be imported from another file.
+    // eslint-disable-next-line @stylexjs/valid-styles
     color: vars.accent,
   },
 });

diff --git a/packages/eslint-plugin/src/rules/isAnimationName.js b/packages/eslint-plugin/src/rules/isAnimationName.js
@@ -27,6 +27,9 @@ export default function isAnimationName(
   }
   if (node.type === 'Identifier' && variables && variables.has(node.name)) {
     const variable = variables.get(node.name);
+    if (variable === 'ARG') {
+      return undefined;
+    }
     if (variable != null) {
       return isAnimationName(variable, variables);
     } else {

diff --git a/packages/eslint-plugin/src/rules/makeUnionRule.js b/packages/eslint-plugin/src/rules/makeUnionRule.js
@@ -24,15 +24,6 @@ export default function makeUnionRule(
     variables?: Variables,
     prop?: Property,
   ): RuleResponse => {
-    let isBorder = false;
-    if (
-      prop != null &&
-      prop.key.type === 'Identifier' &&
-      prop.key.name === 'border'
-    ) {
-      console.log('UNION OF BORDER', prop);
-      isBorder = true;
-    }
     const failedRules = [];
     for (const _rule of rules) {
       const rule =
@@ -50,10 +41,8 @@ export default function makeUnionRule(
       failedRules.push(check);
     }
     const fixable = failedRules.filter((a) => a.suggest != null);
-    if (isBorder) {
-      console.log('UNION OF BORDER fixable Rules:', fixable, node);
-    }
     fixable.sort((a, b) => (a.distance || Infinity) - (b.distance || Infinity));
+
     return {
       message: failedRules.map((a) => a.message).join('\n'),
       suggest: fixable[0] != null ? fixable[0].suggest : undefined,

diff --git a/packages/eslint-plugin/src/stylex-valid-styles.js b/packages/eslint-plugin/src/stylex-valid-styles.js
@@ -42,11 +42,12 @@ import isPercentage from './rules/isPercentage';
 import isAnimationName from './rules/isAnimationName';
 import isStylexDefineVarsToken from './rules/isStylexDefineVarsToken';
 import { borderSplitter } from './utils/split-css-value';
+import evaluate from './utils/evaluate';
 
-export type Variables = $ReadOnlyMap<string, Expression>;
+export type Variables = $ReadOnlyMap<string, Expression | 'ARG'>;
 export type RuleCheck = (
   node: $ReadOnly<Expression | Pattern>,
-  variables?: $ReadOnly<Variables>,
+  variables?: Variables,
   prop?: $ReadOnly<Property>,
 ) => RuleResponse;
 export type RuleResponse = void | {
@@ -2232,7 +2233,7 @@ const stylexValidStyles = {
     ],
   },
   create(context: Rule.RuleContext): { ... } {
-    const variables = new Map<string, Expression>();
+    const variables = new Map<string, Expression | 'ARG'>();
     const dynamicStyleVariables = new Set<string>();
 
     const legacyReason =
@@ -2424,20 +2425,35 @@ const stylexValidStyles = {
           );
         }
         let styleKey: Expression | PrivateIdentifier = style.key;
-        if (style.computed && style.key.type !== 'Literal') {
-          if (style.key.type === 'Identifier') {
-            const varValue = variables.get(style.key.name);
-            if (varValue != null) {
-              styleKey = varValue;
-            } else {
-              return context.report(
-                ({
-                  node: style.key,
-                  loc: style.key.loc,
-                  message: 'Computed key cannot be resolved.',
-                }: Rule.ReportDescriptor),
-              );
-            }
+        if (styleKey.type === 'PrivateIdentifier') {
+          return context.report(
+            ({
+              node: styleKey,
+              loc: styleKey.loc,
+              message: 'Private properties are not allowed in stylex',
+            }: Rule.ReportDescriptor),
+          );
+        }
+        if (style.computed && styleKey.type !== 'Literal') {
+          const val = evaluate(styleKey, variables);
+          if (val == null) {
+            return context.report(
+              ({
+                node: style.key,
+                loc: style.key.loc,
+                message: 'Computed key cannot be resolved.',
+              }: Rule.ReportDescriptor),
+            );
+          } else if (val === 'ARG') {
+            return context.report(
+              ({
+                node: style.key,
+                loc: style.key.loc,
+                message: 'Computed key cannot depend on function argument',
+              }: Rule.ReportDescriptor),
+            );
+          } else {
+            styleKey = val;
           }
         }
         if (styleKey.type !== 'Literal' && styleKey.type !== 'Identifier') {
@@ -2531,7 +2547,18 @@ const stylexValidStyles = {
           stylexDefineVarsTokenImports.size > 0 &&
           isStylexDefineVarsToken(style.value, stylexDefineVarsTokenImports);
         if (!isReferencingStylexDefineVarsTokens) {
-          const check = ruleChecker(style.value, variables, style);
+          let varsWithFnArgs: Map<string, Expression | 'ARG'> = variables;
+          if (dynamicStyleVariables.size > 0) {
+            varsWithFnArgs = new Map();
+            for (const [key, value] of variables) {
+              varsWithFnArgs.set(key, value);
+            }
+            for (const key of dynamicStyleVariables) {
+              varsWithFnArgs.set(key, 'ARG');
+            }
+          }
+
+          const check = ruleChecker(style.value, varsWithFnArgs, style);
           if (check != null) {
             const { message, suggest } = check;
             return context.report(

diff --git a/packages/eslint-plugin/src/utils/evaluate.js b/packages/eslint-plugin/src/utils/evaluate.js
@@ -0,0 +1,38 @@
+/**
+ * Copyright (c) Meta Platforms, Inc. and affiliates.
+ *
+ * This source code is licensed under the MIT license found in the
+ * LICENSE file in the root directory of this source tree.
+ *
+ * @flow strict
+ */
+
+import type { Expression, Pattern, Literal } from 'estree';
+import type { Variables } from '../stylex-valid-styles';
+
+export default function evaluate(
+  node: Expression | Pattern,
+  variables?: Variables,
+): null | Literal | 'ARG' {
+  if (
+    // $FlowFixMe
+    node.type === 'TSSatisfiesExpression' ||
+    // $FlowFixMe
+    node.type === 'TSAsExpression'
+  ) {
+    return evaluate(node.expression, variables);
+  }
+  if (node.type === 'Identifier' && variables != null) {
+    const existingVar = variables.get(node.name);
+    if (existingVar === 'ARG') {
+      return 'ARG';
+    }
+    if (existingVar != null) {
+      return evaluate(existingVar, variables);
+    }
+  }
+  if (node.type === 'Literal') {
+    return node;
+  }
+  return null;
+}
diff --git a/packages/eslint-plugin/src/utils/makeVariableCheckingRule.js b/packages/eslint-plugin/src/utils/makeVariableCheckingRule.js
@@ -15,13 +15,29 @@ import type {
 } from '../stylex-valid-styles';
 
 export default function makeVariableCheckingRule(rule: RuleCheck): RuleCheck {
-  return (node: Expression | Pattern, variables?: Variables): RuleResponse => {
+  const varCheckingRule = (
+    node: Expression | Pattern,
+    variables?: Variables,
+  ): RuleResponse => {
+    if (
+      // $FlowFixMe
+      node.type === 'TSSatisfiesExpression' ||
+      // $FlowFixMe
+      node.type === 'TSAsExpression'
+    ) {
+      return varCheckingRule(node.expression, variables);
+    }
     if (node.type === 'Identifier' && variables != null) {
       const existingVar = variables.get(node.name);
+      if (existingVar === 'ARG') {
+        return undefined;
+      }
       if (existingVar != null) {
-        return rule(existingVar);
+        return varCheckingRule(existingVar, variables);
       }
     }
     return rule(node);
   };
+
+  return varCheckingRule;
 }