Skip unsupported generator methods during parsing#388
Merged
Conversation
- Add parser warnings and recovery for generator methods and function expressions - Expand unsupported-feature tests for object and class generators
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughParser now recognizes and skips generator methods ( Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
source/units/Goccia.Parser.pas (1)
1803-1834: Generator method skip logic in object literals looks good overall.The implementation correctly detects and skips generator methods (
*methodName() { ... }andasync *methodName() { ... }) in object literals by consuming the method name, generic parameters, parameter list, return type annotation, and body block.One minor edge case: "object properties allow reserved words" as method names (e.g.,
{ *if() { yield 1; } }). The current check at line 1818 handlesgttIdentifier,gttString, andgttNumber, but reserved words may be tokenized differently (e.g.,gttIf). This mirrors the pattern at lines 1880-1883 for regular properties, which includes an explicit list of reserved word tokens.Since this is an obscure edge case for already-unsupported syntax, and the parser will likely produce a syntax error (which is acceptable behavior for unsupported constructs), this can be addressed in a follow-up if needed.
♻️ Optional: Handle reserved words as generator method names
else if Check(gttIdentifier) or Check(gttString) or Check(gttNumber) then - Advance; + Advance + else if Match([gttIf, gttElse, gttConst, gttLet, gttClass, gttEnum, gttExtends, gttNew, gttThis, gttSuper, gttStatic, + gttReturn, gttFor, gttWhile, gttDo, gttSwitch, gttCase, gttDefault, gttBreak, + gttThrow, gttTry, gttCatch, gttFinally, gttImport, gttExport, gttFrom, gttAs, + gttTrue, gttFalse, gttNull, gttTypeof, gttInstanceof, gttIn, gttDelete, gttVar, gttWith]) then + ; // Reserved word method name consumed by Match🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/units/Goccia.Parser.pas` around lines 1803 - 1834, The generator-method branch (triggered by Check(gttStar) in the object-literal parsing logic) currently accepts only Check(gttIdentifier), Check(gttString) or Check(gttNumber) for the method name; extend that condition to also accept reserved-word tokens (e.g., gttIf, gttFor, gttReturn, etc.) the same way regular property parsing does (see the pattern used around the other property-handling code), so reserved words used as generator method names are skipped consistently; update the Check(...) list after the Advance that skips the star to include the same reserved token set as the non-generator property handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@source/units/Goccia.Parser.pas`:
- Around line 1803-1834: The generator-method branch (triggered by
Check(gttStar) in the object-literal parsing logic) currently accepts only
Check(gttIdentifier), Check(gttString) or Check(gttNumber) for the method name;
extend that condition to also accept reserved-word tokens (e.g., gttIf, gttFor,
gttReturn, etc.) the same way regular property parsing does (see the pattern
used around the other property-handling code), so reserved words used as
generator method names are skipped consistently; update the Check(...) list
after the Advance that skips the star to include the same reserved token set as
the non-generator property handling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a6d53383-cc5a-4461-b9e4-9fea6e2af00d
📒 Files selected for processing (2)
source/units/Goccia.Parser.pastests/language/statements/unsupported-features.js
Contributor
Suite Timing
Measured on ubuntu-latest x64. |
Contributor
Benchmark Results386 benchmarks Interpreted: 🟢 20 improved · 🔴 289 regressed · 77 unchanged · avg -3.8% arraybuffer.js — Interp: 🔴 11, 3 unch. · avg -5.0% · Bytecode: 🟢 6, 8 unch. · avg +3.3%
arrays.js — Interp: 🔴 17, 2 unch. · avg -7.1% · Bytecode: 🟢 5, 14 unch. · avg +1.8%
async-await.js — Interp: 🔴 4, 2 unch. · avg -7.2% · Bytecode: 6 unch. · avg +1.2%
base64.js — Interp: 🔴 9, 1 unch. · avg -5.8% · Bytecode: 🔴 1, 9 unch. · avg +0.3%
classes.js — Interp: 🔴 21, 10 unch. · avg -5.0% · Bytecode: 🟢 6, 🔴 2, 23 unch. · avg +1.0%
closures.js — Interp: 🔴 11 · avg -7.0% · Bytecode: 🟢 1, 🔴 1, 9 unch. · avg +0.7%
collections.js — Interp: 🔴 11, 1 unch. · avg -7.8% · Bytecode: 🟢 2, 🔴 1, 9 unch. · avg +0.5%
csv.js — Interp: 🔴 12, 1 unch. · avg -7.3% · Bytecode: 🟢 1, 🔴 2, 10 unch. · avg +0.5%
destructuring.js — Interp: 🔴 20, 2 unch. · avg -7.0% · Bytecode: 🟢 3, 🔴 3, 16 unch. · avg +1.3%
fibonacci.js — Interp: 🔴 8 · avg -7.5% · Bytecode: 🟢 1, 🔴 4, 3 unch. · avg -0.2%
float16array.js — Interp: 🟢 4, 🔴 21, 7 unch. · avg -1.8% · Bytecode: 🟢 3, 🔴 10, 19 unch. · avg -2.3%
for-of.js — Interp: 🔴 4, 3 unch. · avg -3.1% · Bytecode: 🟢 1, 🔴 1, 5 unch. · avg +0.7%
helpers/bench-module.js — Interp: 0 · Bytecode: 0
iterators.js — Interp: 🔴 38, 4 unch. · avg -6.8% · Bytecode: 🟢 34, 8 unch. · avg +4.6%
json.js — Interp: 🔴 19, 1 unch. · avg -7.8% · Bytecode: 🟢 3, 🔴 9, 8 unch. · avg -2.1%
jsx.jsx — Interp: 🟢 1, 🔴 10, 10 unch. · avg -2.2% · Bytecode: 21 unch. · avg +0.3%
modules.js — Interp: 🔴 9 · avg -8.5% · Bytecode: 9 unch. · avg -0.1%
numbers.js — Interp: 🔴 7, 4 unch. · avg -4.1% · Bytecode: 🟢 3, 🔴 2, 6 unch. · avg -0.4%
objects.js — Interp: 🔴 7 · avg -9.1% · Bytecode: 🔴 2, 5 unch. · avg -1.4%
promises.js — Interp: 🟢 1, 🔴 4, 7 unch. · avg -2.2% · Bytecode: 🟢 1, 🔴 6, 5 unch. · avg -2.1%
regexp.js — Interp: 🔴 10, 1 unch. · avg -7.8% · Bytecode: 🔴 8, 3 unch. · avg -2.5%
strings.js — Interp: 🟢 4, 🔴 5, 10 unch. · avg +9.0% · Bytecode: 🟢 10, 9 unch. · avg +2.9%
tsv.js — Interp: 🟢 2, 🔴 4, 3 unch. · avg -3.1% · Bytecode: 🟢 4, 5 unch. · avg +1.7%
typed-arrays.js — Interp: 🟢 1, 🔴 18, 3 unch. · avg -7.1% · Bytecode: 🟢 4, 🔴 13, 5 unch. · avg -3.8%
uint8array-encoding.js — Interp: 🟢 7, 🔴 9, 2 unch. · avg +19.9% · Bytecode: 🟢 2, 🔴 3, 13 unch. · avg -0.4%
Measured on ubuntu-latest x64. Benchmark ranges compare cached main-branch min/max ops/sec with the PR run; overlapping ranges are treated as unchanged noise. Percentage deltas are secondary context. |
- Skip generator method names that use reserved words - Add regression coverage for object literal generator methods
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
functiondeclarations so they are skipped consistently during parsing.