From b8baa502c99d9e418dd24c26caf240769c0654c6 Mon Sep 17 00:00:00 2001 From: Naman Goel Date: Sun, 22 Dec 2024 12:36:33 +0300 Subject: [PATCH] feat: Better, granular error messages (#802) * feat: Better, granular error messages * fix errors * feat: Even better errors. Fixed local function eval * chore: another test --- .../stylex-validation-create-test.js | 188 +++++++++++++++++- .../babel-plugin/src/utils/evaluate-path.js | 177 ++++++++++++----- .../src/utils/evaluation-errors.js | 58 ++++++ .../src/visitors/stylex-create/index.js | 17 +- .../stylex-create/parse-stylex-create-arg.js | 2 + 5 files changed, 381 insertions(+), 61 deletions(-) create mode 100644 packages/babel-plugin/src/utils/evaluation-errors.js diff --git a/packages/babel-plugin/__tests__/stylex-validation-create-test.js b/packages/babel-plugin/__tests__/stylex-validation-create-test.js index a1ff0cba4..342e16140 100644 --- a/packages/babel-plugin/__tests__/stylex-validation-create-test.js +++ b/packages/babel-plugin/__tests__/stylex-validation-create-test.js @@ -13,6 +13,39 @@ import { transformSync } from '@babel/core'; import { messages } from '@stylexjs/shared'; import stylexPlugin from '../src/index'; +// Valid string terminator sequences are BEL, ESC\, and 0x9c +const ST = '(?:\\u0007|\\u001B\\u005C|\\u009C)'; +const pattern = [ + `[\\u001B\\u009B][[\\]()#;?]*(?:(?:(?:(?:;[-a-zA-Z\\d\\/#&.:=?%@~_]+)*|[a-zA-Z\\d]+(?:;[-a-zA-Z\\d\\/#&.:=?%@~_]*)*)?${ST})`, + '(?:(?:\\d{1,4}(?:;\\d{0,4})*)?[\\dA-PR-TZcf-nq-uy=><~]))', +].join('|'); + +const regex = new RegExp(pattern, 'g'); + +function stripAnsi(string: string): string { + if (typeof string !== 'string') { + throw new TypeError(`Expected a \`string\`, got \`${typeof string}\``); + } + + // Even though the regex is global, we don't need to reset the `.lastIndex` + // because unlike `.exec()` and `.test()`, `.replace()` does it automatically + // and doing it manually has a performance penalty. + return string.replace(regex, ''); +} + +function cleanError(fn: () => mixed) { + return () => { + try { + fn(); + } catch (error) { + error.message = stripAnsi(error.message); + throw error; + } + }; +} + +const cleanExpect = (fn: () => mixed) => expect(cleanError(fn)); + function transform(source: string, opts: { [key: string]: any } = {}) { return transformSync(source, { filename: opts.filename, @@ -163,7 +196,7 @@ describe('@stylexjs/babel-plugin', () => { /* Properties */ test('properties must be a static value', () => { - expect(() => { + cleanExpect(() => { transform(` import stylex from 'stylex'; const styles = stylex.create({ @@ -172,7 +205,16 @@ describe('@stylexjs/babel-plugin', () => { } }); `); - }).toThrow(messages.NON_STATIC_VALUE); + }).toThrowErrorMatchingInlineSnapshot(` + "unknown file: Referenced constant is not defined. + 3 | const styles = stylex.create({ + 4 | root: { + > 5 | [backgroundColor]: 'red', + | ^^^^^^^^^^^^^^^ + 6 | } + 7 | }); + 8 | " + `); }); /* Values */ @@ -223,7 +265,7 @@ describe('@stylexjs/babel-plugin', () => { `); }).not.toThrow(); // not string or number - expect(() => { + cleanExpect(() => { transform(` import stylex from 'stylex'; const styles = stylex.create({ @@ -232,7 +274,9 @@ describe('@stylexjs/babel-plugin', () => { }, }); `); - }).toThrow(messages.ILLEGAL_PROP_ARRAY_VALUE); + }).toThrowErrorMatchingInlineSnapshot( + '"unknown file: A style array value can only contain strings or numbers."', + ); expect(() => { transform(` import stylex from 'stylex'; @@ -244,7 +288,7 @@ describe('@stylexjs/babel-plugin', () => { `); }).toThrow(messages.ILLEGAL_PROP_VALUE); // not static - expect(() => { + cleanExpect(() => { transform(` import stylex from 'stylex'; const styles = stylex.create({ @@ -253,8 +297,17 @@ describe('@stylexjs/babel-plugin', () => { } }); `); - }).toThrow(messages.NON_STATIC_VALUE); - expect(() => { + }).toThrowErrorMatchingInlineSnapshot(` + "unknown file: Referenced constant is not defined. + 3 | const styles = stylex.create({ + 4 | root: { + > 5 | backgroundColor: backgroundColor, + | ^^^^^^^^^^^^^^^ + 6 | } + 7 | }); + 8 | " + `); + cleanExpect(() => { transform(` import stylex from 'stylex'; const styles = stylex.create({ @@ -263,7 +316,124 @@ describe('@stylexjs/babel-plugin', () => { } }); `); - }).toThrow(messages.NON_STATIC_VALUE); + }).toThrowErrorMatchingInlineSnapshot(` + "unknown file: Referenced constant is not defined. + 3 | const styles = stylex.create({ + 4 | root: { + > 5 | backgroundColor: generateBg(), + | ^^^^^^^^^^ + 6 | } + 7 | }); + 8 | " + `); + cleanExpect(() => { + transform(` + import stylex from 'stylex'; + import {generateBg} from './other-file'; + const styles = stylex.create({ + root: { + backgroundColor: generateBg(), + } + }); + `); + }).toThrowErrorMatchingInlineSnapshot(` + "unknown file: Could not resolve the path to the imported file. + Please ensure that the theme file has a .stylex.js or .stylex.ts file extension and follows the + rules for defining variariables: + + https://stylexjs.com/docs/learn/theming/defining-variables/#rules-when-defining-variables + + 1 | + 2 | import stylex from 'stylex'; + > 3 | import {generateBg} from './other-file'; + | ^^^^^^^^^^ + 4 | const styles = stylex.create({ + 5 | root: { + 6 | backgroundColor: generateBg()," + `); + + cleanExpect(() => { + transform(` + import stylex from 'stylex'; + import generateBg from './other-file'; + const styles = stylex.create({ + root: { + backgroundColor: generateBg(), + } + }); + `); + }).toThrowErrorMatchingInlineSnapshot(` + "unknown file: There was an error when attempting to evaluate the imported file. + Please ensure that the imported file is self-contained and does not rely on dynamic behavior. + + 1 | + 2 | import stylex from 'stylex'; + > 3 | import generateBg from './other-file'; + | ^^^^^^^^^^ + 4 | const styles = stylex.create({ + 5 | root: { + 6 | backgroundColor: generateBg()," + `); + }); + + test('[validation] can evaluate single-expr function calls', () => { + expect(() => + transform(` + import stylex from 'stylex'; + const generateBg = () => 'red'; + export const styles = stylex.create({ + root: { + backgroundColor: generateBg(), + } + }); + `), + ).not.toThrow(); + }); + + test('[validation] can evaluate single-expr function calls in objects', () => { + let result; + expect(() => { + result = transform(` + import stylex from 'stylex'; + const fns = { + generateBg: () => 'red', + }; + export const styles = stylex.create({ + root: { + backgroundColor: fns.generateBg(), + } + }); + `); + }).not.toThrow(); + + expect(result).not.toBeFalsy(); + + expect(result?.code).toMatchInlineSnapshot(` + "import stylex from 'stylex'; + const fns = { + generateBg: () => 'red' + }; + export const styles = { + root: { + backgroundColor: "xrkmrrc", + $$css: true + } + };" + `); + + // $FlowFixMe + expect(result?.metadata?.stylex).toMatchInlineSnapshot(` + [ + [ + "xrkmrrc", + { + "ltr": ".xrkmrrc{background-color:red}", + "rtl": null, + }, + 3000, + ], + ] + `); }); test('values can reference local bindings in stylex.create()', () => { @@ -334,7 +504,7 @@ describe('@stylexjs/babel-plugin', () => { }, }); `); - }).not.toThrow(messages.INVALID_PSEUDO); + }).not.toThrow(); expect(() => { transform(` diff --git a/packages/babel-plugin/src/utils/evaluate-path.js b/packages/babel-plugin/src/utils/evaluate-path.js index a9b0de1af..1ba4c006c 100644 --- a/packages/babel-plugin/src/utils/evaluate-path.js +++ b/packages/babel-plugin/src/utils/evaluate-path.js @@ -28,6 +28,7 @@ import * as t from '@babel/types'; import StateManager from './state-manager'; import { utils } from '@stylexjs/shared'; import * as pathUtils from '../babel-path-utils'; +import * as errMsgs from './evaluation-errors'; // This file contains Babels metainterpreter that can evaluate static code. @@ -67,29 +68,37 @@ export type FunctionConfig = { type State = { confident: boolean, deoptPath: NodePath<> | null, + deoptReason?: string, seen: Map, addedImports: Set, functions: FunctionConfig, traversalState: StateManager, }; -type Result = { - resolved: boolean, - value?: any, -}; +type Result = + | { + resolved: true, + value: any, + } + | { + resolved: false, + reason: string, + }; /** * Deopts the evaluation */ -function deopt(path: NodePath<>, state: State) { +function deopt(path: NodePath<>, state: State, reason: string): void { if (!state.confident) return; state.deoptPath = path; state.confident = false; + state.deoptReason = reason; } function evaluateImportedFile( filePath: string, namedExport: string, state: State, + bindingPath: NodePath<>, ): any { const fs = require('fs'); const fileContents = fs.readFileSync(filePath, 'utf8'); @@ -101,7 +110,7 @@ function evaluateImportedFile( babelrc: true, }); if (!ast || ast.errors || !t.isNode(ast)) { - state.confident = false; + deopt(bindingPath, state, errMsgs.IMPORT_FILE_PARSING_ERROR); return; } @@ -144,7 +153,7 @@ function evaluateImportedFile( if (state.confident) { return result; } else { - state.confident = false; + deopt(bindingPath, state, errMsgs.IMPORT_FILE_EVAL_ERROR); return; } } @@ -215,23 +224,26 @@ function evaluateCached(path: NodePath<>, state: State): any { if (existing.resolved) { return existing.value; } else { - deopt(path, state); + deopt(path, state, existing.reason); return; } } else { - const item: Result = { resolved: false }; + const item: Result = { resolved: false, reason: 'Currently evaluating' }; seen.set(node, item); if (node == null) { - deopt(path, state); + deopt(path, state, errMsgs.PATH_WITHOUT_NODE); return; } const val = _evaluate(path, state); if (state.confident) { - item.resolved = true; - item.value = val; + seen.set(node, { + resolved: true, + value: val, + }); } + return val; } } @@ -368,7 +380,7 @@ function _evaluate(path: NodePath<>, state: State): any { } else if (pathUtils.isStringLiteral(propPath)) { property = propPath.node.value; } else { - return deopt(propPath, state); + return deopt(propPath, state, errMsgs.UNEXPECTED_MEMBER_LOOKUP); } return object[property]; @@ -398,14 +410,18 @@ function _evaluate(path: NodePath<>, state: State): any { importPath.node.source.value, ); if (!absPath) { - return deopt(binding.path, state); + return deopt( + binding.path, + state, + errMsgs.IMPORT_PATH_RESOLUTION_ERROR, + ); } const [type, value] = absPath; const returnValue = type === 'themeNameRef' ? evaluateThemeRef(value, importedName, state) - : evaluateImportedFile(value, importedName, state); + : evaluateImportedFile(value, importedName, state, bindingPath); if (state.confident) { if ( !state.addedImports.has(importPath.node.source.value) && @@ -418,33 +434,47 @@ function _evaluate(path: NodePath<>, state: State): any { } return returnValue; } else { - deopt(binding.path, state); + deopt(binding.path, state, errMsgs.IMPORT_FILE_EVAL_ERROR); } } } + if ( + binding && + bindingPath && + pathUtils.isImportDefaultSpecifier(bindingPath) + ) { + deopt(binding.path, state, errMsgs.IMPORT_FILE_EVAL_ERROR); + } + if (binding && binding.constantViolations.length > 0) { - return deopt(binding.path, state); + return deopt(binding.path, state, errMsgs.NON_CONSTANT); } if (binding && path.node.start < binding.path.node.end) { - return deopt(binding.path, state); + return deopt(binding.path, state, errMsgs.USED_BEFORE_DECLARATION); } if (binding && binding.hasValue) { return binding.value; } else { if (path.node.name === 'undefined') { - return binding ? deopt(binding.path, state) : undefined; + return binding + ? deopt(binding.path, state, errMsgs.UNINITIALIZED_CONST) + : undefined; } else if (path.node.name === 'Infinity') { - return binding ? deopt(binding.path, state) : Infinity; + return binding + ? deopt(binding.path, state, errMsgs.UNINITIALIZED_CONST) + : Infinity; } else if (path.node.name === 'NaN') { - return binding ? deopt(binding.path, state) : NaN; + return binding + ? deopt(binding.path, state, errMsgs.UNINITIALIZED_CONST) + : NaN; } const resolved = (path as $FlowFixMe).resolve(); if (resolved === path) { - return deopt(path, state); + return deopt(path, state, errMsgs.UNDEFINED_CONST); } else { return evaluateCached(resolved, state); } @@ -481,7 +511,11 @@ function _evaluate(path: NodePath<>, state: State): any { case 'void': return undefined; default: - return deopt(path, state); + return deopt( + path, + state, + errMsgs.UNSUPPORTED_OPERATOR(path.node.operator), + ); } } @@ -495,7 +529,8 @@ function _evaluate(path: NodePath<>, state: State): any { if (elemValue.confident) { arr.push(elemValue.value); } else { - elemValue.deopt && deopt(elemValue.deopt, state); + elemValue.deopt && + deopt(elemValue.deopt, state, elemValue.reason ?? 'unknown error'); return; } } @@ -509,12 +544,12 @@ function _evaluate(path: NodePath<>, state: State): any { > = path.get('properties'); for (const prop of props) { if (pathUtils.isObjectMethod(prop)) { - return deopt(prop, state); + return deopt(prop, state, errMsgs.OBJECT_METHOD); } if (pathUtils.isSpreadElement(prop)) { const spreadExpression = evaluateCached(prop.get('argument'), state); if (!state.confident) { - return deopt(prop, state); + return deopt(prop, state, state.deoptReason ?? 'unknown error'); } Object.assign(obj, spreadExpression); continue; @@ -526,10 +561,12 @@ function _evaluate(path: NodePath<>, state: State): any { const { confident, deopt: resultDeopt, + reason: deoptReason, value, } = evaluate(keyPath, state.traversalState, state.functions); if (!confident) { - resultDeopt && deopt(resultDeopt, state); + resultDeopt && + deopt(resultDeopt, state, deoptReason ?? 'unknown error'); return; } key = value; @@ -543,7 +580,8 @@ function _evaluate(path: NodePath<>, state: State): any { const valuePath: NodePath<> = prop.get('value'); let value = evaluate(valuePath, state.traversalState, state.functions); if (!value.confident) { - value.deopt && deopt(value.deopt, state); + value.deopt && + deopt(value.deopt, state, value.reason ?? 'unknown error'); return; } value = value.value; @@ -556,31 +594,67 @@ function _evaluate(path: NodePath<>, state: State): any { if (pathUtils.isLogicalExpression(path)) { // If we are confident that the left side of an && is false, or the left // side of an || is true, we can be confident about the entire expression - const wasConfident = state.confident; - const left = evaluateCached(path.get('left'), state); - const leftConfident = state.confident; - state.confident = wasConfident; - const right = evaluateCached(path.get('right'), state); - const rightConfident = state.confident; + const stateForLeft = { ...state, deoptPath: null, confident: true }; + const leftPath = path.get('left'); + const left = evaluateCached(leftPath, stateForLeft); + const leftConfident = stateForLeft.confident; + + const stateForRight = { ...state, deoptPath: null, confident: true }; + const rightPath = path.get('right'); + const right = evaluateCached(rightPath, stateForRight); + const rightConfident = stateForRight.confident; switch (path.node.operator) { - case '||': + case '||': { // TODO consider having a "truthy type" that doesn't bail on // left uncertainty but can still evaluate to truthy. - state.confident = leftConfident && (!!left || rightConfident); - if (!state.confident) return; + if (leftConfident && (!!left || rightConfident)) { + return left || right; + } + if (!leftConfident) { + deopt(leftPath, state, stateForLeft.deoptReason ?? 'unknown error'); + return; + } + if (!rightConfident) { + deopt(rightPath, state, stateForRight.deoptReason ?? 'unknown error'); + return; + } - return left || right; - case '&&': - state.confident = leftConfident && (!left || rightConfident); - if (!state.confident) return; + deopt(path, state, 'unknown error'); + return; + } + case '&&': { + if (leftConfident && (!left || rightConfident)) { + return left && right; + } + if (!leftConfident) { + deopt(leftPath, state, stateForLeft.deoptReason ?? 'unknown error'); + return; + } + if (!rightConfident) { + deopt(rightPath, state, stateForRight.deoptReason ?? 'unknown error'); + return; + } - return left && right; - case '??': - state.confident = leftConfident && !!(left ?? rightConfident); - if (!state.confident) return; + deopt(path, state, 'unknown error'); + return; + } + case '??': { + if (leftConfident && !!(left ?? rightConfident)) { + return left ?? right; + } + if (!leftConfident) { + deopt(leftPath, state, stateForLeft.deoptReason ?? 'unknown error'); + return; + } + if (!rightConfident) { + deopt(rightPath, state, stateForRight.deoptReason ?? 'unknown error'); + return; + } - return left ?? right; + deopt(path, state, 'unknown error'); + return; + } default: path.node.operator as empty; } @@ -659,6 +733,13 @@ function _evaluate(path: NodePath<>, state: State): any { state.functions.identifiers[callee.node.name] ) { func = state.functions.identifiers[callee.node.name]; + } else if (pathUtils.isIdentifier(callee)) { + const maybeFunction = evaluateCached(callee, state); + if (state.confident) { + func = maybeFunction; + } else { + deopt(callee, state, errMsgs.NON_CONSTANT); + } } if (pathUtils.isMemberExpression(callee)) { @@ -746,7 +827,7 @@ function _evaluate(path: NodePath<>, state: State): any { } } - deopt(path, state); + deopt(path, state, errMsgs.UNSUPPORTED_EXPRESSION(path.node.type)); } function evaluateQuasis( @@ -815,6 +896,7 @@ export function evaluate( confident: boolean, value: any, deopt?: null | NodePath<>, + reason?: string, }> { const addedImports = importsForState.get(traversalState) ?? new Set(); importsForState.set(traversalState, addedImports); @@ -833,6 +915,7 @@ export function evaluate( return { confident: state.confident, deopt: state.deoptPath, + reason: state.deoptReason, value: value, }; } diff --git a/packages/babel-plugin/src/utils/evaluation-errors.js b/packages/babel-plugin/src/utils/evaluation-errors.js new file mode 100644 index 000000000..37811bbb1 --- /dev/null +++ b/packages/babel-plugin/src/utils/evaluation-errors.js @@ -0,0 +1,58 @@ +/** + * 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 + */ + +export const IMPORT_FILE_PARSING_ERROR = `There was error when attempting to parse the imported file. +Please ensure that the 'babelrc' file is configured to be able to parse this file. +`; + +// export const +export const IMPORT_FILE_EVAL_ERROR = `There was an error when attempting to evaluate the imported file. +Please ensure that the imported file is self-contained and does not rely on dynamic behavior. +`; + +export const DEFAULT_IMPORT = `Error: Cannot use default imports. + +Please define your styles without depending on values imported from other files. + +You *may* use named imports to use variables defined with \`stylex.defineVars\` in a file with \`.stylex.js\` or \`.stylex.ts\` file. +See: https://stylexjs.com/docs/learn/theming/defining-variables/#rules-when-defining-variables for more information. +`; + +export const PATH_WITHOUT_NODE = `Unexpected error: +Could not resolve the code being evaluated. +`; + +export const UNEXPECTED_MEMBER_LOOKUP = `Unexpected error: +Could not determine the property being accessed. +`; + +export const IMPORT_PATH_RESOLUTION_ERROR = `Could not resolve the path to the imported file. +Please ensure that the theme file has a .stylex.js or .stylex.ts file extension and follows the +rules for defining variariables: + +https://stylexjs.com/docs/learn/theming/defining-variables/#rules-when-defining-variables +`; + +export const NON_CONSTANT = 'Referenced value is not a constant.\n\n'; + +export const USED_BEFORE_DECLARATION = + 'Referenced value is used before declaration.\n\n'; + +export const UNINITIALIZED_CONST = + 'Referenced constant is not initialized.\n\n'; + +export const UNDEFINED_CONST = 'Referenced constant is not defined.'; + +export const UNSUPPORTED_OPERATOR = (op: string): string => + `Unsupported operator: ${op}\n\n`; + +export const OBJECT_METHOD = 'Unsupported object method.\n\n'; + +export const UNSUPPORTED_EXPRESSION = (type: string): string => + `Unsupported expression: ${type}\n\n`; diff --git a/packages/babel-plugin/src/visitors/stylex-create/index.js b/packages/babel-plugin/src/visitors/stylex-create/index.js index 328b696f2..277139509 100644 --- a/packages/babel-plugin/src/visitors/stylex-create/index.js +++ b/packages/babel-plugin/src/visitors/stylex-create/index.js @@ -108,13 +108,20 @@ export default function transformStyleXCreate( memberExpressions[name].keyframes = { fn: keyframes }; }); - const { confident, value, fns } = evaluateStyleXCreateArg(firstArg, state, { - identifiers, - memberExpressions, - }); + const { confident, value, fns, reason, deopt } = evaluateStyleXCreateArg( + firstArg, + state, + { + identifiers, + memberExpressions, + }, + ); if (!confident) { - throw path.buildCodeFrameError(messages.NON_STATIC_VALUE, SyntaxError); + throw (deopt ?? path).buildCodeFrameError( + reason ?? messages.NON_STATIC_VALUE, + SyntaxError, + ); } const plainObject = value; diff --git a/packages/babel-plugin/src/visitors/stylex-create/parse-stylex-create-arg.js b/packages/babel-plugin/src/visitors/stylex-create/parse-stylex-create-arg.js index 77142bd43..3489f8da7 100644 --- a/packages/babel-plugin/src/visitors/stylex-create/parse-stylex-create-arg.js +++ b/packages/babel-plugin/src/visitors/stylex-create/parse-stylex-create-arg.js @@ -45,6 +45,7 @@ export function evaluateStyleXCreateArg( confident: boolean, value: any, deopt?: null | NodePath<>, + reason?: string, fns?: DynamicFns, }> { if (!pathUtils.isObjectExpression(path)) { @@ -122,6 +123,7 @@ function evaluatePartialObjectRecursively( confident: boolean, value: any, deopt?: null | NodePath<>, + reason?: string, inlineStyles?: $ReadOnly, }> { const obj: { [string]: mixed } = {};