Warning when using unsupported features#15
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughThis pull request refactors the keyword system into reserved and contextual modules, introduces parser warnings for unsupported language constructs (loops, imports/exports, with statement), adds engine-global Changes
Sequence Diagram(s)sequenceDiagram
participant Parser as GocciaParser
participant Engine as TGocciaEngine
participant Scope as GlobalScope
participant Output as Stdout
Parser->>Parser: ParseScript (e.g., for loop)
alt Unsupported Construct
Parser->>Parser: AddWarning(message, suggestion, line, col)
Parser->>Parser: Create no-op statement
end
Engine->>Parser: Execute code
Parser->>Engine: Return AST + Warnings
Engine->>Engine: RegisterBuiltinConstructors()
Engine->>Engine: RegisterGlobalThis()
Note over Engine: Collect all global bindings<br/>into globalThis object
Scope->>Scope: Bind globalThis as const
Engine->>Engine: RegisterGocciaScriptGlobal()
Note over Engine: Create GocciaScript with<br/>version, commit, builtIns
Scope->>Scope: Bind GocciaScript as const
Engine->>Engine: PrintParserWarnings(Parser)
loop For each warning
Engine->>Output: Print warning message, suggestion, location
end
Engine->>Output: Execution complete
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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 `@units/Goccia.Parser.pas`:
- Around line 161-166: TGocciaParser.GetWarnings uses Move to copy FWarnings
into Result which is unsafe because TGocciaParserWarning contains managed string
fields; replace the raw memory copy with an element-by-element assignment loop
so reference counts are updated correctly (keep SetLength(Result, FWarningCount)
and then for each index assign Result[i] := FWarnings[i]), referring to
TGocciaParser.GetWarnings, FWarnings and FWarningCount when updating the code.
Contributor
Benchmark Results113 benchmarks · 🔴 1 regressed · 112 unchanged · avg -1.5% arrays.js — 11 unchanged · avg -0.1%
classes.js — 10 unchanged · avg +0.2%
closures.js — 11 unchanged · avg -0.9%
collections.js — 12 unchanged · avg -1.4%
destructuring.js — 14 unchanged · avg -1.6%
fibonacci.js — 3 unchanged · avg -0.3%
json.js — 11 unchanged · avg -1.0%
numbers.js — 11 unchanged · avg -1.0%
objects.js — 7 unchanged · avg -0.3%
promises.js — 🔴 1 regressed, 11 unchanged · avg -7.1%
strings.js — 11 unchanged · avg -0.6%
Measured on ubuntu-latest x64. Changes within ±7% are considered insignificant. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
units/Goccia.Values.Primitives.pas (2)
519-531:⚠️ Potential issue | 🟡 MinorAdd test procedures for NaN and Infinity boolean coercion.
The test file
units/Goccia.Values.Primitives.Test.paslacks coverage for the new NaN/Infinity semantics inToBooleanLiteral. Add test procedures that verify:
TGocciaNumberLiteralValue.NaNValue.ToBooleanLiteralreturnsFalseValueTGocciaNumberLiteralValue.InfinityValue.ToBooleanLiteralreturnsTrueValueTGocciaNumberLiteralValue.NegativeInfinityValue.ToBooleanLiteralreturnsTrueValueFollow the existing pattern in
TestNumberValue(around line 84) for structure and assertions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@units/Goccia.Values.Primitives.pas` around lines 519 - 531, Add three unit tests (matching the TestNumberValue pattern) that call TGocciaNumberLiteralValue.NaNValue.ToBooleanLiteral, .InfinityValue.ToBooleanLiteral and .NegativeInfinityValue.ToBooleanLiteral and assert they return TGocciaBooleanLiteralValue.FalseValue, TrueValue and TrueValue respectively; follow the existing TestNumberValue structure and naming, create procedures in units/Goccia.Values.Primitives.Test.pas, invoke the same assertion helpers used elsewhere in that test unit to compare the returned TGocciaBooleanLiteralValue against the expected FalseValue/TrueValue results.
372-397:⚠️ Potential issue | 🟠 MajorAdd test cases for NaN/Infinity/negative zero construction; audit Array.sort for special value handling.
The constructor design is sound—normalizing special values to
ZERO_VALUEwhile storing state inFSpecialValueis safe only if callers checkIsNaN/IsInfiniteproperties first. However, the test suite has no coverage for direct special value construction, andArrayValue.passorting compares.Valuedirectly without checking for NaN, which will produce incorrect sort orders (NaN comparisons always yield false).Add unit tests for:
TGocciaNumberLiteralValue.Create(NaN)→ verifyIsNaNandValue = 0TGocciaNumberLiteralValue.Create(Infinity)→ verifyIsInfiniteandValue = 0TGocciaNumberLiteralValue.NegativeZeroValue→ verifyIsNegativeZeroFor the sort comparator, follow the pattern in
StringObjectValue.pas: checkIsNaN/IsInfiniteflags before accessing.Value, or use a comparison method that handles NaN semantically (e.g., NaN comparisons should sort to a fixed position, not treat 0 = 0 as equal).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@units/Goccia.Values.Primitives.pas` around lines 372 - 397, Tests are missing for special-number construction and the array sort comparator in ArrayValue.pas compares .Value directly (breaking on NaN/infinity/negative zero); add unit tests asserting TGocciaNumberLiteralValue.Create(Math.NaN) yields IsNaN true and Value = ZERO_VALUE, TGocciaNumberLiteralValue.Create(Math.Infinity) yields IsInfinite true and Value = ZERO_VALUE, and the NegativeZero factory produces IsNegativeZero true. Update the ArrayValue sort comparator to follow the pattern in StringObjectValue.pas: consult TGocciaNumberLiteralValue.IsNaN/IsInfinite/IsNegativeZero before using .Value, implement a deterministic ordering for NaN/Infinity/−0 (e.g., NaN last, -Infinity < normal < Infinity, treat negative zero distinct from positive zero) and use that comparison routine for sorting instead of direct .Value comparisons.
🧹 Nitpick comments (1)
.github/workflows/pr.yml (1)
49-51: Consider whether full history is needed for test and benchmark jobs.Both jobs download pre-built artifacts and don't appear to require git history. If versioning is embedded during the build step, these jobs could use the default shallow clone (
fetch-depth: 1) for slightly faster checkout times.However, maintaining consistency across all jobs is also a valid approach, especially for a small repository where the performance difference is negligible.
Also applies to: 72-74
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/pr.yml around lines 49 - 51, The checkout steps currently use actions/checkout@v4 with fetch-depth: 0 (full history); for the test and benchmark jobs referenced in the comment, change fetch-depth: 0 to fetch-depth: 1 to use a shallow clone (improves checkout speed) unless those jobs actually require full git history for versioning or artifact operations—update the two occurrences of actions/checkout@v4 (the checkout steps at the lines noted) to set fetch-depth: 1, or explicitly document/centralize why fetch-depth: 0 must be kept if consistency or build-embedded versioning requires full history.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@build.pas`:
- Around line 21-44: PrintVersion currently calls RunCommand for git describe
and rev-parse without checking for failure, causing empty Describe/Commit and
misleading output; update PrintVersion to check the result of each RunCommand
call (for the Describe and Commit invocations) and use safe fallbacks if the
command fails or returns empty (e.g., set Version to '0.0.0-dev' when Describe
is missing and Commit to 'unknown' when rev-parse fails), preserving the
existing trimming/substring logic for Describe and ensuring the final WriteLn
prints the fallback values; reference PrintVersion, RunCommand, Describe,
Commit, Version and adjust the control flow to handle command failure paths
consistently with other RunCommand usages in the file.
In `@tests/language/global-properties/nan.js`:
- Around line 1-121: Split this combined NaN test into three files: nan.happy.js
should contain the normal behavior tests (the describe "NaN as a global
property" block and the "NaN shadowing" tests that assert normal results such as
"global NaN has the correct value", "typeof NaN returns 'number'", propagation,
string conversion, shadowing cases including tests that use let/const/arrow
param/property/method names), nan.edge.js should contain non-fatal edge-case
assertions such as the falsy check ("NaN is falsy") and propagation through
arithmetic if you consider them edge, and nan.error.js should contain the
immutability/error cases from the "NaN immutability" describe block (tests
"global NaN cannot be reassigned" and "global NaN remains unchanged after failed
reassignment"). Preserve each test's content and names (e.g., the describe
blocks "NaN as a global property", "NaN immutability", "NaN shadowing" and test
names) so they remain identifiable, and update test runner imports if needed to
include the three new files.
In `@units/Goccia.Engine.pas`:
- Around line 311-349: Constructed TGocciaObjectValue/TGocciaArrayValue
instances in RegisterGlobalThis and RegisterGocciaScriptGlobal (GlobalThisObj,
BuiltInsArray, GocciaObj) are only referenced by local Pascal variables during
property assignment and may be collected by the GC; wrap their lifetime with
AddTempRoot immediately after creation and call RemoveTempRoot right after you
finish AssignProperty/DefineLexicalBinding (i.e., protect GlobalThisObj while
you copy Scope bindings and set 'globalThis' then remove the temp root after
Scope.DefineLexicalBinding; similarly protect BuiltInsArray and GocciaObj while
building and assigning properties and remove their temp roots after
FInterpreter.GlobalScope.DefineLexicalBinding). Ensure you call
AddTempRoot/RemoveTempRoot in matching pairs around the construction-to-binding
window to prevent premature collection.
- Around line 303-309: The globalThis snapshot is created in RegisterGlobalThis
before RegisterGocciaScriptGlobal runs, so the GocciaScript binding isn't
included on globalThis; move the RegisterGocciaScriptGlobal call to before
RegisterGlobalThis (i.e., call RegisterGocciaScriptGlobal, then
RegisterGlobalThis) so the GocciaScript binding is present on
globalThis.GocciaScript after the snapshot.
In `@units/Goccia.Evaluator.Arithmetic.pas`:
- Around line 174-183: The division logic mishandles IEEE-754 signed zeros: when
computing SameSign you use RightNum.Value >= 0 which treats +0 and -0 the same,
and when RightNum.IsInfinite you return +0 unconditionally; update checks to
detect negative zero (use an IsNegativeZero helper or evaluate 1/RightNum.Value
< 0) so InfinityWithSign(SameSign) uses the true sign of RightNum and
TGocciaNumberLiteralValue.ZeroValue is returned with the correct sign (negative
zero when operand signs dictate) in the RightNum.IsInfinite branch.
- Around line 249-256: The branch handling LeftNum.IsInfinite must first detect
non-integer exponents and return NaN for negative-infinity bases: if
LeftNum.IsInfinite and LeftNum.IsInfinity is true (negative infinity) and
RightNum.Value is non-integer (Frac(RightNum.Value) <> 0) then set Result to a
NaN numeric literal instead of applying the even/odd check; otherwise keep the
integer-exponent logic (use Frac(RightNum.Value / 2) = 0 only after confirming
the exponent is an integer) and preserve the existing InfinityWithSign and
TGocciaNumberLiteralValue.ZeroValue outcomes.
- Around line 258-267: The infinite-exponent branch conflates |base|=1 and sign
handling; update the RightNum.IsInfinite block to handle three cases: if
Abs(LeftNum.Value)=1 then return TGocciaNumberLiteralValue.OneValue when
LeftNum.Value = 1 and TGocciaNumberLiteralValue.NaNValue when LeftNum.Value =
-1; else if Abs(LeftNum.Value) > 1 then return
InfinityWithSign(RightNum.IsInfinity) for +Infinity and
TGocciaNumberLiteralValue.ZeroValue for RightNum.IsNegativeInfinity; else
(Abs(LeftNum.Value) < 1) return TGocciaNumberLiteralValue.ZeroValue for
RightNum.IsInfinity and InfinityWithSign(RightNum.IsNegativeInfinity) for
RightNum.IsNegativeInfinity; use LeftNum.Value, RightNum.IsInfinite,
RightNum.IsInfinity, RightNum.IsNegativeInfinity,
TGocciaNumberLiteralValue.ZeroValue/OneValue/NaNValue and InfinityWithSign to
implement this logic.
In `@units/Goccia.Parser.pas`:
- Around line 1357-1479: Skip logic in SkipBlock/SkipStatementOrBlock and in
VarStatement/ForStatement/WhileStatement/DoWhileStatement/WithStatement
currently scans to the first ')' or uses SkipUntilSemicolon which can desync on
nested parentheses and compound bodies; replace the ad-hoc scans by (1) adding a
balanced-paren helper (e.g. SkipBalancedParens) used by
ForStatement/WhileStatement/DoWhileStatement/WithStatement instead of looping
until the first gttRightParen, and (2) change SkipStatementOrBlock to actually
parse a single statement body (call the existing statement parsing routine to
build the statement and then free it) rather than calling SkipUntilSemicolon, so
recovery handles nested blocks, if/else/try/switch correctly. Ensure SkipBlock
remains balanced for braces and use these helpers from the referenced methods to
avoid over-skipping.
In `@units/Goccia.Version.pas`:
- Around line 38-57: In Initialize, after calling RunGit to set CachedCommit,
add a fallback when CachedCommit is empty (non-git or git unavailable) by
assigning a non-empty placeholder (e.g. 'unknown' or '0000000') so tests
expecting GocciaScript.commit.length > 0 pass; update the procedure that calls
RunGit(['rev-parse', '--short', 'HEAD']) and set CachedCommit to the placeholder
if it returns an empty string, leaving the existing CachedVersion logic
unchanged.
---
Outside diff comments:
In `@units/Goccia.Values.Primitives.pas`:
- Around line 519-531: Add three unit tests (matching the TestNumberValue
pattern) that call TGocciaNumberLiteralValue.NaNValue.ToBooleanLiteral,
.InfinityValue.ToBooleanLiteral and .NegativeInfinityValue.ToBooleanLiteral and
assert they return TGocciaBooleanLiteralValue.FalseValue, TrueValue and
TrueValue respectively; follow the existing TestNumberValue structure and
naming, create procedures in units/Goccia.Values.Primitives.Test.pas, invoke the
same assertion helpers used elsewhere in that test unit to compare the returned
TGocciaBooleanLiteralValue against the expected FalseValue/TrueValue results.
- Around line 372-397: Tests are missing for special-number construction and the
array sort comparator in ArrayValue.pas compares .Value directly (breaking on
NaN/infinity/negative zero); add unit tests asserting
TGocciaNumberLiteralValue.Create(Math.NaN) yields IsNaN true and Value =
ZERO_VALUE, TGocciaNumberLiteralValue.Create(Math.Infinity) yields IsInfinite
true and Value = ZERO_VALUE, and the NegativeZero factory produces
IsNegativeZero true. Update the ArrayValue sort comparator to follow the pattern
in StringObjectValue.pas: consult
TGocciaNumberLiteralValue.IsNaN/IsInfinite/IsNegativeZero before using .Value,
implement a deterministic ordering for NaN/Infinity/−0 (e.g., NaN last,
-Infinity < normal < Infinity, treat negative zero distinct from positive zero)
and use that comparison routine for sorting instead of direct .Value
comparisons.
---
Duplicate comments:
In `@tests/language/global-properties/infinity.js`:
- Around line 1-147: Split this single test file into three files following the
project test-structure: a happy-path file containing normal behavior checks
(tests titled "global Infinity has the correct value", "typeof Infinity returns
'number'", "Infinity is equal to itself", "negative Infinity", "Infinity is
truthy", "negative Infinity is truthy", "arithmetic with Infinity", "Infinity
converts to 'Infinity' string", "Number.isFinite rejects Infinity", and the
"Infinity as a property/method name works" and "arrow function parameter can
shadow Infinity" tests), an edge-cases file for shadowing and scoping behavior
(tests under "Infinity shadowing" such as "Infinity can be shadowed in a block
scope with let/const", "shadowed Infinity does not affect outer scope", "nested
shadowing restores correctly", and the "Infinity as a property name works" if
considered non-error edge), and an errors file for mutation/reassignment failure
checks (tests under "Infinity immutability": "global Infinity cannot be
reassigned" and "global Infinity remains unchanged after failed reassignment");
ensure each new file keeps the same test names and descriptions,
imports/describe blocks, and that tests referencing Number.isFinite or using
try/catch remain unchanged so assertions still run as before.
In `@tests/language/global-properties/undefined.js`:
- Around line 1-150: Split this combined test into three files: a happy-path
file containing stable non-error behaviors (move tests from describe("undefined
as a global property") and non-failing shadowing tests such as "undefined can be
shadowed in a block scope with let/const", "arrow function parameter can shadow
undefined", "undefined as a property name works", "undefined as a method name
works", and "undefined shadowing" cases that assert values/types), an edge-cases
file for borderline behaviors (move nested/shadowing variations like "nested
shadowing restores correctly" and the "shadowed undefined can be any type" group
and arithmetic/string coercion tests like Number.isNaN and `${undefined}`), and
an error-cases file for tests that assert thrown behavior (move
describe("undefined immutability") tests such as "global undefined cannot be
reassigned" and related reassignment-try/catch checks). Keep the original
describe blocks and test names (e.g., describe("undefined as a global
property"), describe("undefined immutability"), describe("undefined shadowing"))
to locate and extract tests, and ensure each new file follows the existing test
header/comments and naming convention (e.g., undefined/happy.js,
undefined/edge.js, undefined/error.js).
---
Nitpick comments:
In @.github/workflows/pr.yml:
- Around line 49-51: The checkout steps currently use actions/checkout@v4 with
fetch-depth: 0 (full history); for the test and benchmark jobs referenced in the
comment, change fetch-depth: 0 to fetch-depth: 1 to use a shallow clone
(improves checkout speed) unless those jobs actually require full git history
for versioning or artifact operations—update the two occurrences of
actions/checkout@v4 (the checkout steps at the lines noted) to set fetch-depth:
1, or explicitly document/centralize why fetch-depth: 0 must be kept if
consistency or build-embedded versioning requires full history.
Comment on lines
+21
to
+44
| procedure PrintVersion; | ||
| var | ||
| Describe, Commit, Version: string; | ||
| DashPos: SizeInt; | ||
| begin | ||
| RunCommand('git', ['describe', '--tags', '--always'], Describe); | ||
| Describe := Trim(Describe); | ||
| RunCommand('git', ['rev-parse', '--short', 'HEAD'], Commit); | ||
| Commit := Trim(Commit); | ||
|
|
||
| if Describe = '' then | ||
| Version := '0.0.0-dev' | ||
| else | ||
| begin | ||
| DashPos := Pos('-', Describe); | ||
| if DashPos > 0 then | ||
| Version := Copy(Describe, 1, DashPos - 1) + '-dev' | ||
| else | ||
| Version := Describe; | ||
| end; | ||
|
|
||
| WriteLn('Version: ', Version, ' (', Commit, ')'); | ||
| WriteLn(''); | ||
| end; |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n --type=pas -C2 "RunCommand\("Repository: frostney/GocciaScript
Length of output: 92
🏁 Script executed:
# Find the RunCommand procedure/function definition
rg -n --type=pas "^(function|procedure)\s+RunCommand\s*\("Repository: frostney/GocciaScript
Length of output: 92
🏁 Script executed:
# Check build.pas structure and context around line 249
cat -n build.pas | head -260 | tail -40Repository: frostney/GocciaScript
Length of output: 1222
🏁 Script executed:
# Search for RunCommand in Pascal files
rg -n "RunCommand\(" --glob="*.pas" -C2Repository: frostney/GocciaScript
Length of output: 1684
🏁 Script executed:
# Find RunCommand definition
rg -n "procedure RunCommand|function RunCommand" --glob="*.pas"Repository: frostney/GocciaScript
Length of output: 47
🏁 Script executed:
# Check complete build.pas for error handling context
fd "build.pas" -x wc -l {}Repository: frostney/GocciaScript
Length of output: 80
🏁 Script executed:
# Show exact lines 21-44 from build.pas with line numbers
sed -n '21,44p' build.pas | cat -n --vt100Repository: frostney/GocciaScript
Length of output: 138
🏁 Script executed:
# Show the full context of how Describe is used after assignment
sed -n '21,45p' build.pas | cat -nRepository: frostney/GocciaScript
Length of output: 816
Add error handling to git commands in PrintVersion to avoid misleading version output.
The RunCommand calls at lines 26 and 28 lack error checks, inconsistent with every other RunCommand usage in this file (lines 94, 110, 142, 159, 174). If git commands fail (missing git, shallow clone, etc.), the output will display an empty Commit string instead of a fallback value.
Suggested fix
- RunCommand('git', ['describe', '--tags', '--always'], Describe);
- Describe := Trim(Describe);
- RunCommand('git', ['rev-parse', '--short', 'HEAD'], Commit);
- Commit := Trim(Commit);
+ if not RunCommand('git', ['describe', '--tags', '--always'], Describe) then
+ Describe := '';
+ Describe := Trim(Describe);
+
+ if not RunCommand('git', ['rev-parse', '--short', 'HEAD'], Commit) then
+ Commit := 'unknown';
+ Commit := Trim(Commit);
+ if Commit = '' then
+ Commit := 'unknown';📝 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.
Suggested change
| procedure PrintVersion; | |
| var | |
| Describe, Commit, Version: string; | |
| DashPos: SizeInt; | |
| begin | |
| RunCommand('git', ['describe', '--tags', '--always'], Describe); | |
| Describe := Trim(Describe); | |
| RunCommand('git', ['rev-parse', '--short', 'HEAD'], Commit); | |
| Commit := Trim(Commit); | |
| if Describe = '' then | |
| Version := '0.0.0-dev' | |
| else | |
| begin | |
| DashPos := Pos('-', Describe); | |
| if DashPos > 0 then | |
| Version := Copy(Describe, 1, DashPos - 1) + '-dev' | |
| else | |
| Version := Describe; | |
| end; | |
| WriteLn('Version: ', Version, ' (', Commit, ')'); | |
| WriteLn(''); | |
| end; | |
| procedure PrintVersion; | |
| var | |
| Describe, Commit, Version: string; | |
| DashPos: SizeInt; | |
| begin | |
| if not RunCommand('git', ['describe', '--tags', '--always'], Describe) then | |
| Describe := ''; | |
| Describe := Trim(Describe); | |
| if not RunCommand('git', ['rev-parse', '--short', 'HEAD'], Commit) then | |
| Commit := 'unknown'; | |
| Commit := Trim(Commit); | |
| if Commit = '' then | |
| Commit := 'unknown'; | |
| if Describe = '' then | |
| Version := '0.0.0-dev' | |
| else | |
| begin | |
| DashPos := Pos('-', Describe); | |
| if DashPos > 0 then | |
| Version := Copy(Describe, 1, DashPos - 1) + '-dev' | |
| else | |
| Version := Describe; | |
| end; | |
| WriteLn('Version: ', Version, ' (', Commit, ')'); | |
| WriteLn(''); | |
| end; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@build.pas` around lines 21 - 44, PrintVersion currently calls RunCommand for
git describe and rev-parse without checking for failure, causing empty
Describe/Commit and misleading output; update PrintVersion to check the result
of each RunCommand call (for the Describe and Commit invocations) and use safe
fallbacks if the command fails or returns empty (e.g., set Version to
'0.0.0-dev' when Describe is missing and Commit to 'unknown' when rev-parse
fails), preserving the existing trimming/substring logic for Describe and
ensuring the final WriteLn prints the fallback values; reference PrintVersion,
RunCommand, Describe, Commit, Version and adjust the control flow to handle
command failure paths consistently with other RunCommand usages in the file.
Comment on lines
+1
to
+121
| /*--- | ||
| description: > | ||
| NaN is a global property (not a keyword) per ECMAScript spec. | ||
| It can be shadowed by local variable declarations. | ||
| features: [global-properties, NaN, shadowing] | ||
| ---*/ | ||
|
|
||
| describe("NaN as a global property", () => { | ||
| test("global NaN has the correct value", () => { | ||
| expect(Number.isNaN(NaN)).toBe(true); | ||
| }); | ||
|
|
||
| test("typeof NaN returns 'number'", () => { | ||
| expect(typeof NaN).toBe("number"); | ||
| }); | ||
|
|
||
| test("NaN is not equal to itself", () => { | ||
| expect(NaN === NaN).toBe(false); | ||
| expect(NaN !== NaN).toBe(true); | ||
| }); | ||
|
|
||
| test("NaN is falsy", () => { | ||
| let result = "truthy"; | ||
| if (!NaN) { | ||
| result = "falsy"; | ||
| } | ||
| expect(result).toBe("falsy"); | ||
| }); | ||
|
|
||
| test("NaN propagates through arithmetic", () => { | ||
| expect(Number.isNaN(NaN + 1)).toBe(true); | ||
| expect(Number.isNaN(NaN * 2)).toBe(true); | ||
| expect(Number.isNaN(NaN - 0)).toBe(true); | ||
| expect(Number.isNaN(NaN / 10)).toBe(true); | ||
| }); | ||
|
|
||
| test("NaN converts to 'NaN' string", () => { | ||
| expect(`${NaN}`).toBe("NaN"); | ||
| expect("" + NaN).toBe("NaN"); | ||
| }); | ||
| }); | ||
|
|
||
| describe("NaN immutability", () => { | ||
| test("global NaN cannot be reassigned", () => { | ||
| expect(() => { | ||
| NaN = 42; | ||
| }).toThrow(); | ||
| }); | ||
|
|
||
| test("global NaN remains unchanged after failed reassignment", () => { | ||
| try { | ||
| NaN = 0; | ||
| } catch (e) { | ||
| // expected | ||
| } | ||
| expect(Number.isNaN(NaN)).toBe(true); | ||
| }); | ||
| }); | ||
|
|
||
| describe("NaN shadowing", () => { | ||
| test("NaN can be shadowed in a block scope with let", () => { | ||
| expect(Number.isNaN(NaN)).toBe(true); | ||
| { | ||
| let NaN = 42; | ||
| expect(NaN).toBe(42); | ||
| } | ||
| expect(Number.isNaN(NaN)).toBe(true); | ||
| }); | ||
|
|
||
| test("NaN can be shadowed in a block scope with const", () => { | ||
| { | ||
| const NaN = "not a number"; | ||
| expect(NaN).toBe("not a number"); | ||
| } | ||
| expect(Number.isNaN(NaN)).toBe(true); | ||
| }); | ||
|
|
||
| test("shadowed NaN does not affect outer scope", () => { | ||
| const outer = NaN; | ||
| { | ||
| let NaN = 100; | ||
| expect(NaN).toBe(100); | ||
| } | ||
| expect(Number.isNaN(outer)).toBe(true); | ||
| }); | ||
|
|
||
| test("nested shadowing restores correctly", () => { | ||
| expect(Number.isNaN(NaN)).toBe(true); | ||
| { | ||
| let NaN = 1; | ||
| expect(NaN).toBe(1); | ||
| { | ||
| let NaN = 2; | ||
| expect(NaN).toBe(2); | ||
| } | ||
| expect(NaN).toBe(1); | ||
| } | ||
| expect(Number.isNaN(NaN)).toBe(true); | ||
| }); | ||
|
|
||
| test("arrow function parameter can shadow NaN", () => { | ||
| const fn = (NaN) => NaN; | ||
| expect(fn(42)).toBe(42); | ||
| expect(fn("test")).toBe("test"); | ||
| }); | ||
|
|
||
| test("NaN as a property name works", () => { | ||
| const obj = { NaN: "prop-value" }; | ||
| expect(obj.NaN).toBe("prop-value"); | ||
| expect(obj["NaN"]).toBe("prop-value"); | ||
| }); | ||
|
|
||
| test("NaN as a method name works", () => { | ||
| const obj = { | ||
| NaN() { | ||
| return "method-value"; | ||
| }, | ||
| }; | ||
| expect(obj.NaN()).toBe("method-value"); | ||
| }); | ||
| }); |
Contributor
There was a problem hiding this comment.
Split NaN tests into happy/edge/error files.
This file mixes happy paths, edge cases, and error cases. Please split into separate files (e.g., nan.happy.js, nan.edge.js, nan.error.js) following the existing test structure.
As per coding guidelines, "When implementing a new language feature, create JavaScript test files under the tests/ directory following the existing directory structure, with separate files for happy paths, edge cases, and error cases."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/language/global-properties/nan.js` around lines 1 - 121, Split this
combined NaN test into three files: nan.happy.js should contain the normal
behavior tests (the describe "NaN as a global property" block and the "NaN
shadowing" tests that assert normal results such as "global NaN has the correct
value", "typeof NaN returns 'number'", propagation, string conversion, shadowing
cases including tests that use let/const/arrow param/property/method names),
nan.edge.js should contain non-fatal edge-case assertions such as the falsy
check ("NaN is falsy") and propagation through arithmetic if you consider them
edge, and nan.error.js should contain the immutability/error cases from the "NaN
immutability" describe block (tests "global NaN cannot be reassigned" and
"global NaN remains unchanged after failed reassignment"). Preserve each test's
content and names (e.g., the describe blocks "NaN as a global property", "NaN
immutability", "NaN shadowing" and test names) so they remain identifiable, and
update test runner imports if needed to include the three new files.
Comment on lines
303
to
+309
| // Create Function constructor | ||
| FunctionConstructor := TGocciaClassValue.Create('Function', nil); | ||
| FInterpreter.GlobalScope.DefineLexicalBinding('Function', FunctionConstructor, dtConst); | ||
|
|
||
| RegisterGlobalThis; | ||
| RegisterGocciaScriptGlobal; | ||
| end; |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the Goccia.Engine.pas file around the mentioned lines
cd /tmp && find . -name "Goccia.Engine.pas" 2>/dev/null | head -5Repository: frostney/GocciaScript
Length of output: 47
🏁 Script executed:
# Search for the file location
find . -type f -name "*.pas" | grep -i engine | head -10Repository: frostney/GocciaScript
Length of output: 90
🏁 Script executed:
# Let's also check the repository structure
git ls-files | grep -E "(Engine|\.pas$)" | head -20Repository: frostney/GocciaScript
Length of output: 712
🏁 Script executed:
# Read the Goccia.Engine.pas file to examine the registration methods
wc -l ./units/Goccia.Engine.pasRepository: frostney/GocciaScript
Length of output: 94
🏁 Script executed:
# Read the relevant section around lines 303-349
sed -n '290,360p' ./units/Goccia.Engine.pasRepository: frostney/GocciaScript
Length of output: 2571
🏁 Script executed:
# Search for all globalThis modifications and usage patterns
rg -n "globalThis" --type pas --type js --type gocciaRepository: frostney/GocciaScript
Length of output: 92
🏁 Script executed:
# Search for GocciaScript usage to understand if it should be in globalThis
rg -n "GocciaScript" --type pas --type js --type gocciaRepository: frostney/GocciaScript
Length of output: 92
🏁 Script executed:
# Check if there's any code that patches globalThis after these registrations
sed -n '1,480p' ./units/Goccia.Engine.pas | grep -n "globalThis\|GocciaScript"Repository: frostney/GocciaScript
Length of output: 2188
🏁 Script executed:
# Search for globalThis and GocciaScript without file type restrictions
rg -n "globalThis" -g '*.pas' -g '*.js' -g '*.goccia'Repository: frostney/GocciaScript
Length of output: 7356
🏁 Script executed:
# Search for GocciaScript usage
rg -n "GocciaScript" -g '*.pas' -g '*.js' -g '*.goccia'Repository: frostney/GocciaScript
Length of output: 6233
🏁 Script executed:
# Look for any test files that might reveal expected behavior
find . -type f \( -name "*test*" -o -name "*Test*" \) | head -20Repository: frostney/GocciaScript
Length of output: 362
🏁 Script executed:
# Check if any tests or code explicitly verify globalThis.GocciaScript
rg -n "globalThis\.GocciaScript" -g '*.js' -g '*.pas' -g '*.goccia'Repository: frostney/GocciaScript
Length of output: 47
🏁 Script executed:
# Check if there's any code that adds GocciaScript to globalThis after registration
sed -n '300,360p' ./units/Goccia.Engine.pas | grep -A 50 "RegisterGocciaScriptGlobal"Repository: frostney/GocciaScript
Length of output: 1723
🏁 Script executed:
# Look at the full initialization sequence to confirm no patching occurs elsewhere
sed -n '260,360p' ./units/Goccia.Engine.pasRepository: frostney/GocciaScript
Length of output: 3954
globalThis snapshot taken before GocciaScript registration.
RegisterGlobalThis creates a snapshot of the current scope's bindings (line 319) and assigns them to the globalThis object. At that point, GocciaScript has not yet been registered. Swapping the call order on lines 307–308 would make GocciaScript accessible via globalThis.GocciaScript; otherwise, it remains accessible only as a top-level binding.
Suggested fix
- RegisterGlobalThis;
- RegisterGocciaScriptGlobal;
+ RegisterGocciaScriptGlobal;
+ RegisterGlobalThis;📝 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.
Suggested change
| // Create Function constructor | |
| FunctionConstructor := TGocciaClassValue.Create('Function', nil); | |
| FInterpreter.GlobalScope.DefineLexicalBinding('Function', FunctionConstructor, dtConst); | |
| RegisterGlobalThis; | |
| RegisterGocciaScriptGlobal; | |
| end; | |
| // Create Function constructor | |
| FunctionConstructor := TGocciaClassValue.Create('Function', nil); | |
| FInterpreter.GlobalScope.DefineLexicalBinding('Function', FunctionConstructor, dtConst); | |
| RegisterGocciaScriptGlobal; | |
| RegisterGlobalThis; | |
| end; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@units/Goccia.Engine.pas` around lines 303 - 309, The globalThis snapshot is
created in RegisterGlobalThis before RegisterGocciaScriptGlobal runs, so the
GocciaScript binding isn't included on globalThis; move the
RegisterGocciaScriptGlobal call to before RegisterGlobalThis (i.e., call
RegisterGocciaScriptGlobal, then RegisterGlobalThis) so the GocciaScript binding
is present on globalThis.GocciaScript after the snapshot.
Comment on lines
+311
to
349
| procedure TGocciaEngine.RegisterGlobalThis; | ||
| var | ||
| GlobalThisObj: TGocciaObjectValue; | ||
| Scope: TGocciaScope; | ||
| Name: string; | ||
| begin | ||
| Scope := FInterpreter.GlobalScope; | ||
| GlobalThisObj := TGocciaObjectValue.Create; | ||
|
|
||
| for Name in Scope.GetOwnBindingNames do | ||
| GlobalThisObj.AssignProperty(Name, Scope.GetValue(Name)); | ||
|
|
||
| GlobalThisObj.AssignProperty('globalThis', GlobalThisObj); | ||
| Scope.DefineLexicalBinding('globalThis', GlobalThisObj, dtConst); | ||
| end; | ||
|
|
||
| procedure TGocciaEngine.RegisterGocciaScriptGlobal; | ||
| const | ||
| PREFIX_LENGTH = 2; // Strip 'gg' prefix from enum names | ||
| var | ||
| GocciaObj: TGocciaObjectValue; | ||
| BuiltInsArray: TGocciaArrayValue; | ||
| Flag: TGocciaGlobalBuiltin; | ||
| Name: string; | ||
| begin | ||
| BuiltInsArray := TGocciaArrayValue.Create; | ||
| for Flag in FGlobals do | ||
| begin | ||
| Name := GetEnumName(TypeInfo(TGocciaGlobalBuiltin), Ord(Flag)); | ||
| BuiltInsArray.Elements.Add(TGocciaStringLiteralValue.Create(Copy(Name, PREFIX_LENGTH + 1, Length(Name) - PREFIX_LENGTH))); | ||
| end; | ||
|
|
||
| GocciaObj := TGocciaObjectValue.Create; | ||
| GocciaObj.AssignProperty('version', TGocciaStringLiteralValue.Create(GetVersion)); | ||
| GocciaObj.AssignProperty('commit', TGocciaStringLiteralValue.Create(GetCommit)); | ||
| GocciaObj.AssignProperty('builtIns', BuiltInsArray); | ||
|
|
||
| FInterpreter.GlobalScope.DefineLexicalBinding('GocciaScript', GocciaObj, dtConst); | ||
| end; |
Contributor
There was a problem hiding this comment.
Temp-root new values during global registration.
Line 318-348 allocates TGocciaObjectValue/TGocciaArrayValue instances that are only held in local Pascal variables until the scope bindings are created. If GC runs during property assignment, these values can be collected. Please wrap the construction phase with AddTempRoot/RemoveTempRoot.
🛠️ Suggested fix
procedure TGocciaEngine.RegisterGlobalThis;
var
GlobalThisObj: TGocciaObjectValue;
Scope: TGocciaScope;
Name: string;
begin
Scope := FInterpreter.GlobalScope;
GlobalThisObj := TGocciaObjectValue.Create;
+ TGocciaGarbageCollector.Instance.AddTempRoot(GlobalThisObj);
+ try
- for Name in Scope.GetOwnBindingNames do
- GlobalThisObj.AssignProperty(Name, Scope.GetValue(Name));
-
- GlobalThisObj.AssignProperty('globalThis', GlobalThisObj);
- Scope.DefineLexicalBinding('globalThis', GlobalThisObj, dtConst);
+ for Name in Scope.GetOwnBindingNames do
+ GlobalThisObj.AssignProperty(Name, Scope.GetValue(Name));
+
+ GlobalThisObj.AssignProperty('globalThis', GlobalThisObj);
+ Scope.DefineLexicalBinding('globalThis', GlobalThisObj, dtConst);
+ finally
+ TGocciaGarbageCollector.Instance.RemoveTempRoot(GlobalThisObj);
+ end;
end; procedure TGocciaEngine.RegisterGocciaScriptGlobal;
const
PREFIX_LENGTH = 2; // Strip 'gg' prefix from enum names
var
GocciaObj: TGocciaObjectValue;
BuiltInsArray: TGocciaArrayValue;
Flag: TGocciaGlobalBuiltin;
Name: string;
begin
BuiltInsArray := TGocciaArrayValue.Create;
+ TGocciaGarbageCollector.Instance.AddTempRoot(BuiltInsArray);
+ try
for Flag in FGlobals do
begin
Name := GetEnumName(TypeInfo(TGocciaGlobalBuiltin), Ord(Flag));
BuiltInsArray.Elements.Add(TGocciaStringLiteralValue.Create(Copy(Name, PREFIX_LENGTH + 1, Length(Name) - PREFIX_LENGTH)));
end;
GocciaObj := TGocciaObjectValue.Create;
- GocciaObj.AssignProperty('version', TGocciaStringLiteralValue.Create(GetVersion));
- GocciaObj.AssignProperty('commit', TGocciaStringLiteralValue.Create(GetCommit));
- GocciaObj.AssignProperty('builtIns', BuiltInsArray);
-
- FInterpreter.GlobalScope.DefineLexicalBinding('GocciaScript', GocciaObj, dtConst);
+ TGocciaGarbageCollector.Instance.AddTempRoot(GocciaObj);
+ try
+ GocciaObj.AssignProperty('version', TGocciaStringLiteralValue.Create(GetVersion));
+ GocciaObj.AssignProperty('commit', TGocciaStringLiteralValue.Create(GetCommit));
+ GocciaObj.AssignProperty('builtIns', BuiltInsArray);
+
+ FInterpreter.GlobalScope.DefineLexicalBinding('GocciaScript', GocciaObj, dtConst);
+ finally
+ TGocciaGarbageCollector.Instance.RemoveTempRoot(GocciaObj);
+ end;
+ finally
+ TGocciaGarbageCollector.Instance.RemoveTempRoot(BuiltInsArray);
+ end;
end;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@units/Goccia.Engine.pas` around lines 311 - 349, Constructed
TGocciaObjectValue/TGocciaArrayValue instances in RegisterGlobalThis and
RegisterGocciaScriptGlobal (GlobalThisObj, BuiltInsArray, GocciaObj) are only
referenced by local Pascal variables during property assignment and may be
collected by the GC; wrap their lifetime with AddTempRoot immediately after
creation and call RemoveTempRoot right after you finish
AssignProperty/DefineLexicalBinding (i.e., protect GlobalThisObj while you copy
Scope bindings and set 'globalThis' then remove the temp root after
Scope.DefineLexicalBinding; similarly protect BuiltInsArray and GocciaObj while
building and assigning properties and remove their temp roots after
FInterpreter.GlobalScope.DefineLexicalBinding). Ensure you call
AddTempRoot/RemoveTempRoot in matching pairs around the construction-to-binding
window to prevent premature collection.
Comment on lines
174
to
183
| SameSign := LeftNum.IsInfinity = (RightNum.Value >= 0); | ||
| Result := InfinityWithSign(SameSign); | ||
| Exit; | ||
| end; | ||
|
|
||
| if LeftNum.IsNaN or RightNum.IsNaN then | ||
| if RightNum.IsInfinite then | ||
| begin | ||
| Result := TGocciaNumberLiteralValue.NaNValue; | ||
| Result := TGocciaNumberLiteralValue.ZeroValue; | ||
| Exit; | ||
| end; |
Contributor
There was a problem hiding this comment.
Negative zero handling may produce incorrect sign.
The sign checks don't account for IEEE 754 negative zero:
-
Line 174:
RightNum.Value >= 0evaluates true for both+0and-0, soInfinity / (-0)incorrectly yields+Infinityinstead of-Infinity. -
Line 181:
finite / ±Infinityalways returns+0, but ECMAScript requires signed zero based on operand signs (e.g.,5 / (-Infinity) = -0).
If signed-zero correctness is a goal, consider using IsNegativeZero helper or 1/Value < 0 to detect -0.
💡 Sketch for handling signed zero in divisor
+function IsNegativeZero(const AValue: Double): Boolean; inline;
+begin
+ Result := (AValue = 0) and ((1.0 / AValue) < 0);
+end;
+
// In EvaluateDivision, line 174:
- SameSign := LeftNum.IsInfinity = (RightNum.Value >= 0);
+ SameSign := LeftNum.IsInfinity xor IsNegativeZero(RightNum.Value);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@units/Goccia.Evaluator.Arithmetic.pas` around lines 174 - 183, The division
logic mishandles IEEE-754 signed zeros: when computing SameSign you use
RightNum.Value >= 0 which treats +0 and -0 the same, and when
RightNum.IsInfinite you return +0 unconditionally; update checks to detect
negative zero (use an IsNegativeZero helper or evaluate 1/RightNum.Value < 0) so
InfinityWithSign(SameSign) uses the true sign of RightNum and
TGocciaNumberLiteralValue.ZeroValue is returned with the correct sign (negative
zero when operand signs dictate) in the RightNum.IsInfinite branch.
Comment on lines
+249
to
+256
| if LeftNum.IsInfinite then | ||
| begin | ||
| if RightNum.Value > 0 then | ||
| Result := InfinityWithSign(LeftNum.IsInfinity or (Frac(RightNum.Value / 2) = 0)) | ||
| else | ||
| Result := TGocciaNumberLiteralValue.ZeroValue; | ||
| Exit; | ||
| end; |
Contributor
There was a problem hiding this comment.
Fractional exponents with negative infinity base yield incorrect result.
When base is -Infinity and exponent is a non-integer (e.g., 0.5), the result should be NaN (complex number domain), but the current logic treats all exponents uniformly:
(-Infinity) ** 0.5 // NaN in ECMAScript
(-Infinity) ** 2 // Infinity
(-Infinity) ** 3 // -InfinityThe Frac(RightNum.Value / 2) = 0 check determines even vs. odd but doesn't distinguish integer from fractional exponents.
🐛 Proposed fix to handle fractional exponents
if LeftNum.IsInfinite then
begin
+ // Fractional exponent with negative base → NaN
+ if (not LeftNum.IsInfinity) and (Frac(RightNum.Value) <> 0) then
+ begin
+ Result := TGocciaNumberLiteralValue.NaNValue;
+ Exit;
+ end;
if RightNum.Value > 0 then
Result := InfinityWithSign(LeftNum.IsInfinity or (Frac(RightNum.Value / 2) = 0))
else
Result := TGocciaNumberLiteralValue.ZeroValue;
Exit;
end;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@units/Goccia.Evaluator.Arithmetic.pas` around lines 249 - 256, The branch
handling LeftNum.IsInfinite must first detect non-integer exponents and return
NaN for negative-infinity bases: if LeftNum.IsInfinite and LeftNum.IsInfinity is
true (negative infinity) and RightNum.Value is non-integer (Frac(RightNum.Value)
<> 0) then set Result to a NaN numeric literal instead of applying the even/odd
check; otherwise keep the integer-exponent logic (use Frac(RightNum.Value / 2) =
0 only after confirming the exponent is an integer) and preserve the existing
InfinityWithSign and TGocciaNumberLiteralValue.ZeroValue outcomes.
Comment on lines
+258
to
+267
| if RightNum.IsInfinite then | ||
| begin | ||
| if Abs(LeftNum.Value) = 1 then | ||
| Result := TGocciaNumberLiteralValue.NaNValue | ||
| else if Abs(LeftNum.Value) > 1 then | ||
| Result := InfinityWithSign(RightNum.IsInfinity) | ||
| else | ||
| Result := InfinityWithSign(RightNum.IsNegativeInfinity); | ||
| Exit; | ||
| end; |
Contributor
There was a problem hiding this comment.
Infinite exponent edge cases produce incorrect results.
Several ECMAScript edge cases are not handled correctly:
| Expression | Expected | Current |
|---|---|---|
1 ** Infinity |
1 |
NaN |
2 ** (-Infinity) |
0 |
-Infinity |
0.5 ** Infinity |
0 |
-Infinity |
The logic conflates |base| = 1 (should return 1 for base exactly 1, NaN for -1) and incorrectly returns infinity instead of zero for convergent cases.
🐛 Proposed fix for infinite exponent handling
if RightNum.IsInfinite then
begin
- if Abs(LeftNum.Value) = 1 then
- Result := TGocciaNumberLiteralValue.NaNValue
- else if Abs(LeftNum.Value) > 1 then
- Result := InfinityWithSign(RightNum.IsInfinity)
+ if LeftNum.Value = 1 then
+ Result := TGocciaNumberLiteralValue.OneValue
+ else if LeftNum.Value = -1 then
+ Result := TGocciaNumberLiteralValue.NaNValue
+ else if Abs(LeftNum.Value) > 1 then
+ begin
+ if RightNum.IsInfinity then
+ Result := TGocciaNumberLiteralValue.InfinityValue
+ else
+ Result := TGocciaNumberLiteralValue.ZeroValue;
+ end
else
- Result := InfinityWithSign(RightNum.IsNegativeInfinity);
+ begin
+ if RightNum.IsInfinity then
+ Result := TGocciaNumberLiteralValue.ZeroValue
+ else
+ Result := TGocciaNumberLiteralValue.InfinityValue;
+ end;
Exit;
end;📝 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.
Suggested change
| if RightNum.IsInfinite then | |
| begin | |
| if Abs(LeftNum.Value) = 1 then | |
| Result := TGocciaNumberLiteralValue.NaNValue | |
| else if Abs(LeftNum.Value) > 1 then | |
| Result := InfinityWithSign(RightNum.IsInfinity) | |
| else | |
| Result := InfinityWithSign(RightNum.IsNegativeInfinity); | |
| Exit; | |
| end; | |
| if RightNum.IsInfinite then | |
| begin | |
| if LeftNum.Value = 1 then | |
| Result := TGocciaNumberLiteralValue.OneValue | |
| else if LeftNum.Value = -1 then | |
| Result := TGocciaNumberLiteralValue.NaNValue | |
| else if Abs(LeftNum.Value) > 1 then | |
| begin | |
| if RightNum.IsInfinity then | |
| Result := TGocciaNumberLiteralValue.InfinityValue | |
| else | |
| Result := TGocciaNumberLiteralValue.ZeroValue; | |
| end | |
| else | |
| begin | |
| if RightNum.IsInfinity then | |
| Result := TGocciaNumberLiteralValue.ZeroValue | |
| else | |
| Result := TGocciaNumberLiteralValue.InfinityValue; | |
| end; | |
| Exit; | |
| end; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@units/Goccia.Evaluator.Arithmetic.pas` around lines 258 - 267, The
infinite-exponent branch conflates |base|=1 and sign handling; update the
RightNum.IsInfinite block to handle three cases: if Abs(LeftNum.Value)=1 then
return TGocciaNumberLiteralValue.OneValue when LeftNum.Value = 1 and
TGocciaNumberLiteralValue.NaNValue when LeftNum.Value = -1; else if
Abs(LeftNum.Value) > 1 then return InfinityWithSign(RightNum.IsInfinity) for
+Infinity and TGocciaNumberLiteralValue.ZeroValue for
RightNum.IsNegativeInfinity; else (Abs(LeftNum.Value) < 1) return
TGocciaNumberLiteralValue.ZeroValue for RightNum.IsInfinity and
InfinityWithSign(RightNum.IsNegativeInfinity) for RightNum.IsNegativeInfinity;
use LeftNum.Value, RightNum.IsInfinite, RightNum.IsInfinity,
RightNum.IsNegativeInfinity,
TGocciaNumberLiteralValue.ZeroValue/OneValue/NaNValue and InfinityWithSign to
implement this logic.
Comment on lines
+1357
to
1479
| procedure TGocciaParser.SkipBlock; | ||
| var | ||
| Depth: Integer; | ||
| begin | ||
| Consume(gttLeftBrace, 'Expected "{"'); | ||
| Depth := 1; | ||
| while not IsAtEnd and (Depth > 0) do | ||
| begin | ||
| if Check(gttLeftBrace) then Inc(Depth) | ||
| else if Check(gttRightBrace) then Dec(Depth); | ||
| if Depth > 0 then Advance; | ||
| end; | ||
| Consume(gttRightBrace, 'Expected "}"'); | ||
| end; | ||
|
|
||
| procedure TGocciaParser.SkipStatementOrBlock; | ||
| begin | ||
| if Check(gttLeftBrace) then | ||
| SkipBlock | ||
| else | ||
| SkipUntilSemicolon; | ||
| end; | ||
|
|
||
| function TGocciaParser.VarStatement: TGocciaStatement; | ||
| var | ||
| Line, Column: Integer; | ||
| begin | ||
| Line := Previous.Line; | ||
| Column := Previous.Column; | ||
|
|
||
| AddWarning('''var'' declarations are not supported in GocciaScript', | ||
| 'Use ''let'' or ''const'' instead', | ||
| Line, Column); | ||
|
|
||
| SkipUntilSemicolon; | ||
|
|
||
| Result := TGocciaEmptyStatement.Create(Line, Column); | ||
| end; | ||
|
|
||
| function TGocciaParser.ForStatement: TGocciaStatement; | ||
| var | ||
| Line, Column: Integer; | ||
| begin | ||
| Line := Previous.Line; | ||
| Column := Previous.Column; | ||
|
|
||
| // For now, just consume the basic for loop structure and return a simple statement | ||
| Consume(gttLeftParen, 'Expected "(" after "for"'); | ||
| AddWarning('''for'' loops are not supported in GocciaScript', | ||
| 'Use array methods like .forEach(), .map(), .filter(), or .reduce() instead', | ||
| Line, Column); | ||
|
|
||
| // Skip everything until the closing parenthesis | ||
| Consume(gttLeftParen, 'Expected "(" after "for"'); | ||
| while not Check(gttRightParen) and not IsAtEnd do | ||
| Advance; | ||
|
|
||
| Consume(gttRightParen, 'Expected ")" after for clauses'); | ||
|
|
||
| // Parse body - can be a block or single statement | ||
| if Check(gttLeftBrace) then | ||
| begin | ||
| Advance; // consume the '{' | ||
| Result := BlockStatement; | ||
| end | ||
| else | ||
| Result := Statement; | ||
| SkipStatementOrBlock; | ||
|
|
||
| Result := TGocciaEmptyStatement.Create(Line, Column); | ||
| end; | ||
|
|
||
| function TGocciaParser.WhileStatement: TGocciaStatement; | ||
| var | ||
| Condition: TGocciaExpression; | ||
| Body: TGocciaStatement; | ||
| Line, Column: Integer; | ||
| begin | ||
| Line := Previous.Line; | ||
| Column := Previous.Column; | ||
|
|
||
| AddWarning('''while'' loops are not supported in GocciaScript', | ||
| 'Use array methods like .forEach(), .map(), .filter(), or .reduce() instead', | ||
| Line, Column); | ||
|
|
||
| Consume(gttLeftParen, 'Expected "(" after "while"'); | ||
| Condition := Expression; | ||
| while not Check(gttRightParen) and not IsAtEnd do | ||
| Advance; | ||
| Consume(gttRightParen, 'Expected ")" after while condition'); | ||
|
|
||
| Body := Statement; | ||
| SkipStatementOrBlock; | ||
|
|
||
| Result := TGocciaWhileStatement.Create(Condition, Body, Line, Column); | ||
| Result := TGocciaEmptyStatement.Create(Line, Column); | ||
| end; | ||
|
|
||
| function TGocciaParser.DoWhileStatement: TGocciaStatement; | ||
| var | ||
| Body: TGocciaStatement; | ||
| Condition: TGocciaExpression; | ||
| Line, Column: Integer; | ||
| begin | ||
| Line := Previous.Line; | ||
| Column := Previous.Column; | ||
|
|
||
| // Parse body first | ||
| Body := Statement; | ||
| AddWarning('''do...while'' loops are not supported in GocciaScript', | ||
| 'Use array methods like .forEach(), .map(), .filter(), or .reduce() instead', | ||
| Line, Column); | ||
|
|
||
| // Expect 'while' keyword | ||
| Consume(gttWhile, 'Expected "while" after do body'); | ||
| SkipStatementOrBlock; | ||
|
|
||
| // Parse condition | ||
| Consume(gttWhile, 'Expected "while" after do body'); | ||
| Consume(gttLeftParen, 'Expected "(" after "while"'); | ||
| Condition := Expression; | ||
| while not Check(gttRightParen) and not IsAtEnd do | ||
| Advance; | ||
| Consume(gttRightParen, 'Expected ")" after do-while condition'); | ||
|
|
||
| // Expect semicolon | ||
| Consume(gttSemicolon, 'Expected ";" after do-while statement'); | ||
|
|
||
| Result := TGocciaDoWhileStatement.Create(Body, Condition, Line, Column); | ||
| Result := TGocciaEmptyStatement.Create(Line, Column); | ||
| end; | ||
|
|
||
| function TGocciaParser.WithStatement: TGocciaStatement; | ||
| var | ||
| Line, Column: Integer; | ||
| begin | ||
| Line := Previous.Line; | ||
| Column := Previous.Column; | ||
|
|
||
| AddWarning('The ''with'' statement is not supported in GocciaScript', '', | ||
| Line, Column); | ||
|
|
||
| Consume(gttLeftParen, 'Expected "(" after "with"'); | ||
| while not Check(gttRightParen) and not IsAtEnd do | ||
| Advance; | ||
| Consume(gttRightParen, 'Expected ")" after with expression'); | ||
|
|
||
| SkipStatementOrBlock; | ||
|
|
||
| Result := TGocciaEmptyStatement.Create(Line, Column); | ||
| end; |
Contributor
There was a problem hiding this comment.
Skip logic can desync on nested parens and compound bodies.
Line 1407-1455 and Line 1471-1474 scan only to the first ), so nested parentheses in loop/with conditions can stop early. Line 1372-1378 uses SkipUntilSemicolon for non-block bodies, which over-skips if/else, try, switch, etc., and can mis-parse the remainder of the file when unsupported constructs appear. Consider a balanced-paren skip helper and parsing a single statement body (then freeing it) for reliable recovery.
🛠️ Possible fix
@@
- procedure SkipBlock;
- procedure SkipStatementOrBlock;
+ procedure SkipBlock;
+ procedure SkipParenGroup;
+ procedure SkipStatementOrBlock;
@@
+procedure TGocciaParser.SkipParenGroup;
+var
+ Depth: Integer;
+begin
+ Consume(gttLeftParen, 'Expected "("');
+ Depth := 1;
+ while not IsAtEnd and (Depth > 0) do
+ begin
+ if Check(gttLeftParen) then Inc(Depth)
+ else if Check(gttRightParen) then Dec(Depth);
+ if Depth > 0 then Advance;
+ end;
+ Consume(gttRightParen, 'Expected ")"');
+end;
+
procedure TGocciaParser.SkipStatementOrBlock;
+var
+ Stmt: TGocciaStatement;
begin
if Check(gttLeftBrace) then
SkipBlock
else
- SkipUntilSemicolon;
+ begin
+ Stmt := Statement;
+ Stmt.Free;
+ end;
end;
@@
- Consume(gttLeftParen, 'Expected "(" after "for"');
- while not Check(gttRightParen) and not IsAtEnd do
- Advance;
- Consume(gttRightParen, 'Expected ")" after for clauses');
+ SkipParenGroup;
@@
- Consume(gttLeftParen, 'Expected "(" after "while"');
- while not Check(gttRightParen) and not IsAtEnd do
- Advance;
- Consume(gttRightParen, 'Expected ")" after while condition');
+ SkipParenGroup;
@@
- Consume(gttLeftParen, 'Expected "(" after "while"');
- while not Check(gttRightParen) and not IsAtEnd do
- Advance;
- Consume(gttRightParen, 'Expected ")" after do-while condition');
+ SkipParenGroup;
@@
- Consume(gttLeftParen, 'Expected "(" after "with"');
- while not Check(gttRightParen) and not IsAtEnd do
- Advance;
- Consume(gttRightParen, 'Expected ")" after with expression');
+ SkipParenGroup;Also applies to: 1215-1216
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@units/Goccia.Parser.pas` around lines 1357 - 1479, Skip logic in
SkipBlock/SkipStatementOrBlock and in
VarStatement/ForStatement/WhileStatement/DoWhileStatement/WithStatement
currently scans to the first ')' or uses SkipUntilSemicolon which can desync on
nested parentheses and compound bodies; replace the ad-hoc scans by (1) adding a
balanced-paren helper (e.g. SkipBalancedParens) used by
ForStatement/WhileStatement/DoWhileStatement/WithStatement instead of looping
until the first gttRightParen, and (2) change SkipStatementOrBlock to actually
parse a single statement body (call the existing statement parsing routine to
build the statement and then free it) rather than calling SkipUntilSemicolon, so
recovery handles nested blocks, if/else/try/switch correctly. Ensure SkipBlock
remains balanced for braces and use these helpers from the referenced methods to
avoid over-skipping.
Comment on lines
+38
to
+57
| procedure Initialize; | ||
| var | ||
| Describe: string; | ||
| DashPos: SizeInt; | ||
| begin | ||
| Describe := RunGit(['describe', '--tags', '--always']); | ||
| CachedCommit := RunGit(['rev-parse', '--short', 'HEAD']); | ||
|
|
||
| if (Describe = '') or not LooksLikeSemver(Describe) then | ||
| begin | ||
| CachedVersion := '0.0.0-dev'; | ||
| Exit; | ||
| end; | ||
|
|
||
| DashPos := Pos('-', Describe); | ||
| if DashPos > 0 then | ||
| CachedVersion := Copy(Describe, 1, DashPos - 1) + '-dev' | ||
| else | ||
| CachedVersion := Describe; | ||
| end; |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Search for test references to commit.length and git-related tests
find . -name "*.js" -o -name "*.pas" | head -20
rg -n "commit.length" --type js
rg -n "commit" tests/ -A 3 -B 3 | head -100Repository: frostney/GocciaScript
Length of output: 1849
🏁 Script executed:
# Check the GetCommit function implementation
rg -n "function GetCommit\|procedure GetCommit" units/Goccia.Version.pas -A 5Repository: frostney/GocciaScript
Length of output: 47
🏁 Script executed:
# Check RunGit implementation to see what it returns when git is unavailable
rg -n "function RunGit\|procedure RunGit" units/ -A 10Repository: frostney/GocciaScript
Length of output: 47
🏁 Script executed:
# Search for GetCommit and RunGit without strict function syntax
rg -n "GetCommit\|RunGit" units/Goccia.Version.pas -B 2 -A 5Repository: frostney/GocciaScript
Length of output: 47
🏁 Script executed:
# Get the full content of Goccia.Version.pas
cat units/Goccia.Version.pasRepository: frostney/GocciaScript
Length of output: 1280
🏁 Script executed:
# Search for where GocciaScript.commit is set from
rg -n "GocciaScript.*commit\|GetCommit" units/ --type pas -B 2 -A 2Repository: frostney/GocciaScript
Length of output: 92
🏁 Script executed:
# Search for where GetCommit is used to set GocciaScript.commit
rg -n "GetCommit\|GocciaScript.*commit" units/ -B 2 -A 2Repository: frostney/GocciaScript
Length of output: 47
🏁 Script executed:
# Check for any CI configuration that might run in git environments
ls -la | grep -E "\.yml|\.yaml|github|gitlab|drone"
cat .github/workflows/*.yml 2>/dev/null | head -50Repository: frostney/GocciaScript
Length of output: 1385
🏁 Script executed:
# Check if there's any handling for non-git environments in tests or build
rg -n "git|commit|version" build.pas -B 2 -A 2Repository: frostney/GocciaScript
Length of output: 298
Add fallback for empty commit hash in non-git environments.
The test in tests/language/global-properties/gocciascript.js (lines 48–49) expects GocciaScript.commit.length > 0, but when git is unavailable or the directory is not a git repository, CachedCommit will be an empty string while CachedVersion falls back to "0.0.0-dev". This causes the test assertion to fail.
Add a fallback for the commit hash:
Fix empty commit fallback
if (Describe = '') or not LooksLikeSemver(Describe) then
begin
CachedVersion := '0.0.0-dev';
+ if CachedCommit = '' then
+ CachedCommit := 'unknown';
Exit;
end;
+
+ if CachedCommit = '' then
+ CachedCommit := 'unknown';📝 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.
Suggested change
| procedure Initialize; | |
| var | |
| Describe: string; | |
| DashPos: SizeInt; | |
| begin | |
| Describe := RunGit(['describe', '--tags', '--always']); | |
| CachedCommit := RunGit(['rev-parse', '--short', 'HEAD']); | |
| if (Describe = '') or not LooksLikeSemver(Describe) then | |
| begin | |
| CachedVersion := '0.0.0-dev'; | |
| Exit; | |
| end; | |
| DashPos := Pos('-', Describe); | |
| if DashPos > 0 then | |
| CachedVersion := Copy(Describe, 1, DashPos - 1) + '-dev' | |
| else | |
| CachedVersion := Describe; | |
| end; | |
| procedure Initialize; | |
| var | |
| Describe: string; | |
| DashPos: SizeInt; | |
| begin | |
| Describe := RunGit(['describe', '--tags', '--always']); | |
| CachedCommit := RunGit(['rev-parse', '--short', 'HEAD']); | |
| if (Describe = '') or not LooksLikeSemver(Describe) then | |
| begin | |
| CachedVersion := '0.0.0-dev'; | |
| if CachedCommit = '' then | |
| CachedCommit := 'unknown'; | |
| Exit; | |
| end; | |
| if CachedCommit = '' then | |
| CachedCommit := 'unknown'; | |
| DashPos := Pos('-', Describe); | |
| if DashPos > 0 then | |
| CachedVersion := Copy(Describe, 1, DashPos - 1) + '-dev' | |
| else | |
| CachedVersion := Describe; | |
| end; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@units/Goccia.Version.pas` around lines 38 - 57, In Initialize, after calling
RunGit to set CachedCommit, add a fallback when CachedCommit is empty (non-git
or git unavailable) by assigning a non-empty placeholder (e.g. 'unknown' or
'0000000') so tests expecting GocciaScript.commit.length > 0 pass; update the
procedure that calls RunGit(['rev-parse', '--short', 'HEAD']) and set
CachedCommit to the placeholder if it returns an empty string, leaving the
existing CachedVersion logic unchanged.
This was referenced Feb 19, 2026
Merged
Merged
This was referenced Mar 7, 2026
This was referenced Mar 27, 2026
This was referenced Apr 5, 2026
Merged
This was referenced Apr 11, 2026
This was referenced Apr 18, 2026
This was referenced Apr 28, 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 by CodeRabbit
Release Notes
New Features
globalThisglobal object that references all current bindingsGocciaScriptglobal object exposing version, commit hash, and enabled built-inswithstatement) instead of failing silentlyImprovements