Fix edge cases for special values

[frostney]

Summary by CodeRabbit

• Tests

  • Extended test coverage for Number.isFinite() and Number.isInteger() with additional edge cases.
  • Enhanced String.charAt() and String.charCodeAt() tests, including Symbol argument handling and Infinity edge cases.

• Bug Fixes

  • Improved type checking and validation for numeric operations.
  • Enhanced resource management for garbage collection scenarios.

[coderabbitai]

📝 Walkthrough

Walkthrough

This PR adds comprehensive test cases for Number and String built-in methods while implementing corresponding type-guard improvements and garbage collection safeguards in the Goccia runtime to handle edge cases such as Symbol arguments, non-number inputs, and improved resource management during object construction.

Changes

Cohort / File(s)	Summary
Number Built-in Tests tests/built-ins/Number/isFinite.js, tests/built-ins/Number/isInteger.js	Extended test coverage with new assertions for non-number inputs (objects, arrays, strings, booleans, null, undefined), missing arguments, edge cases (Infinity, -Infinity), and reorganized test blocks for clarity.
String Method Tests tests/built-ins/String/prototype/charAt.js, tests/built-ins/String/prototype/charCodeAt.js	Added assertions for Symbol argument handling (TypeErrors), Infinity/-Infinity edge cases, and removed clarifying comments while preserving core behavior verification.
Number Implementation units/Goccia.Builtins.GlobalNumber.pas	Reordered type checks to guard against invalid casts before attempting to access TGocciaNumberLiteralValue, preventing type errors when non-numeric arguments are provided.
String Implementation units/Goccia.Builtins.GlobalString.pas, units/Goccia.Values.StringObjectValue.pas	Refactored string method index conversion with centralized handling for undefined/null/NaN defaults and Infinity bounds; simplified charAt/charCodeAt logic with consistent coercion path.
Garbage Collection & Resource Management units/Goccia.SharedPrototype.pas, units/Goccia.Values.ClassValue.pas	Added GarbageCollector dependency and implemented nil-checks and temporary GC root protection to safeguard object pinning during construction and destructor cleanup.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

• Fix test suites for further ECMAScript compatibility #5: Modifies string operation handling and Symbol-related error behavior in String methods, directly related to the refactored string coercion logic and Symbol TypeError tests.

Poem

🐰 With whiskers twitching, I hop through your code,
Testing each number and string on the road,
Symbol-catching and garbage cleaned neat,
Type guards and GC make the logic complete! ✨

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'Fix edge cases for special values' accurately reflects the main focus of the changeset, which addresses edge case handling for special numeric and type values across multiple test and implementation files.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch fix-edge-cases

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

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.Values.ClassValue.pas`:
- Around line 425-432: TGocciaGC.Instance can be nil, so wrap the temporary root
calls with an Assigned check: before calling
TGocciaGC.Instance.AddTempRoot(Instance) and before
TGocciaGC.Instance.RemoveTempRoot(Instance) in the constructor invocation block
(around ConstructorToCall.Call(AArguments, Instance)), guard both calls with if
Assigned(TGocciaGC.Instance) then ...; apply the same defensive pattern to other
unguarded sites that call AddTempRoot/RemoveTempRoot such as Engine.Destroy,
Builtins.Benchmark/Globals, Builtins.GlobalPromise, MicrotaskQueue and
Goccia.Scope to avoid nil dereferences.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

# First, find the TGocciaGC class definition and how Instance is implemented
fd -e pas | xargs grep -l "TGocciaGC" | head -20

Repository: frostney/GocciaScript

Length of output: 752

---

🏁 Script executed:

# Find TGocciaGC class definition and Instance property
rg -n "class TGocciaGC|class var.*Instance|property Instance" --type=pas -A 3

Repository: frostney/GocciaScript

Length of output: 92

---

🏁 Script executed:

# Check how and when TGocciaGC is initialized
rg -n "TGocciaGC\.Create|InitializeGC|Setup|Initialize.*GC" --type=pas -B 2 -A 5

Repository: frostney/GocciaScript

Length of output: 92

---

🏁 Script executed:

# Examine the specific file and lines in question
cat -n units/Goccia.Values.ClassValue.pas | sed -n '415,440p'

Repository: frostney/GocciaScript

Length of output: 1067

---

🏁 Script executed:

# Check if TGocciaGC.Instance is guarded anywhere else in the codebase
rg -n "if Assigned.*TGocciaGC|if.*TGocciaGC.*nil" --type=pas

Repository: frostney/GocciaScript

Length of output: 92

---

🏁 Script executed:

# Search for TGocciaGC class definition and Instance property
grep -rn "class TGocciaGC\|property Instance" units/Goccia.GarbageCollector.pas | head -20

Repository: frostney/GocciaScript

Length of output: 47

---

🏁 Script executed:

# Find where TGocciaGC.Instance is used and check for guards
grep -n "TGocciaGC\.Instance" units/*.pas | head -30

Repository: frostney/GocciaScript

Length of output: 2679

---

🏁 Script executed:

# Read the GarbageCollector unit to understand singleton implementation
head -100 units/Goccia.GarbageCollector.pas

Repository: frostney/GocciaScript

Length of output: 2842

---

🏁 Script executed:

# Check Engine initialization to see when GC is set up
grep -n "GarbageCollector\|TGocciaGC" units/Goccia.Engine.pas | head -30

Repository: frostney/GocciaScript

Length of output: 362

---

🏁 Script executed:

# Check if Instantiate can be called before engine initialization
grep -n "\.Instantiate\|TGocciaClassValue" units/Goccia.Engine.pas

Repository: frostney/GocciaScript

Length of output: 436

---

🏁 Script executed:

# Check GlobalPromise unguarded calls for context
sed -n '535,550p' units/Goccia.Builtins.GlobalPromise.pas

Repository: frostney/GocciaScript

Length of output: 635

---

🏁 Script executed:

# Check Engine line 157 and surrounding context
sed -n '150,175p' units/Goccia.Engine.pas

Repository: frostney/GocciaScript

Length of output: 983

---

🏁 Script executed:

# Search for any other unguarded TGocciaGC.Instance calls
grep -n "TGocciaGC\.Instance\." units/*.pas | grep -v "if Assigned\|if not Assigned" | head -20

Repository: frostney/GocciaScript

Length of output: 1865

---

Add Assigned check for TGocciaGC.Instance in AddTempRoot/RemoveTempRoot calls.

TGocciaGC.Instance is a lazy singleton that can be nil. While class instantiation is currently ordered after engine initialization, other parts of the codebase guard these calls (e.g., Engine.Destroy line 170, Builtins.Benchmark, Builtins.Globals, MicrotaskQueue). Lines 426 and 430 should follow the defensive pattern for consistency.

Additionally, this issue exists in other unguarded locations (Engine.pas:157, 197; Builtins.GlobalPromise:539, 544; Goccia.Scope:182) and should be addressed systematically.

🔧 Suggested guard

   begin
-    TGocciaGC.Instance.AddTempRoot(Instance);
+    if Assigned(TGocciaGC.Instance) then
+      TGocciaGC.Instance.AddTempRoot(Instance);
     try
       ConstructorToCall.Call(AArguments, Instance);
     finally
-      TGocciaGC.Instance.RemoveTempRoot(Instance);
+      if Assigned(TGocciaGC.Instance) then
+        TGocciaGC.Instance.RemoveTempRoot(Instance);
     end;
   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

	begin
	TGocciaGC.Instance.AddTempRoot(Instance);
	try
	ConstructorToCall.Call(AArguments, Instance);
	finally
	TGocciaGC.Instance.RemoveTempRoot(Instance);
	end;
	end;
	begin
	if Assigned(TGocciaGC.Instance) then
	TGocciaGC.Instance.AddTempRoot(Instance);
	try
	ConstructorToCall.Call(AArguments, Instance);
	finally
	if Assigned(TGocciaGC.Instance) then
	TGocciaGC.Instance.RemoveTempRoot(Instance);
	end;
	end;

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@units/Goccia.Values.ClassValue.pas` around lines 425 - 432,
TGocciaGC.Instance can be nil, so wrap the temporary root calls with an Assigned
check: before calling TGocciaGC.Instance.AddTempRoot(Instance) and before
TGocciaGC.Instance.RemoveTempRoot(Instance) in the constructor invocation block
(around ConstructorToCall.Call(AArguments, Instance)), guard both calls with if
Assigned(TGocciaGC.Instance) then ...; apply the same defensive pattern to other
unguarded sites that call AddTempRoot/RemoveTempRoot such as Engine.Destroy,
Builtins.Benchmark/Globals, Builtins.GlobalPromise, MicrotaskQueue and
Goccia.Scope to avoid nil dereferences.

[github-actions]

Benchmark Results

113 benchmarks · 113 unchanged · avg +0.7%

arrays.js — 11 unchanged · avg +0.5%

Benchmark	Base (ops/sec)	PR (ops/sec)	Change
Array.from length 100	145,387	145,876	+0.3%
Array.of 10 elements	160,409	161,141	+0.5%
spread into new array	169,977	173,506	+2.1%
map over 50 elements	91,391	92,032	+0.7%
filter over 50 elements	91,278	90,994	-0.3%
reduce sum 50 elements	96,217	96,510	+0.3%
forEach over 50 elements	95,486	95,189	-0.3%
find in 50 elements	102,410	101,684	-0.7%
sort 20 elements	4,091	4,204	+2.8%
flat nested array	80,859	80,724	-0.2%
flatMap	39,799	39,948	+0.4%

classes.js — 10 unchanged · avg -0.2%

Benchmark	Base (ops/sec)	PR (ops/sec)	Change
simple class new	75,395	74,395	-1.3%
class with defaults	57,302	57,032	-0.5%
50 instances via Array.from	79,040	79,450	+0.5%
instance method call	34,671	34,735	+0.2%
static method call	59,651	60,180	+0.9%
single-level inheritance	29,455	29,378	-0.3%
two-level inheritance	27,593	27,315	-1.0%
private field access	37,137	36,792	-0.9%
private methods	42,824	42,577	-0.6%
getter/setter access	38,968	39,195	+0.6%

closures.js — 11 unchanged · avg +1.1%

Benchmark	Base (ops/sec)	PR (ops/sec)	Change
closure over single variable	57,374	57,407	+0.1%
closure over multiple variables	56,833	57,529	+1.2%
nested closures	61,964	62,294	+0.5%
function as argument	43,125	43,919	+1.8%
function returning function	57,015	57,947	+1.6%
compose two functions	35,148	35,664	+1.5%
fn.call	83,556	85,143	+1.9%
fn.apply	61,852	62,438	+0.9%
fn.bind	73,529	73,301	-0.3%
recursive sum to 50	5,030	5,139	+2.2%
recursive tree traversal	8,886	8,970	+0.9%

collections.js — 12 unchanged · avg -0.5%

Benchmark	Base (ops/sec)	PR (ops/sec)	Change
add 50 elements	85,366	84,934	-0.5%
has lookup (50 elements)	67,438	67,363	-0.1%
delete elements	66,972	67,039	+0.1%
forEach iteration	82,930	82,571	-0.4%
spread to array	88,919	86,693	-2.5%
deduplicate array	56,890	56,854	-0.1%
set 50 entries	84,607	84,207	-0.5%
get lookup (50 entries)	65,203	65,384	+0.3%
has check	71,662	71,607	-0.1%
delete entries	65,492	65,167	-0.5%
forEach iteration	78,039	77,016	-1.3%
keys/values/entries	51,705	51,755	+0.1%

destructuring.js — 14 unchanged · avg +1.1%

Benchmark	Base (ops/sec)	PR (ops/sec)	Change
simple array destructuring	182,990	188,092	+2.8%
with rest element	136,440	140,179	+2.7%
with defaults	185,000	188,818	+2.1%
skip elements	199,742	204,571	+2.4%
nested array destructuring	110,264	111,041	+0.7%
swap variables	217,622	218,318	+0.3%
simple object destructuring	138,950	140,411	+1.1%
with defaults	165,141	167,688	+1.5%
with renaming	162,519	163,783	+0.8%
nested object destructuring	84,799	84,674	-0.1%
rest properties	87,440	87,848	+0.5%
object parameter	52,410	52,308	-0.2%
array parameter	67,074	67,485	+0.6%
mixed destructuring in map	89,220	88,855	-0.4%

fibonacci.js — 3 unchanged · avg +1.1%

Benchmark	Base (ops/sec)	PR (ops/sec)	Change
recursive fib(15)	138	141	+1.7%
recursive fib(20)	12	13	+1.8%
iterative fib(20) via reduce	66,441	66,193	-0.4%

json.js — 11 unchanged · avg +1.2%

Benchmark	Base (ops/sec)	PR (ops/sec)	Change
parse simple object	121,802	122,866	+0.9%
parse nested object	75,351	76,759	+1.9%
parse array of objects	48,078	48,259	+0.4%
parse large flat object	50,142	50,916	+1.5%
parse mixed types	64,054	65,274	+1.9%
stringify simple object	99,703	100,925	+1.2%
stringify nested object	56,654	57,613	+1.7%
stringify array of objects	109,768	110,424	+0.6%
stringify mixed types	48,242	48,402	+0.3%
parse then stringify	37,660	38,261	+1.6%
stringify then parse	47,721	48,143	+0.9%

numbers.js — 11 unchanged · avg +1.9%

Benchmark	Base (ops/sec)	PR (ops/sec)	Change
integer arithmetic	178,916	185,695	+3.8%
floating point arithmetic	201,138	207,365	+3.1%
number coercion	106,107	108,611	+2.4%
toFixed	73,171	74,303	+1.5%
toString	100,404	101,634	+1.2%
valueOf	133,284	134,762	+1.1%
toPrecision	94,243	95,360	+1.2%
Number.isNaN	157,932	161,286	+2.1%
Number.isFinite	151,117	155,020	+2.6%
Number.isInteger	155,399	157,783	+1.5%
Number.parseInt and parseFloat	140,944	141,953	+0.7%

objects.js — 7 unchanged · avg +0.5%

Benchmark	Base (ops/sec)	PR (ops/sec)	Change
create simple object	227,915	229,675	+0.8%
create nested object	121,420	120,679	-0.6%
create 50 objects via Array.from	140,679	142,793	+1.5%
property read	103,497	105,542	+2.0%
Object.keys	72,948	73,218	+0.4%
Object.entries	47,664	47,841	+0.4%
spread operator	93,557	92,690	-0.9%

promises.js — 12 unchanged · avg +0.3%

Benchmark	Base (ops/sec)	PR (ops/sec)	Change
Promise.resolve(value)	307,495	306,345	-0.4%
new Promise(resolve => resolve(value))	121,107	120,720	-0.3%
Promise.reject(reason)	317,624	320,973	+1.1%
resolve + then (1 handler)	104,561	103,504	-1.0%
resolve + then chain (3 deep)	42,882	43,561	+1.6%
resolve + then chain (10 deep)	88,320	88,500	+0.2%
reject + catch + then	63,508	64,471	+1.5%
resolve + finally + then	55,993	56,225	+0.4%
Promise.all (5 resolved)	22,738	22,730	-0.0%
Promise.race (5 resolved)	23,877	23,834	-0.2%
Promise.allSettled (5 mixed)	18,363	18,297	-0.4%
Promise.any (5 mixed)	22,634	22,900	+1.2%

strings.js — 11 unchanged · avg +0.4%

Benchmark	Base (ops/sec)	PR (ops/sec)	Change
string concatenation	250,189	247,175	-1.2%
template literal	245,751	250,120	+1.8%
string repeat	240,388	241,650	+0.5%
split and join	104,568	103,138	-1.4%
indexOf and includes	109,429	109,621	+0.2%
toUpperCase and toLowerCase	157,445	157,658	+0.1%
slice and substring	96,552	96,717	+0.2%
trim operations	115,292	116,559	+1.1%
replace and replaceAll	133,873	137,101	+2.4%
startsWith and endsWith	87,231	87,547	+0.4%
padStart and padEnd	124,460	125,134	+0.5%

_{Measured on ubuntu-latest x64. Changes within ±7% are considered insignificant.}