Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[compiler] Todo for fbt with multiple pronoun/plural #30437

Merged
merged 4 commits into from
Jul 29, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 30 additions & 13 deletions compiler/packages/babel-plugin-react-compiler/src/HIR/BuildHIR.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2130,24 +2130,41 @@ function lowerExpression(
suggestions: null,
});
}
const fbtEnumLocations: Array<SourceLocation> = [];
// see `error.todo-multiple-fbt-plural` fixture for explanation
const fbtLocations = {
enum: new Array<SourceLocation>(),
plural: new Array<SourceLocation>(),
pronoun: new Array<SourceLocation>(),
};
Comment on lines +2134 to +2138
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These can be tracked separately as fbt does not dedupe across enum/plural/pronouns

expr.traverse({
JSXClosingElement(path) {
path.skip();
},
JSXNamespacedName(path) {
if (
path.node.namespace.name === tagName &&
path.node.name.name === 'enum'
) {
fbtEnumLocations.push(path.node.loc ?? GeneratedSource);
if (path.node.namespace.name === tagName) {
switch (path.node.name.name) {
case 'enum':
fbtLocations.enum.push(path.node.loc ?? GeneratedSource);
break;
case 'plural':
fbtLocations.plural.push(path.node.loc ?? GeneratedSource);
break;
case 'pronoun':
fbtLocations.pronoun.push(path.node.loc ?? GeneratedSource);
break;
}
}
},
});
if (fbtEnumLocations.length > 1) {
CompilerError.throwTodo({
reason: `Support <${tagName}> tags with multiple <${tagName}:enum> values`,
loc: fbtEnumLocations.at(-1) ?? GeneratedSource,
description: null,
suggestions: null,
});
for (const [name, locations] of Object.entries(fbtLocations)) {
if (locations.length > 1) {
CompilerError.throwTodo({
reason: `Support <${tagName}> tags with multiple <${tagName}:${name}> values`,
loc: locations.at(-1) ?? GeneratedSource,
description: null,
suggestions: null,
});
}
}
}

Expand Down

This file was deleted.

Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@

## Input

```javascript
import fbt from 'fbt';

/**
* Forget + fbt inconsistency. Evaluator errors with the following
* Found differences in evaluator results
* Non-forget (expected):
* (kind: ok) 1 rewrite to Rust · 2 months traveling
* Forget:
* (kind: ok) 1 rewrites to Rust · 2 months traveling
*
* The root issue here is that fbt:plural/enum/pronoun read `.start` and `.end` from
* babel nodes to slice into source strings for some complex dedupe logic
* (see [_getStringVariationCombinations](https://github.com/facebook/fbt/blob/main/packages/babel-plugin-fbt/src/JSFbtBuilder.js#L297))
*
*
* Since Forget does not add `.start` and `.end` for babel nodes it synthesizes,
* [getRawSource](https://github.com/facebook/fbt/blob/main/packages/babel-plugin-fbt/src/FbtUtil.js#L666-L673)
* simply returns the whole source code string. As a result, all fbt nodes dedupe together
* and _getStringVariationCombinations ends up early exiting (before adding valid candidate values).
*
*
*
* For fbt:plural tags specifically, the `count` node require that a `.start/.end`
* (see [code in FbtPluralNode](https://github.com/facebook/fbt/blob/main/packages/babel-plugin-fbt/src/fbt-nodes/FbtPluralNode.js#L87-L90))
*/
function Foo({rewrites, months}) {
return (
<fbt desc="Test fbt description">
<fbt:plural count={rewrites} name="number of rewrites" showCount="yes">
rewrite
</fbt:plural>
to Rust ·
<fbt:plural count={months} name="number of months" showCount="yes">
month
</fbt:plural>
traveling
</fbt>
);
}

export const FIXTURE_ENTRYPOINT = {
fn: Foo,
params: [{rewrites: 1, months: 2}],
};

```


## Error

```
31 | </fbt:plural>
32 | to Rust ·
> 33 | <fbt:plural count={months} name="number of months" showCount="yes">
| ^^^^^^^^^^ Todo: Support <fbt> tags with multiple <fbt:plural> values (33:33)
34 | month
35 | </fbt:plural>
36 | traveling
```


Original file line number Diff line number Diff line change
Expand Up @@ -8,18 +8,20 @@ import fbt from 'fbt';
* Forget:
* (kind: ok) 1 rewrites to Rust · 2 months traveling
*
* The root issue here is that fbt:plural reads `.start` and `.end` from
* babel nodes to slice into source strings. (fbt:enum suffers from the same
* problem).
* See:
* - [getRawSource](https://github.com/facebook/fbt/blob/main/packages/babel-plugin-fbt/src/FbtUtil.js#L666-L673)
* - [getArgCode](https://github.com/facebook/fbt/blob/main/packages/babel-plugin-fbt/src/fbt-nodes/FbtArguments.js#L88-L97)
* - [_getStringVariationCombinations](https://github.com/facebook/fbt/blob/main/packages/babel-plugin-fbt/src/JSFbtBuilder.js#L297)
* The root issue here is that fbt:plural/enum/pronoun read `.start` and `.end` from
* babel nodes to slice into source strings for some complex dedupe logic
* (see [_getStringVariationCombinations](https://github.com/facebook/fbt/blob/main/packages/babel-plugin-fbt/src/JSFbtBuilder.js#L297))
*
*
* Since Forget does not add `.start` and `.end` for babel nodes it synthesizes,
* [getRawSource](https://github.com/facebook/fbt/blob/main/packages/babel-plugin-fbt/src/FbtUtil.js#L666-L673)
* simply returns the whole source code string. As a result, all fbt nodes dedupe together
* and _getStringVariationCombinations ends up early exiting (before adding valid candidate values).
*
* Specifically, the `count` node requires that a `.start/.end` be attached
* (see [code in FbtPluralNode](https://github.com/facebook/fbt/blob/main/packages/babel-plugin-fbt/src/fbt-nodes/FbtPluralNode.js#L87-L90))
*
* In this fixture, `count` nodes are the `rewrites` and `months` identifiers.
*
* For fbt:plural tags specifically, the `count` node require that a `.start/.end`
* (see [code in FbtPluralNode](https://github.com/facebook/fbt/blob/main/packages/babel-plugin-fbt/src/fbt-nodes/FbtPluralNode.js#L87-L90))
*/
function Foo({rewrites, months}) {
return (
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,12 @@ function Foo(props) {
);
}

export const FIXTURE_ENTRYPOINT = {
fn: Foo,
params: [{value: 1}],
sequentialRenders: [{value: 1}, {value: 0}],
};

```

## Code
Expand Down Expand Up @@ -49,5 +55,14 @@ function Foo(props) {
return t0;
}

export const FIXTURE_ENTRYPOINT = {
fn: Foo,
params: [{ value: 1 }],
sequentialRenders: [{ value: 1 }, { value: 0 }],
};

```


### Eval output
(kind: ok) hello 1,
goodbye 0,
Original file line number Diff line number Diff line change
Expand Up @@ -12,3 +12,9 @@ function Foo(props) {
</fbt>
);
}

export const FIXTURE_ENTRYPOINT = {
fn: Foo,
params: [{value: 1}],
sequentialRenders: [{value: 1}, {value: 0}],
};
2 changes: 0 additions & 2 deletions compiler/packages/snap/src/SproutTodoFilter.ts
Original file line number Diff line number Diff line change
Expand Up @@ -440,7 +440,6 @@ const skipFilter = new Set([
'fbt/fbtparam-with-jsx-element-content',
'fbt/fbtparam-text-must-use-expression-container',
'fbt/fbtparam-with-jsx-fragment-value',
'fbt/fbt-preserve-jsxtext',
'todo.useContext-mutate-context-in-callback',
'loop-unused-let',
'reanimated-no-memo-arg',
Expand Down Expand Up @@ -485,7 +484,6 @@ const skipFilter = new Set([
'rules-of-hooks/rules-of-hooks-69521d94fa03',

// bugs
'fbt/bug-multiple-fbt-plural',
'fbt/bug-fbt-preserve-whitespace-param',
'bug-invalid-hoisting-functionexpr',
'original-reactive-scopes-fork/bug-nonmutating-capture-in-unsplittable-memo-block',
Expand Down