Ignore leading shebang lines in the lexer#191
Merged
Conversation
- Treat Unix hashbangs as comments at the start of source files - Add lexer and end-to-end regression coverage - Document direct script execution with shebangs
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughGocciaScript now accepts Unix shebang lines (e.g., Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
Benchmark Results274 benchmarks Interpreted: 🟢 53 improved · 🔴 203 regressed · 18 unchanged · avg -7.8% arraybuffer.js — Interp: 🟢 10, 🔴 3, 1 unch. · avg +3.6% · Bytecode: 🟢 14 · avg +11.7%
arrays.js — Interp: 🟢 14, 🔴 2, 3 unch. · avg +2.0% · Bytecode: 🟢 19 · avg +9.1%
async-await.js — Interp: 🟢 6 · avg +3.4% · Bytecode: 🟢 6 · avg +11.6%
classes.js — Interp: 🟢 14, 🔴 10, 7 unch. · avg +0.2% · Bytecode: 🟢 27, 4 unch. · avg +12.7%
closures.js — Interp: 🔴 11 · avg -4.9% · Bytecode: 🟢 10, 1 unch. · avg +8.1%
collections.js — Interp: 🔴 10, 2 unch. · avg -7.6% · Bytecode: 🟢 6, 🔴 5, 1 unch. · avg +0.9%
destructuring.js — Interp: 🔴 21, 1 unch. · avg -7.7% · Bytecode: 🟢 20, 2 unch. · avg +10.4%
fibonacci.js — Interp: 🔴 8 · avg -7.4% · Bytecode: 🟢 8 · avg +10.6%
for-of.js — Interp: 🔴 7 · avg -14.1% · Bytecode: 🟢 6, 1 unch. · avg +7.0%
helpers/bench-module.js — Interp: 0 · Bytecode: 0
iterators.js — Interp: 🔴 20 · avg -15.5% · Bytecode: 🟢 20 · avg +10.9%
json.js — Interp: 🟢 4, 🔴 13, 3 unch. · avg -4.2% · Bytecode: 🟢 20 · avg +11.7%
jsx.jsx — Interp: 🔴 21 · avg -12.3% · Bytecode: 🟢 21 · avg +5.7%
modules.js — Interp: 🔴 9 · avg -13.5% · Bytecode: 🟢 7, 🔴 1, 1 unch. · avg +11.5%
numbers.js — Interp: 🟢 2, 🔴 9 · avg -5.5% · Bytecode: 🟢 11 · avg +17.3%
objects.js — Interp: 🔴 7 · avg -16.0% · Bytecode: 🟢 3, 4 unch. · avg +2.9%
promises.js — Interp: 🔴 12 · avg -20.1% · Bytecode: 🟢 11, 1 unch. · avg +6.2%
regexp.js — Interp: 🔴 11 · avg -12.2% · Bytecode: 🟢 11 · avg +4.6%
strings.js — Interp: 🔴 11 · avg -15.6% · Bytecode: 🟢 8, 🔴 2, 1 unch. · avg +2.4%
typed-arrays.js — Interp: 🟢 3, 🔴 18, 1 unch. · avg -13.0% · Bytecode: 🟢 21, 1 unch. · avg +10.1%
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. |
Contributor
Suite Timing
Measured on ubuntu-latest x64. |
![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)
units/Goccia.Lexer.Test.pas (1)
20-29: Consider adding an explicit#13#10case for line tracking.Using
sLineBreakonly checks the current host newline. Adding a fixed CRLF assertion would keep this behavior locked even when PR checks run on a single OS.Suggested test addition
type TLexerTests = class(TTestSuite) private procedure TestIgnoresLeadingHashbang; procedure TestPreservesLineNumbersAfterHashbang; + procedure TestPreservesLineNumbersAfterHashbangCRLF; public procedure SetupTests; override; end; procedure TLexerTests.SetupTests; begin Test('Ignores leading hashbang', TestIgnoresLeadingHashbang); Test('Preserves line numbers after hashbang', TestPreservesLineNumbersAfterHashbang); + Test('Preserves line numbers after hashbang (CRLF)', TestPreservesLineNumbersAfterHashbangCRLF); end; +procedure TLexerTests.TestPreservesLineNumbersAfterHashbangCRLF; +var + Lexer: TGocciaLexer; + Tokens: TObjectList<TGocciaToken>; +begin + Lexer := TGocciaLexer.Create('#!/usr/bin/env goccia' + `#13`#10 + 'const value = 1;', '<test>'); + try + Tokens := Lexer.ScanTokens; + Expect<Integer>(Tokens[0].Line).ToBe(2); + Expect<Integer>(Tokens[0].Column).ToBe(1); + finally + Lexer.Free; + end; +end;Also applies to: 49-63
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@units/Goccia.Lexer.Test.pas` around lines 20 - 29, The tests currently only assert newline handling using sLineBreak which is platform-dependent; update the lexer line-tracking tests (specifically the TestPreservesLineNumbersAfterHashbang and related test procedures referenced in TLexerTests.SetupTests and the tests around lines 49-63) to include an explicit CRLF case (`#13`#10) in addition to sLineBreak so CI on different OSes still verifies CRLF behavior; modify the test bodies to feed an input containing "#!" followed by `#13`#10 and assert the same preserved line numbers as with sLineBreak.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@units/Goccia.Lexer.Test.pas`:
- Around line 20-29: The tests currently only assert newline handling using
sLineBreak which is platform-dependent; update the lexer line-tracking tests
(specifically the TestPreservesLineNumbersAfterHashbang and related test
procedures referenced in TLexerTests.SetupTests and the tests around lines
49-63) to include an explicit CRLF case (`#13`#10) in addition to sLineBreak so CI
on different OSes still verifies CRLF behavior; modify the test bodies to feed
an input containing "#!" followed by `#13`#10 and assert the same preserved line
numbers as with sLineBreak.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e68fbe60-c73c-40fd-af6c-2bba4285fc29
📒 Files selected for processing (5)
README.mddocs/build-system.mdtests/language/shebang.jsunits/Goccia.Lexer.Test.pasunits/Goccia.Lexer.pas
- Cover leading hashbang skipping with both LF and CRLF - Verify token line numbers stay correct after hashbangs
This was referenced Apr 12, 2026
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
#!...) before tokenization starts.Testing
tests/language/shebang.jsto verify scripts with a leading shebang execute normally.units/Goccia.Lexer.Test.pascoverage for skipping the hashbang and preserving line numbers.Summary by CodeRabbit
New Features
#!/usr/bin/env goccia) at the start of source files; they are ignored during parsing so scripts can be executable.Documentation
Tests