fix(formatjs): avoid hygiene crashes#625
Conversation
🦋 Changeset detectedLatest commit: 833fac9 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Review of #625 —
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6045e153f1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| Expr::Ident(ident) => { | ||
| let expr = self.bindings.get(&ident.to_id())?; | ||
| self.evaluate_static_string_with_depth(expr, depth + 1) |
There was a problem hiding this comment.
When a descriptor value is an identifier, this new resolver only looks in self.bindings, which is populated as the visitor reaches declarations. Because visit_mut_module_items still descends into function bodies before later module items, valid code like function C() { return <FormattedMessage defaultMessage={MSG} /> } const MSG = "Hello"; now sees MSG as missing, emits the static-evaluation error, and skips the transform/extraction; the previous module-wide Evaluator was not limited by traversal order.
Useful? React with 👍 / 👎.
| } | ||
|
|
||
| match Self::strip_expr(expr) { | ||
| Expr::Lit(Lit::Str(s)) => Some(s.value.as_str().expect("non-utf8 string").to_string()), |
There was a problem hiding this comment.
For statically constant messages that include non-string primitives, such as defaultMessage={\Step ${2}`}ordefaultMessage={"Step " + 2}, the replacement evaluator now returns Nonebecause literal handling only acceptsLit::Str`. The previous SWC evaluator evaluated these JavaScript constants with string coercion, so these valid messages now emit the static-evaluation error and are left unextracted.
Useful? React with 👍 / 👎.
| let expr = self.bindings.get(&ident.to_id())?; | ||
| self.evaluate_static_string_with_depth(expr, depth + 1) | ||
| } | ||
| _ => None, |
There was a problem hiding this comment.
Static messages that read from object literals, for example const m = { hello: "Hello" }; formatMessage({ defaultMessage: m.hello }), now fall into this catch-all and are treated as non-evaluable because the replacement evaluator only follows bare identifiers. The old Evaluator::eval handled descriptor expressions directly, so these constant member lookups were extractable before but now emit the static-evaluation error and skip extraction.
Useful? React with 👍 / 👎.
Code ReviewNice fix for the hygiene crash — the redesign (drop the heavyweight Correctness
Code quality
Tests
API / Backward compat
Performance / Security
Overall, this is a solid fix. The biggest open question is whether the narrowing of evaluation scope (no destructuring, no imports, no function-scope hoisting, only |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9eeca1c46c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| Prop::Shorthand(ident) => { | ||
| let key = ident.sym.to_string(); | ||
| let ident_expr = Expr::Ident(ident.clone()); | ||
|
|
||
| (key, visitor.get_message_descriptor_value(&ident_expr)) |
There was a problem hiding this comment.
When a descriptor uses ES shorthand, e.g. const defaultMessage = "Hi"; defineMessage({ defaultMessage }) with removeDefaultMessage: true, this new branch now treats the shorthand as an extractable descriptor. However, the later mutation/removal pass only handles Prop::KeyValue entries for defaultMessage, so the transformed output still contains the shorthand defaultMessage, violating the removeDefaultMessage behavior for the newly supported syntax. Either rewrite/remove shorthand descriptor properties too, or avoid extracting them as descriptors.
Useful? React with 👍 / 👎.
| for prop in obj.props.iter().rev() { | ||
| let PropOrSpread::Prop(prop) = prop else { | ||
| continue; | ||
| }; |
There was a problem hiding this comment.
When resolving a member from an object literal, this loop scans properties in reverse but simply skips spreads. For const messages = { hello: "A", ...runtimeMessages }; formatMessage({ defaultMessage: messages.hello }), the spread occurs after hello and can override it at runtime, but the resolver ignores the spread and returns the earlier literal, causing extraction and ID generation for a message that may not be the runtime default. If a spread is encountered before finding the key, the lookup should be treated as non-static unless the spread itself is statically resolved.
Useful? React with 👍 / 👎.
| _ => Some(StaticValue::Str(format!( | ||
| "{}{}", | ||
| left.to_js_string(), | ||
| right.to_js_string() | ||
| ))), |
There was a problem hiding this comment.
This fallback treats every + with at least one non-number as string concatenation, but JavaScript only concatenates when either operand becomes a string; otherwise booleans/null are coerced numerically. For example, defaultMessage={"Count " + (1 + true)} should statically evaluate to "Count 2", while this resolver produces "Count 1true", changing the extracted message and generated ID for valid static descriptors that the previous evaluator handled with JS semantics.
Useful? React with 👍 / 👎.
| Prop::Shorthand(ident) => { | ||
| let key = ident.sym.to_string(); | ||
| let ident_expr = Expr::Ident(ident.clone()); | ||
|
|
||
| (key, visitor.get_message_descriptor_value(&ident_expr)) |
There was a problem hiding this comment.
This new shorthand branch resolves the identifier before checking whether the property is a message descriptor key. As a result, defineMessage({ defaultMessage: "Hi", metadata }) now emits the static-evaluation error when metadata is dynamic or unbound, even though non-descriptor fields should be ignored and shorthand properties were previously skipped entirely. Move the key allow-list check ahead of get_message_descriptor_value for shorthand properties.
Useful? React with 👍 / 👎.
Code review for PR #625 — fix(formatjs): avoid hygiene crashesNice, focused fix. Removing the Below are some observations from the diff. Most are nits; a couple are worth a second look. Correctness
Performance / memory
Backward compatibility / API
Tests
Nit
Overall: this is a good fix that addresses the real bug and makes the plugin meaningfully leaner. Approving in spirit; the items above are mostly polish or future cleanup, with #1, #3, and #6 being the ones I'd think hardest about. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cb7fa5af24
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| match Self::strip_expr(expr) { | ||
| Expr::Lit(Lit::Str(s)) => Some(StaticValue::Str(s.value.as_str()?.to_string())), | ||
| Expr::Lit(Lit::Num(n)) => Some(StaticValue::Num(n.value)), | ||
| Expr::Lit(Lit::Bool(b)) => Some(StaticValue::Bool(b.value)), | ||
| Expr::Lit(Lit::Null(_)) => Some(StaticValue::Null), | ||
| Expr::Lit(Lit::BigInt(n)) => Some(StaticValue::BigInt(n.value.to_string())), | ||
| Expr::Tpl(tpl) => self |
There was a problem hiding this comment.
When a message contains a negative numeric constant, e.g. defaultMessage={`Step ${-1}`} or defaultMessage: "Step " + -1, SWC represents the -1 as an Expr::Unary, but the replacement evaluator only accepts literal numbers and falls through to None. The previous Evaluator::eval handled these JavaScript constants, so these valid static descriptors now emit the non-evaluable error and are skipped instead of being extracted.
Useful? React with 👍 / 👎.
| let expr = self.resolve_member_expr_value(member, depth + 1)?; | ||
| self.evaluate_static_value_with_depth(&expr, depth + 1) | ||
| } | ||
| _ => None, |
There was a problem hiding this comment.
When a descriptor uses a statically decidable conditional, such as defaultMessage: true ? "Enabled" : "Disabled", this evaluator reaches the catch-all because it has no Expr::Cond handling. The previous SWC evaluator could fold constant conditionals, so these valid static descriptors now emit the non-evaluable error and are skipped instead of being transformed/extracted.
Useful? React with 👍 / 👎.
| } else if value == f64::NEG_INFINITY { | ||
| "-Infinity".to_string() | ||
| } else { | ||
| value.to_string() |
There was a problem hiding this comment.
When numeric values are coerced inside a template or string concatenation, f64::to_string() does not match JavaScript String(number) for values outside JS's fixed-format range, e.g. defaultMessage={`ID ${1e21}`} should extract ID 1e+21 but this produces the expanded decimal form. That changes the extracted default message and generated ID compared with the previous JS-semantics evaluator for otherwise static descriptors.
Useful? React with 👍 / 👎.
| if self.prop_name_to_key(&kv.key, depth + 1).as_deref() == Some(key.as_str()) { | ||
| return Some(kv.value.clone()); | ||
| } |
There was a problem hiding this comment.
When resolving a member from an object literal, an unresolvable computed property to the right of the target can still override it at runtime, e.g. const messages = { hello: "A", [runtimeKey]: "B" }; formatMessage({ defaultMessage: messages.hello }) should not be treated as static if runtimeKey might be "hello". Here prop_name_to_key() returning None is just considered a non-match and the scan continues to the earlier literal, causing extraction and ID generation for a message that may not be the runtime default.
Useful? React with 👍 / 👎.
Code ReviewThanks for tackling #604 — replacing the heavyweight Correctness
Minor
Performance
TestsCoverage is strong — particularly the cyclic-binding case and the spread/computed-key invalidation tests. Suggestions for filling small gaps:
Nits
Overall this is a solid, well-tested fix. The points above are mostly about clarifying the new semantic surface (what does and doesn't resolve) and the API-compat story for the deprecated entry point. Nice work landing it. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 80ea19072a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ) { | ||
| (true, false) => false, | ||
| _ => true, |
There was a problem hiding this comment.
When a binding starts as an object literal and is reassigned to a non-object before it is used, this branch preserves the stale object instead of updating or invalidating the binding. For example, let messages = { hello: "A" }; messages = getMessages(); formatMessage({ defaultMessage: messages.hello }) will still extract/hash "A", even though the runtime value is dynamic (and may not even be an object). The special React-compiler case needs to be narrower, or non-object reassignments should make later member resolution fail.
Useful? React with 👍 / 👎.
| for item in items.iter() { | ||
| self.collect_module_item_bindings(item); |
There was a problem hiding this comment.
Pre-collecting every module-level var binding before visiting statements lets later valid var initializers overwrite earlier uses. In var MSG = "A"; formatMessage({ defaultMessage: MSG }); var MSG = "B";, both declarations share the same binding, so the prepass leaves MSG as "B" before the formatMessage call is processed, causing extraction/ID generation for the wrong message even though runtime uses "A" at that point.
Useful? React with 👍 / 👎.
| if let AssignTarget::Simple(SimpleAssignTarget::Ident(ident)) = &assign_expr.left { | ||
| let variable_id = ident.id.to_id(); | ||
|
|
||
| // Check if we already have a binding for this variable | ||
| let should_update = match self.resolve_identifier(ident) { | ||
| Some(existing_expr) => { | ||
| // Only overwrite if the new expression is an object literal | ||
| // and the existing one is not, or if both are object literals | ||
| match (existing_expr, &*assign_expr.right) { | ||
| (Expr::Object(_), Expr::Object(_)) => true, // Both objects, update | ||
| (_, Expr::Object(_)) => true, /* New is object, existing */ | ||
| // is not, update | ||
| (Expr::Object(_), _) => false, /* Existing is object, new is not, don't */ | ||
| // update | ||
| _ => true, // Neither is object, update | ||
| } | ||
| } | ||
| None => true, // No existing binding, always update | ||
| }; | ||
|
|
||
| if should_update { | ||
| self.evaluator.store(variable_id, &assign_expr.right); | ||
| } | ||
| self.store_binding(ident.id.to_id(), &assign_expr.right); |
There was a problem hiding this comment.
Because the new resolver folds member reads out of cached object literals, ignoring member assignments leaves stale values in bindings. For example, after const messages = { hello: "A" }; messages.hello = getRuntimeMessage(); formatMessage({ defaultMessage: messages.hello }), this branch does nothing for the messages.hello = ... write, so the later lookup still extracts and hashes "A" even though the runtime message is dynamic or different.
Useful? React with 👍 / 👎.
| fn visit_mut_var_declarator(&mut self, declarator: &mut VarDeclarator) { | ||
| declarator.visit_mut_children_with(self); | ||
|
|
||
| if let Pat::Ident(ident) = &declarator.name { | ||
| if let Some(init) = &declarator.init { | ||
| self.store_binding(ident.id.to_id(), init); |
There was a problem hiding this comment.
The module prepass fixes later top-level constants, but function/block scopes still rely on normal traversal because visit_mut_var_declarator stores a binding only after visiting the initializer's children. In valid code such as const render = () => <FormattedMessage defaultMessage={MSG} />; const MSG = "Hi"; return render();, the arrow body is visited before MSG is stored, so extraction emits the non-evaluable error even though the binding is initialized before the closure runs. This is fresh evidence beyond the earlier top-level later-constant issue: only visit_mut_module_items precollects declarations, not nested bodies.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 833fac9f96
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if let Some(id) = Self::member_base_ident(member) { | ||
| self.invalidate_binding(id); |
There was a problem hiding this comment.
When an object binding has been aliased, invalidating only the syntactic base of the member write leaves other bindings pointing at the same object stale. For example, const messages = { hello: "A" }; const alias = messages; alias.hello = getMessage(); formatMessage({ defaultMessage: messages.hello }) still resolves messages.hello from the cached object and extracts/hashes "A", even though the runtime value was mutated through alias. The new member-resolution cache should either invalidate aliases that resolve to the same object or stop treating aliased object member reads as static after a member write.
Useful? React with 👍 / 👎.
ReviewNice fix for the hygiene crash — replacing What I like
Issues and suggestions1. Duplicated descriptor-key list. 2. Code duplication between 3. 4. Behavior change worth flagging in the changeset. The old 5. Conditional/branch assignments give arbitrary results. With 6. 7. 8. Error span quality. 9. Minor: Correctness spot-checks I did
Security / performance
Overall: 👍 ready to merge once the changeset is updated to call out the assign-invalidation behavior change. Items 1–3 and 9 are nice-to-haves that could be folded in here or as a follow-up. |
1 participant
Closes #604
Summary
swc_ecma_minifier::Evaluatorand cloning the moduleformatMessagecalls withast: trueTests
cargo check -p swc_formatjs_transform -p swc_plugin_formatjspnpm -F @swc/plugin-formatjs testpnpm exec taplo format --check packages/formatjs/Cargo.tomlpnpm exec prettier --check packages/formatjs/__tests__/wasm.test.tsnext-swc-formatjs-reactcompiler-bugsimplerbranch, local packed@swc/plugin-formatjs,npm run buildNotes
pnpm -F @swc/plugin-formatjs teststill prints the pre-existing Vitest warning about an unawaited rejection assertion in an older test, but all 28 tests pass.