fix(parse/js): fix a case where valid js was being interpretted as ts#9550
fix(parse/js): fix a case where valid js was being interpretted as ts#9550
Conversation
🦋 Changeset detectedLatest commit: 67ff502 The changes in this PR will be included in the next version bump. This PR includes changesets to release 13 packages
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 |
github-actions
bot
added
A-Parser
Parser conformance results onjs/262
jsx/babel
markdown/commonmark
symbols/microsoft
ts/babel
🔥 Regression (1):ts/microsoft
🔥 Regression (1): |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (5)
📒 Files selected for processing (8)
✅ Files skipped from review due to trivial changes (5)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThe parser was updated to correctly parse conditional expressions whose consequent is an arrow function returning a parenthesised object. An Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Comment Tip You can get early access to new features in CodeRabbit.Enable the |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@crates/biome_js_parser/tests/js_test_suite/ok/conditional_arrow_return_type_in_consequent.ts`:
- Around line 1-3: Update the test fixture so it hits the guard in function.rs
that only fires when the parameter list starts with `{` or `[` by adding a
destructured-parameter variant of the arrow function; specifically, in the
conditional_arrow_return_type_in_consequent.ts fixture where you currently have
the slotFn ternary with a typed simple parameter, replace or add a branch that
uses an object- or array-destructured parameter with the same return type
annotation (e.g., a parameter like an object destructuring rather than `i`) so
the parser path in crates/biome_js_parser/src/syntax/function.rs (the guard
around return type annotation parsing) is exercised.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 20c65c2e-57b3-4d9c-b0de-78beb845ede9
⛔ Files ignored due to path filters (2)
crates/biome_js_parser/tests/js_test_suite/ok/conditional_arrow_object_body_in_consequent.js.snapis excluded by!**/*.snapand included by**crates/biome_js_parser/tests/js_test_suite/ok/conditional_arrow_return_type_in_consequent.ts.snapis excluded by!**/*.snapand included by**
📒 Files selected for processing (4)
.changeset/fix-js-parser-9548.mdcrates/biome_js_parser/src/syntax/function.rscrates/biome_js_parser/tests/js_test_suite/ok/conditional_arrow_object_body_in_consequent.jscrates/biome_js_parser/tests/js_test_suite/ok/conditional_arrow_return_type_in_consequent.ts
| const slotFn = isFirstMount | ||
| ? ((i): Slot => ({ [CONTENT_SLOT]: i })) | ||
| : i => wrapSlotExpr(newExprs[i]); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's check the fixture file
cat -n crates/biome_js_parser/tests/js_test_suite/ok/conditional_arrow_return_type_in_consequent.tsRepository: biomejs/biome
Length of output: 184
🏁 Script executed:
# Now let's look at the changed parser code around lines 591-605
sed -n '580,620p' crates/biome_js_parser/src/syntax/function.rs | cat -nRepository: biomejs/biome
Length of output: 1537
🏁 Script executed:
# Let's get more context around the Ambiguity enum and the speculative parsing logic
rg "enum Ambiguity" -A 10 crates/biome_js_parser/src/Repository: biomejs/biome
Length of output: 944
🏁 Script executed:
# Search for other test fixtures with destructured parameters and return types
fd "conditional_arrow.*\.ts$" crates/biome_js_parser/tests/ | head -20Repository: biomejs/biome
Length of output: 149
🏁 Script executed:
# Let's also check if there are any existing tests with destructured params and TypeScript return types
rg '\{\s*\[' crates/biome_js_parser/tests/js_test_suite/ok/ --type ts -B2 -A2 | head -50Repository: biomejs/biome
Length of output: 2617
🏁 Script executed:
# Let's look at the broader context of the changed code
sed -n '550,650p' crates/biome_js_parser/src/syntax/function.rs | cat -nRepository: biomejs/biome
Length of output: 4129
🏁 Script executed:
# Search for what PR or commit made this change
git log --oneline -20 -- crates/biome_js_parser/src/syntax/function.rsRepository: biomejs/biome
Length of output: 136
🏁 Script executed:
# Check if there are existing test fixtures that test destructured arrow parameters with return types
rg 'const.*?(\{.*?\}):.*?=>' crates/biome_js_parser/tests/js_test_suite/ok/ --type ts -A1 | head -40Repository: biomejs/biome
Length of output: 39
🏁 Script executed:
# Look for has_ambiguous_destructured_parameters to understand when it's set
rg 'has_ambiguous_destructured_parameters' crates/biome_js_parser/src/ -B3 -A3Repository: biomejs/biome
Length of output: 1146
🏁 Script executed:
# Let's see the actual commit that made this change
git show a0f44283e --statRepository: biomejs/biome
Length of output: 50371
🏁 Script executed:
# Look at the full diff of that commit
git show a0f44283eRepository: biomejs/biome
Length of output: 50377
🏁 Script executed:
# Let's look at the exact condition and understand when it fires
sed -n '570,620p' crates/biome_js_parser/src/syntax/function.rsRepository: biomejs/biome
Length of output: 1713
🏁 Script executed:
# Let's check what has_ambiguous_destructured_parameters checks
sed -n '573,576p' crates/biome_js_parser/src/syntax/function.rsRepository: biomejs/biome
Length of output: 309
🏁 Script executed:
# Let's create a simple test to understand the parser behavior
cat > /tmp/test_parser.ts << 'EOF'
// Current fixture - unambiguous parameter
const slotFn = isFirstMount
? ((i): Slot => ({ [CONTENT_SLOT]: i }))
: i => wrapSlotExpr(newExprs[i]);
// Suggested fixture - ambiguous parameter with return type
const slotFn2 = isFirstMount
? ({ [CONTENT_SLOT]: i }): Slot => ({ [CONTENT_SLOT]: i })
: i => wrapSlotExpr(newExprs[i]);
EOF
cat /tmp/test_parser.tsRepository: biomejs/biome
Length of output: 393
🏁 Script executed:
# Look for similar test patterns in the codebase
find crates/biome_js_parser/tests/js_test_suite/ok -name "*.ts" -type f -exec grep -l "}: .* =>" {} \; | head -5Repository: biomejs/biome
Length of output: 39
🏁 Script executed:
# Check if there are existing arrow function tests with destructured params and return types
rg '{\s*\[.*\].*}:\s*\w+\s*=>' crates/biome_js_parser/tests/ -A2 -B2 | head -40Repository: biomejs/biome
Length of output: 157
🏁 Script executed:
# Let's verify the fixture works by checking if there are similar patterns nearby
rg 'Slot\s*=>' crates/biome_js_parser/tests/ -B2 -A2 | head -30Repository: biomejs/biome
Length of output: 1051
🏁 Script executed:
# Let's trace through the logic manually - check what the second token would be
# for the current fixture
cat > /tmp/analyze.txt << 'EOF'
Current fixture: ((i): Slot => ...)
After opening (: [
Token 0: i
Token 1: )
has_ambiguous_destructured_parameters = matches!(p.nth(1), T!['['] | T!['{'])
= matches!(T![')'], T!['['] | T!['{'])
= FALSE
So the new guard condition:
if !(ambiguity.is_disallowed()
&& has_ambiguous_destructured_parameters ← FALSE, so whole condition becomes FALSE
&& !TypeScript.is_supported(p)
&& p.at(T![:]))
Since has_ambiguous_destructured_parameters is FALSE, the negation becomes TRUE,
so the return type annotation WILL be parsed (the guard is NOT triggered).
---
Suggested fixture: ({ [CONTENT_SLOT]: i }): Slot => ...
After opening (: [
Token 0: {
Token 1: [CONTENT_SLOT] or space
has_ambiguous_destructured_parameters = matches!(p.nth(1), T!['['] | T!['{'])
= matches!(T!['{'], T!['['] | T!['{'])
= TRUE
So the new guard condition becomes:
if !(ambiguity.is_disallowed()
&& TRUE
&& !TypeScript.is_supported(p) ← TRUE in JS mode
&& p.at(T![:])) ← TRUE, we're about to see ':'
If ALL conditions are TRUE, the negation makes the whole if FALSE,
so we SKIP the return type annotation parsing (the guard IS triggered).
EOF
cat /tmp/analyze.txtRepository: biomejs/biome
Length of output: 1243
This fixture misses the branch you just changed.
The guard that skips return type annotation parsing in crates/biome_js_parser/src/syntax/function.rs (lines 591–605) only fires when the parameter list starts with { or [. The current fixture ((i): Slot => ...) has a simple parameter, not destructuring, so it doesn't exercise this branch. Add a destructured parameter case:
Suggested fixture
const slotFn = isFirstMount
- ? ((i): Slot => ({ [CONTENT_SLOT]: i }))
+ ? ({ [CONTENT_SLOT]: i }): Slot => ({ [CONTENT_SLOT]: i })
: i => wrapSlotExpr(newExprs[i]);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const slotFn = isFirstMount | |
| ? ((i): Slot => ({ [CONTENT_SLOT]: i })) | |
| : i => wrapSlotExpr(newExprs[i]); | |
| const slotFn = isFirstMount | |
| ? ({ [CONTENT_SLOT]: i }): Slot => ({ [CONTENT_SLOT]: i }) | |
| : i => wrapSlotExpr(newExprs[i]); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@crates/biome_js_parser/tests/js_test_suite/ok/conditional_arrow_return_type_in_consequent.ts`
around lines 1 - 3, Update the test fixture so it hits the guard in function.rs
that only fires when the parameter list starts with `{` or `[` by adding a
destructured-parameter variant of the arrow function; specifically, in the
conditional_arrow_return_type_in_consequent.ts fixture where you currently have
the slotFn ternary with a typed simple parameter, replace or add a branch that
uses an object- or array-destructured parameter with the same return type
annotation (e.g., a parameter like an object destructuring rather than `i`) so
the parser path in crates/biome_js_parser/src/syntax/function.rs (the guard
around return type annotation parsing) is exercised.
Merging this PR will not alter performance
Comparing 
Footnotes
|
crates/biome_js_parser/tests/js_test_suite/ok/conditional_arrow_return_type_in_consequent.ts
Show resolved
Hide resolved
dyc3
force-pushed
the
dyc3/fix-js-as-ts-missparse
branch
from
a0f4428 to
5caccc6
Compare
ematipico
left a comment
ematipico
left a comment
There was a problem hiding this comment.
Ops, it seems we regressed something.
Maybe we should look at what regressed and feed it to the agent
dyc3
force-pushed
the
dyc3/fix-js-as-ts-missparse
branch
from
5caccc6 to
67ff502
Compare
|
We still regressed, we shouldn't have merged this one |
2 participants
Summary
I used gpt 5.4 to find the bug and fix it. I can post the agent's rationale if needed, because the fix looks a bit odd at a glance.
fixes #9548
Test Plan
snapshots
Docs