Skip to content

refactor(parser): Avoid peek in parse_delimited_list#11343

Merged
graphite-app[bot] merged 1 commit intomainfrom
r/delimited-list
May 28, 2025
Merged

refactor(parser): Avoid peek in parse_delimited_list#11343
graphite-app[bot] merged 1 commit intomainfrom
r/delimited-list

Conversation

@leaysgur
Copy link
Member

@leaysgur leaysgur commented May 28, 2025

Out of curiosity, I've just deleted the peeking in parse_delimited_list.

Then, coverage errors show me there are only a few places missing error report for invalid trailing comma after rest element, etc.

So I added those checks manually.

In TSC, these checks seem to be done at checker, not parser. But we need this in parser for JS.


(Sorry, I have PASSED the test, but I am not very confident that this change is intrinsically sound.)

@graphite-app
Copy link
Contributor

graphite-app bot commented May 28, 2025

How to use the Graphite Merge Queue

Add either label to this PR to merge it via the merge queue:

  • 0-merge - adds this PR to the back of the merge queue
  • hotfix - for urgent hot fixes, skip the queue and merge this PR next

You must have a Graphite account in order to use the merge queue. Sign up using this link.

An organization admin has enabled the Graphite Merge Queue in this repository.

Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.

@github-actions github-actions bot added A-parser Area - Parser C-cleanup Category - technical debt or refactoring. Solution not expected to change behavior labels May 28, 2025
@codspeed-hq
Copy link

codspeed-hq bot commented May 28, 2025

CodSpeed Instrumentation Performance Report

Merging #11343 will improve performances by 14.65%

Comparing r/delimited-list (069b843) with main (3249ab6)

Summary

⚡ 1 improvements
✅ 37 untouched benchmarks

Benchmarks breakdown

Benchmark BASE HEAD Change
mangler[cal.com.tsx] 3.5 ms 3 ms +14.65%

@leaysgur leaysgur force-pushed the r/delimited-list branch from 44ee2be to 8edef9f Compare May 28, 2025 07:11
@leaysgur
Copy link
Member Author

leaysgur commented May 28, 2025

At this point, there appears to be no regression in the parser benchmark.

But strangely, when I added a commit that removes the unused Lexer::lookahead related things, the regression occurs!
(Please see edit history of codespeed comment)

...🤔

@leaysgur leaysgur marked this pull request as ready for review May 28, 2025 08:15
Copilot AI review requested due to automatic review settings May 28, 2025 08:15
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

Refactors parse_delimited_list to return a tuple with an optional trailing-separator span, and adds parser-level checks for invalid trailing commas in specific constructs.

  • Changed all parse_delimited_list invocations to the new signature (Vec, Option<u32>)
  • Added explicit error reporting for trailing commas in index signatures and parenthesized expressions
  • Updated context wrappers to destructure the new tuple return

Reviewed Changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
crates/oxc_parser/src/ts/types.rs Switched to tuple return from parse_delimited_list and added trailing-comma error for index signatures
crates/oxc_parser/src/ts/statement.rs Adapted enum-body parsing to the new parse_delimited_list signature
crates/oxc_parser/src/js/object.rs Updated object-expression parsing to destructure the new return
crates/oxc_parser/src/js/module.rs Simplified import/export specifier parsing with the new signature
crates/oxc_parser/src/js/expression.rs Hooked into trailing-comma span for parentheses, arrays, and calls
crates/oxc_parser/src/cursor.rs Changed parse_delimited_list to return trailing-separator info and marked unused peek_at
Comments suppressed due to low confidence (2)

crates/oxc_parser/src/ts/types.rs:1233

  • There should be a unit test covering the new trailing-comma error path in index signatures to ensure this diagnostic fires when a comma precedes ].
if let Some(comma_span) = comma_span {

crates/oxc_parser/src/js/expression.rs:212

  • Add tests to validate that a trailing comma inside parentheses triggers the fatal_error path and correctly reports the unexpected comma.
if let Some(comma_span) = comma_span {

@Boshen Boshen added the 0-merge Merge with Graphite Merge Queue label May 28, 2025
Copy link
Member

Boshen commented May 28, 2025

Merge activity

Out of curiosity, I've just deleted the peeking in `parse_delimited_list`.

Then, coverage errors show me there are only a few places missing error report for invalid trailing comma after rest element, etc.

So I added those checks manually.

In TSC, these checks seem to be done at checker, not parser. But we need this in parser for JS.

---

(Sorry, I have PASSED the test, but I am not very confident that this change is intrinsically sound.)
@graphite-app graphite-app bot force-pushed the r/delimited-list branch from 8edef9f to 069b843 Compare May 28, 2025 08:30
@graphite-app graphite-app bot merged commit 069b843 into main May 28, 2025
25 checks passed
@graphite-app graphite-app bot deleted the r/delimited-list branch May 28, 2025 08:37
@graphite-app graphite-app bot removed the 0-merge Merge with Graphite Merge Queue label May 28, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-parser Area - Parser C-cleanup Category - technical debt or refactoring. Solution not expected to change behavior

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants