From e215aa1160adc51b8f31788872c11468585d5cc8 Mon Sep 17 00:00:00 2001 From: Mofei Zhang Date: Fri, 26 Jul 2024 17:36:04 -0400 Subject: [PATCH] [compiler] Fix FBT whitespace handling again (again (again)) ghstack-source-id: 00a86e41cfb8a6fb56b7fcd811740d1d9b89a611 Pull Request resolved: https://github.com/facebook/react/pull/30451 --- .../src/HIR/BuildHIR.ts | 62 +++++------ .../src/HIR/HIRBuilder.ts | 5 + ...ug-fbt-preserve-whitespace-param.expect.md | 100 ------------------ .../fbt/bug-fbt-preserve-whitespace-param.tsx | 32 ------ .../fbt-preserve-whitespace-subtree.expect.md | 89 ++++++++++++++++ .../fbt/fbt-preserve-whitespace-subtree.tsx | 26 +++++ ...preserve-whitespace-two-subtrees.expect.md | 81 ++++++++++++++ .../fbt-preserve-whitespace-two-subtrees.tsx | 25 +++++ .../packages/snap/src/SproutTodoFilter.ts | 1 - 9 files changed, 254 insertions(+), 167 deletions(-) delete mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/fbt/bug-fbt-preserve-whitespace-param.expect.md delete mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/fbt/bug-fbt-preserve-whitespace-param.tsx create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/fbt/fbt-preserve-whitespace-subtree.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/fbt/fbt-preserve-whitespace-subtree.tsx create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/fbt/fbt-preserve-whitespace-two-subtrees.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/fbt/fbt-preserve-whitespace-two-subtrees.tsx diff --git a/compiler/packages/babel-plugin-react-compiler/src/HIR/BuildHIR.ts b/compiler/packages/babel-plugin-react-compiler/src/HIR/BuildHIR.ts index 78aaa3e7c3..5dccb5ffd2 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/HIR/BuildHIR.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/HIR/BuildHIR.ts @@ -2113,10 +2113,10 @@ function lowerExpression( } props.push({kind: 'JsxAttribute', name: propName, place: value}); } - if ( - tag.kind === 'BuiltinTag' && - (tag.name === 'fbt' || tag.name === 'fbs') - ) { + + const isFbt = + tag.kind === 'BuiltinTag' && (tag.name === 'fbt' || tag.name === 'fbs'); + if (isFbt) { const tagName = tag.name; const openingIdentifier = opening.get('name'); const tagIdentifier = openingIdentifier.isJSXIdentifier() @@ -2168,35 +2168,17 @@ function lowerExpression( } } - let children: Array; - if ( - tag.kind === 'BuiltinTag' && - (tag.name === 'fbt' || tag.name === 'fbs') - ) { - children = expr - .get('children') - .map(child => { - if (child.isJSXText()) { - /* - * FBT whitespace normalization differs from standard JSX: - * https://github.com/facebook/fbt/blob/0b4e0d13c30bffd0daa2a75715d606e3587b4e40/packages/babel-plugin-fbt/src/FbtUtil.js#L76-L87 - */ - const text = child.node.value.replace(/[^\S\u00A0]+/g, ' '); - return lowerValueToTemporary(builder, { - kind: 'JSXText', - value: text, - loc: child.node.loc ?? GeneratedSource, - }); - } - return lowerJsxElement(builder, child); - }) - .filter(notNull); - } else { - children = expr - .get('children') - .map(child => lowerJsxElement(builder, child)) - .filter(notNull); - } + /** + * Increment fbt counter before traversing into children, as whitespace + * in jsx text is handled differently for fbt subtrees. + */ + isFbt && builder.fbtDepth++; + const children: Array = expr + .get('children') + .map(child => lowerJsxElement(builder, child)) + .filter(notNull); + isFbt && builder.fbtDepth--; + return { kind: 'JsxExpression', tag, @@ -3158,7 +3140,19 @@ function lowerJsxElement( return lowerExpressionToTemporary(builder, expression); } } else if (exprPath.isJSXText()) { - const text = trimJsxText(exprPath.node.value); + let text: string | null; + if (builder.fbtDepth > 0) { + /* + * FBT whitespace normalization differs from standard JSX. + * https://github.com/facebook/fbt/blob/0b4e0d13c30bffd0daa2a75715d606e3587b4e40/packages/babel-plugin-fbt/src/FbtUtil.js#L76-L87 + * Since the fbt transform runs after, let's just preserve all + * whitespace in FBT subtrees as is. + */ + text = exprPath.node.value; + } else { + text = trimJsxText(exprPath.node.value); + } + if (text === null) { return null; } diff --git a/compiler/packages/babel-plugin-react-compiler/src/HIR/HIRBuilder.ts b/compiler/packages/babel-plugin-react-compiler/src/HIR/HIRBuilder.ts index 628b3e1562..0b47b9ae70 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/HIR/HIRBuilder.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/HIR/HIRBuilder.ts @@ -110,6 +110,11 @@ export default class HIRBuilder { #exceptionHandlerStack: Array = []; parentFunction: NodePath; errors: CompilerError = new CompilerError(); + /** + * Traversal context: counts the number of `fbt` tag parents + * of the current babel node. + */ + fbtDepth: number = 0; get nextIdentifierId(): IdentifierId { return this.#env.nextIdentifierId; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/fbt/bug-fbt-preserve-whitespace-param.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/fbt/bug-fbt-preserve-whitespace-param.expect.md deleted file mode 100644 index 3baa1d9811..0000000000 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/fbt/bug-fbt-preserve-whitespace-param.expect.md +++ /dev/null @@ -1,100 +0,0 @@ - -## Input - -```javascript -import fbt from 'fbt'; - -/** - * Currently fails with the following: - * Found differences in evaluator results - * Non-forget (expected): - * (kind: ok)
Jason !
- * Forget: - * (kind: ok)
Jason!
- - */ - -function Foo(props) { - return ( - // prettier-ignore -
- - - - {props.name} - - ! - - -
- ); -} - -export const FIXTURE_ENTRYPOINT = { - fn: Foo, - params: [{name: 'Jason'}], -}; - -``` - -## Code - -```javascript -import { c as _c } from "react/compiler-runtime"; -import fbt from "fbt"; - -/** - * Currently fails with the following: - * Found differences in evaluator results - * Non-forget (expected): - * (kind: ok)
Jason !
- * Forget: - * (kind: ok)
Jason!
- - */ - -function Foo(props) { - const $ = _c(2); - let t0; - if ($[0] !== props.name) { - t0 = ( -
- {fbt._( - "{=m0}", - [ - fbt._implicitParam( - "=m0", - - {fbt._( - "{user name}!", - [ - fbt._param( - "user name", - - props.name, - ), - ], - { hk: "mBBZ9" }, - )} - , - ), - ], - { hk: "3RVfuk" }, - )} -
- ); - $[0] = props.name; - $[1] = t0; - } else { - t0 = $[1]; - } - return t0; -} - -export const FIXTURE_ENTRYPOINT = { - fn: Foo, - params: [{ name: "Jason" }], -}; - -``` - \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/fbt/bug-fbt-preserve-whitespace-param.tsx b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/fbt/bug-fbt-preserve-whitespace-param.tsx deleted file mode 100644 index 3f0b845335..0000000000 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/fbt/bug-fbt-preserve-whitespace-param.tsx +++ /dev/null @@ -1,32 +0,0 @@ -import fbt from 'fbt'; - -/** - * Currently fails with the following: - * Found differences in evaluator results - * Non-forget (expected): - * (kind: ok)
Jason !
- * Forget: - * (kind: ok)
Jason!
- - */ - -function Foo(props) { - return ( - // prettier-ignore -
- - - - {props.name} - - ! - - -
- ); -} - -export const FIXTURE_ENTRYPOINT = { - fn: Foo, - params: [{name: 'Jason'}], -}; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/fbt/fbt-preserve-whitespace-subtree.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/fbt/fbt-preserve-whitespace-subtree.expect.md new file mode 100644 index 0000000000..e4e16a9e1d --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/fbt/fbt-preserve-whitespace-subtree.expect.md @@ -0,0 +1,89 @@ + +## Input + +```javascript +import fbt from 'fbt'; + +/** + * Note that fbt whitespace rules apply to the entire fbt subtree, + * not just direct children of fbt elements. + * (e.g. here, the JSXText children of the span element also use + * fbt whitespace rules) + */ + +function Foo(props) { + return ( + + + + {props.name} + + ! + + + ); +} + +export const FIXTURE_ENTRYPOINT = { + fn: Foo, + params: [{name: 'Jason'}], +}; + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; +import fbt from "fbt"; + +/** + * Note that fbt whitespace rules apply to the entire fbt subtree, + * not just direct children of fbt elements. + * (e.g. here, the JSXText children of the span element also use + * fbt whitespace rules) + */ + +function Foo(props) { + const $ = _c(2); + let t0; + if ($[0] !== props.name) { + t0 = fbt._( + "{=m0}", + [ + fbt._implicitParam( + "=m0", + + {fbt._( + "{user name really long description for prettier} !", + [ + fbt._param( + "user name really long description for prettier", + + props.name, + ), + ], + { hk: "rdgIJ" }, + )} + , + ), + ], + { hk: "32Ufy5" }, + ); + $[0] = props.name; + $[1] = t0; + } else { + t0 = $[1]; + } + return t0; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Foo, + params: [{ name: "Jason" }], +}; + +``` + +### Eval output +(kind: ok) Jason ! \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/fbt/fbt-preserve-whitespace-subtree.tsx b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/fbt/fbt-preserve-whitespace-subtree.tsx new file mode 100644 index 0000000000..c7790be17e --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/fbt/fbt-preserve-whitespace-subtree.tsx @@ -0,0 +1,26 @@ +import fbt from 'fbt'; + +/** + * Note that fbt whitespace rules apply to the entire fbt subtree, + * not just direct children of fbt elements. + * (e.g. here, the JSXText children of the span element also use + * fbt whitespace rules) + */ + +function Foo(props) { + return ( + + + + {props.name} + + ! + + + ); +} + +export const FIXTURE_ENTRYPOINT = { + fn: Foo, + params: [{name: 'Jason'}], +}; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/fbt/fbt-preserve-whitespace-two-subtrees.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/fbt/fbt-preserve-whitespace-two-subtrees.expect.md new file mode 100644 index 0000000000..89aed91eec --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/fbt/fbt-preserve-whitespace-two-subtrees.expect.md @@ -0,0 +1,81 @@ + +## Input + +```javascript +import fbt from 'fbt'; + +function Foo({name1, name2}) { + return ( + + + + {name1} + + + and + + + {name2} + + + accepted your PR! + + ); +} + +export const FIXTURE_ENTRYPOINT = { + fn: Foo, + params: [{name1: 'Mike', name2: 'Jan'}], +}; + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; +import fbt from "fbt"; + +function Foo(t0) { + const $ = _c(3); + const { name1, name2 } = t0; + let t1; + if ($[0] !== name1 || $[1] !== name2) { + t1 = fbt._( + "{user1} and {user2} accepted your PR!", + [ + fbt._param( + "user1", + + + {name1} + , + ), + fbt._param( + "user2", + + + {name2} + , + ), + ], + { hk: "2PxMie" }, + ); + $[0] = name1; + $[1] = name2; + $[2] = t1; + } else { + t1 = $[2]; + } + return t1; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Foo, + params: [{ name1: "Mike", name2: "Jan" }], +}; + +``` + +### Eval output +(kind: ok) Mike and Jan accepted your PR! \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/fbt/fbt-preserve-whitespace-two-subtrees.tsx b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/fbt/fbt-preserve-whitespace-two-subtrees.tsx new file mode 100644 index 0000000000..67cfc3873a --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/fbt/fbt-preserve-whitespace-two-subtrees.tsx @@ -0,0 +1,25 @@ +import fbt from 'fbt'; + +function Foo({name1, name2}) { + return ( + + + + {name1} + + + and + + + {name2} + + + accepted your PR! + + ); +} + +export const FIXTURE_ENTRYPOINT = { + fn: Foo, + params: [{name1: 'Mike', name2: 'Jan'}], +}; diff --git a/compiler/packages/snap/src/SproutTodoFilter.ts b/compiler/packages/snap/src/SproutTodoFilter.ts index 9abcbe6a5e..ff542dfb97 100644 --- a/compiler/packages/snap/src/SproutTodoFilter.ts +++ b/compiler/packages/snap/src/SproutTodoFilter.ts @@ -484,7 +484,6 @@ const skipFilter = new Set([ 'rules-of-hooks/rules-of-hooks-69521d94fa03', // bugs - 'fbt/bug-fbt-preserve-whitespace-param', 'bug-invalid-hoisting-functionexpr', 'original-reactive-scopes-fork/bug-nonmutating-capture-in-unsplittable-memo-block', 'original-reactive-scopes-fork/bug-hoisted-declaration-with-scope',