Disable runtime checks for production builds

[frostney]

Summary by CodeRabbit

• New Features

  • Added an array utility to set/create array entries, auto-expanding as needed.

• Chores / Refactor

  • Standardized collection types across the codebase and updated related APIs and call sites.
  • Inverted production build guard to control runtime checks.

• Documentation

  • Expanded style guidance and embedding docs, including recommendations for list usage and naming.

• Tests

  • Updated test runner collection handling and cleanup behavior.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

---

📝 Walkthrough

Walkthrough

Adds a new array utility procedure to create/assign array elements with automatic expansion; introduces TGocciaValueList and TGocciaScopeList aliases and migrates many internal collections to use them; updates compiler directive conditional in Goccia.inc; and extends documentation and style guidance.

Changes

Cohort / File(s)	Summary
Array Utility units/Goccia.Utils.Array.pas	New exported ArrayCreateDataProperty(const AArray: TGocciaArrayValue; const AIndex: Integer; const AValue: TGocciaValue); inline; that inserts a value at an index, expanding AArray.Elements with nils as needed and raising range errors for negative indices.
List aliases units/Goccia.Values.Primitives.pas, units/Goccia.Scope.pas	Adds public type aliases TGocciaValueList = TObjectList<TGocciaValue> and TGocciaScopeList = TObjectList<TGocciaScope>.
Migrate collections to named lists units/Goccia.Arguments.Collection.pas, units/Goccia.GarbageCollector.pas, units/Goccia.Values.ArrayValue.pas, units/Goccia.Values.FunctionBase.pas, units/Goccia.Values.SetValue.pas, units/TestRunner.pas, units/Goccia.Evaluator.pas	Replaces prior generic list types with the new named aliases for fields, properties, constructor calls, and method signatures; updates initializations (often Create(False)) and adapts call sites/local variables accordingly.
Parser AST list typing units/Goccia.Parser.pas	Changes statements collection type from TObjectList<TGocciaStatement> to TObjectList<TGocciaASTNode> and removes casts when passing the list to TGocciaBlockStatement.Create.
Compiler directives units/Goccia.inc	Inverts the conditional to use {$IFDEF PRODUCTION} to disable overflow/range/object checks and certain diagnostics in production builds, preserving checks when PRODUCTION is not defined.
Documentation & style AGENTS.md, docs/code-style.md, docs/embedding.md	Adds guidance favoring TObjectList<T> for class types, recommends single exported type aliases (examples and rationale), naming conventions and formatting rules; updates embedding docs to reference TGocciaValueList for TGocciaArrayValue.Elements.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

• Add iterators #20 — touches TGocciaArrayValue element storage/handling and is likely related to the Elements -> TGocciaValueList migration and array manipulation utilities.

Poem

🐰 I hop through lists both short and tall,
I fill the gaps when indices call.
A nil, a bump, then AValue lands—hooray!
Aliases tidy every play.
Carrots of code to nibble today. 🥕

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 inconclusive)

Check name	Status	Explanation	Resolution
Title check	❓ Inconclusive	The PR title 'Disable runtime checks for production builds' accurately reflects the primary change in Goccia.inc, but the changeset includes extensive refactoring of list types across multiple units that is not captured by this title.	Consider if the title should reflect the full scope of changes, which includes widespread adoption of named type aliases (TGocciaValueList, TGocciaScopeList) and documentation updates, or clarify whether the production build optimization is the main focus.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
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 unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch feat-disable-runtime-checks-on-prod

---

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

[github-actions]

Benchmark Results

179 benchmarks · 🟢 5 improved · 174 unchanged · avg +1.9%

arrays.js — 19 unchanged · avg -0.2%

Benchmark	Base (ops/sec)	PR (ops/sec)	Change
Array.from length 100	5,003	4,939	-1.3%
Array.from 10 elements	126,299	126,781	+0.4%
Array.of 10 elements	155,288	154,000	-0.8%
spread into new array	162,747	163,721	+0.6%
map over 50 elements	4,814	4,808	-0.1%
filter over 50 elements	4,495	4,477	-0.4%
reduce sum 50 elements	4,667	4,647	-0.4%
forEach over 50 elements	4,242	4,250	+0.2%
find in 50 elements	4,975	4,883	-1.8%
sort 20 elements	4,228	4,200	-0.7%
flat nested array	69,515	67,565	-2.8%
flatMap	36,896	37,247	+1.0%
map inside map (5x5)	9,846	9,766	-0.8%
filter inside map (5x10)	2,875	2,867	-0.3%
reduce inside map (5x10)	2,995	3,004	+0.3%
forEach inside forEach (5x10)	2,751	2,818	+2.4%
find inside some (10x10)	1,678	1,668	-0.6%
map+filter chain nested (5x20)	1,177	1,158	-1.6%
reduce flatten (10x5)	3,403	3,495	+2.7%

classes.js — 15 unchanged · avg -0.4%

Benchmark	Base (ops/sec)	PR (ops/sec)	Change
simple class new	68,237	68,131	-0.2%
class with defaults	52,687	52,920	+0.4%
50 instances via Array.from	2,500	2,533	+1.3%
instance method call	32,561	32,481	-0.2%
static method call	54,591	54,424	-0.3%
single-level inheritance	27,077	26,567	-1.9%
two-level inheritance	24,553	24,369	-0.7%
private field access	34,896	34,284	-1.8%
private methods	40,343	39,876	-1.2%
getter/setter access	36,961	36,745	-0.6%
static getter read	67,674	67,642	-0.0%
static getter/setter pair	47,788	47,948	+0.3%
inherited static getter	40,530	40,475	-0.1%
inherited static setter	43,271	42,913	-0.8%
inherited static getter with this binding	34,172	34,134	-0.1%

closures.js — 11 unchanged · avg -0.5%

Benchmark	Base (ops/sec)	PR (ops/sec)	Change
closure over single variable	56,906	56,496	-0.7%
closure over multiple variables	56,873	56,617	-0.5%
nested closures	61,895	60,966	-1.5%
function as argument	43,066	42,694	-0.9%
function returning function	56,167	56,263	+0.2%
compose two functions	35,039	34,602	-1.2%
fn.call	86,836	85,526	-1.5%
fn.apply	59,971	60,615	+1.1%
fn.bind	70,880	70,842	-0.1%
recursive sum to 50	4,924	4,949	+0.5%
recursive tree traversal	8,710	8,642	-0.8%

collections.js — 12 unchanged · avg +0.5%

Benchmark	Base (ops/sec)	PR (ops/sec)	Change
add 50 elements	3,241	3,199	-1.3%
has lookup (50 elements)	5,589	5,533	-1.0%
delete elements	14,466	14,371	-0.7%
forEach iteration	3,195	3,158	-1.1%
spread to array	6,345	6,372	+0.4%
deduplicate array	24,397	24,635	+1.0%
set 50 entries	2,620	2,600	-0.7%
get lookup (50 entries)	3,077	3,161	+2.7%
has check	3,135	3,194	+1.9%
delete entries	8,179	8,265	+1.0%
forEach iteration	2,173	2,184	+0.5%
keys/values/entries	2,465	2,537	+2.9%

destructuring.js — 14 unchanged · avg +0.7%

Benchmark	Base (ops/sec)	PR (ops/sec)	Change
simple array destructuring	183,028	179,713	-1.8%
with rest element	133,730	134,548	+0.6%
with defaults	183,841	182,336	-0.8%
skip elements	196,876	193,640	-1.6%
nested array destructuring	105,139	103,667	-1.4%
swap variables	207,858	211,962	+2.0%
simple object destructuring	139,420	137,452	-1.4%
with defaults	168,200	169,471	+0.8%
with renaming	165,653	164,677	-0.6%
nested object destructuring	82,221	83,861	+2.0%
rest properties	86,188	88,023	+2.1%
object parameter	51,430	52,945	+2.9%
array parameter	63,804	66,234	+3.8%
mixed destructuring in map	5,855	6,051	+3.4%

fibonacci.js — 6 unchanged · avg +0.5%

Benchmark	Base (ops/sec)	PR (ops/sec)	Change
recursive fib(15)	133	134	+0.4%
recursive fib(20)	12	12	-1.2%
iterative fib(20) via reduce	6,849	7,030	+2.6%
iterator fib(20)	3,876	3,927	+1.3%
iterator fib(20) via Iterator.from + take	4,030	4,035	+0.1%
iterator fib(20) last value via reduce	3,343	3,340	-0.1%

iterators.js — 20 unchanged · avg +2.2%

Benchmark	Base (ops/sec)	PR (ops/sec)	Change
Iterator.from({next}).toArray() — 20 elements	5,273	5,307	+0.6%
Iterator.from({next}).toArray() — 50 elements	2,239	2,256	+0.8%
spread pre-wrapped iterator — 20 elements	5,186	5,332	+2.8%
Iterator.from({next}).forEach — 50 elements	1,665	1,718	+3.2%
Iterator.from({next}).reduce — 50 elements	1,679	1,709	+1.8%
wrap array iterator	31,370	31,596	+0.7%
wrap plain {next()} object	3,586	3,662	+2.1%
map + toArray (50 elements)	1,438	1,478	+2.7%
filter + toArray (50 elements)	1,526	1,556	+1.9%
take(10) + toArray (50 element source)	8,263	8,656	+4.8%
drop(40) + toArray (50 element source)	2,135	2,179	+2.1%
chained map + filter + take (100 element source)	2,582	2,649	+2.6%
some + every (50 elements)	961	980	+2.0%
find (50 elements)	2,055	2,163	+5.3%
array.values().map().filter().toArray()	1,515	1,555	+2.6%
array.values().take(5).toArray()	7,376	7,381	+0.1%
array.values().drop(45).toArray()	4,541	4,595	+1.2%
map.entries() chained helpers	1,903	1,961	+3.1%
set.values() chained helpers	3,037	3,111	+2.4%
string iterator map + toArray	4,323	4,353	+0.7%

json.js — 20 unchanged · avg +2.6%

Benchmark	Base (ops/sec)	PR (ops/sec)	Change
parse simple object	112,282	117,900	+5.0%
parse nested object	72,859	74,714	+2.5%
parse array of objects	40,890	42,604	+4.2%
parse large flat object	36,103	36,783	+1.9%
parse mixed types	51,559	54,805	+6.3%
stringify simple object	98,298	99,901	+1.6%
stringify nested object	56,095	56,630	+1.0%
stringify array of objects	10,956	11,074	+1.1%
stringify mixed types	46,672	48,119	+3.1%
reviver doubles numbers	22,398	22,964	+2.5%
reviver filters properties	21,120	21,300	+0.9%
reviver on nested object	27,184	27,205	+0.1%
reviver on array	15,606	15,979	+2.4%
replacer function doubles numbers	21,344	21,715	+1.7%
replacer function excludes properties	28,603	28,761	+0.5%
array replacer (allowlist)	58,714	61,395	+4.6%
stringify with 2-space indent	54,008	56,011	+3.7%
stringify with tab indent	55,505	57,080	+2.8%
parse then stringify	34,983	36,099	+3.2%
stringify then parse	11,945	12,308	+3.0%

jsx.jsx — 🟢 3 improved, 18 unchanged · avg +3.5%

Benchmark	Base (ops/sec)	PR (ops/sec)	Change
simple element	114,243	116,489	+2.0%
self-closing element	117,815	119,303	+1.3%
element with string attribute	97,910	100,465	+2.6%
element with multiple attributes	84,894	86,872	+2.3%
element with expression attribute	87,569	89,604	+2.3%
text child	113,679	116,976	+2.9%
expression child	107,260	112,207	+4.6%
mixed text and expression	102,832	107,325	+4.4%
nested elements (3 levels)	43,208	44,693	+3.4%
sibling children	32,579	33,299	+2.2%
component element	81,759	83,991	+2.7%
component with children	50,673	51,615	+1.9%
dotted component	69,254	69,876	+0.9%
empty fragment	109,737	112,814	+2.8%
fragment with children	32,082	33,371	+4.0%
spread attributes	61,108	63,742	+4.3%
spread with overrides	52,934	57,002	🟢 +7.7%
shorthand props	82,067	87,864	🟢 +7.1%
nav bar structure	15,319	15,860	+3.5%
card component tree	18,297	18,689	+2.1%
10 list items via Array.from	7,624	8,202	🟢 +7.6%

numbers.js — 11 unchanged · avg +2.4%

Benchmark	Base (ops/sec)	PR (ops/sec)	Change
integer arithmetic	184,349	185,767	+0.8%
floating point arithmetic	207,868	209,308	+0.7%
number coercion	104,400	104,541	+0.1%
toFixed	70,100	74,311	+6.0%
toString	94,878	98,458	+3.8%
valueOf	122,336	130,781	+6.9%
toPrecision	89,217	94,294	+5.7%
Number.isNaN	156,377	156,322	-0.0%
Number.isFinite	149,475	149,760	+0.2%
Number.isInteger	153,480	155,438	+1.3%
Number.parseInt and parseFloat	142,451	143,376	+0.6%

objects.js — 🟢 1 improved, 6 unchanged · avg +4.1%

Benchmark	Base (ops/sec)	PR (ops/sec)	Change
create simple object	224,978	234,876	+4.4%
create nested object	117,495	123,985	+5.5%
create 50 objects via Array.from	4,504	4,656	+3.4%
property read	103,974	105,160	+1.1%
Object.keys	73,766	77,670	+5.3%
Object.entries	47,920	48,844	+1.9%
spread operator	90,350	96,794	🟢 +7.1%

promises.js — 12 unchanged · avg +5.1%

Benchmark	Base (ops/sec)	PR (ops/sec)	Change
Promise.resolve(value)	301,493	313,487	+4.0%
new Promise(resolve => resolve(value))	117,350	120,797	+2.9%
Promise.reject(reason)	312,169	326,397	+4.6%
resolve + then (1 handler)	100,661	105,179	+4.5%
resolve + then chain (3 deep)	42,287	45,086	+6.6%
resolve + then chain (10 deep)	14,110	14,723	+4.3%
reject + catch + then	62,615	65,801	+5.1%
resolve + finally + then	54,740	57,164	+4.4%
Promise.all (5 resolved)	21,874	23,319	+6.6%
Promise.race (5 resolved)	23,130	24,403	+5.5%
Promise.allSettled (5 mixed)	18,005	19,159	+6.4%
Promise.any (5 mixed)	21,675	22,996	+6.1%

strings.js — 🟢 1 improved, 10 unchanged · avg +5.6%

Benchmark	Base (ops/sec)	PR (ops/sec)	Change
string concatenation	274,512	276,573	+0.8%
template literal	250,697	257,743	+2.8%
string repeat	233,131	247,083	+6.0%
split and join	100,102	102,875	+2.8%
indexOf and includes	101,203	107,962	+6.7%
toUpperCase and toLowerCase	146,562	155,604	+6.2%
slice and substring	89,601	95,447	+6.5%
trim operations	108,291	115,499	+6.7%
replace and replaceAll	128,881	137,498	+6.7%
startsWith and endsWith	79,357	87,723	🟢 +10.5%
padStart and padEnd	120,219	126,812	+5.5%

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

[coderabbitai]

Actionable comments posted: 2

🤖 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.inc`:
- Around line 2-11: In Goccia.inc the {$ELSE} branch re-enables only
{$overflowchecks} and {$rangechecks} but leaves {$S}, {$OBJECTCHECKS} and {$C}
disabled; update the non-PRODUCTION branch to explicitly re-enable stack
checking and object checks and turn assertions back on by adding the
corresponding compiler directives (e.g. {$S+} or {$S ON}, {$OBJECTCHECKS ON} (or
{$OBJECTCHECKS+}), and {$C+}) in the {$ELSE} section so debug builds have the
intended stricter checking.

In `@units/Goccia.Utils.Array.pas`:
- Around line 7-25: Add a non-negative index check to ArrayCreateDataProperty to
guard against negative AIndex values: import Goccia.Values.ErrorHelper in the
uses clause and at the start of ArrayCreateDataProperty (procedure
ArrayCreateDataProperty(const AArray: TGocciaArrayValue; const AIndex: Integer;
const AValue: TGocciaValue)) validate that AIndex >= 0 and call
Goccia.Values.ErrorHelper.ThrowRangeError() when it is negative before
proceeding with AArray.Elements manipulation; this uses ThrowRangeError instead
of ERangeError per project guidelines.

[coderabbitai]

Actionable comments posted: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

units/Goccia.Values.FunctionBase.pas (1)

258-263: ⚠️ Potential issue | 🟠 Major

Memory leak: BoundArgs is never freed.

BoundArgs is created at line 258, its elements are copied into the new TGocciaBoundFunctionValue at line 275-279, but the original BoundArgs list is never freed. The constructor creates its own copy, so the caller should free the original.

🐛 Proposed fix to free BoundArgs

   // Remaining arguments are pre-filled arguments
   BoundArgs := TGocciaValueList.Create(False);
   for I := 1 to AArgs.Length - 1 do
     BoundArgs.Add(AArgs.GetElement(I));

   // Create and return a bound function
   Result := TGocciaBoundFunctionValue.Create(AThisValue, BoundThis, BoundArgs);
+  BoundArgs.Free;
 end;

🤖 Prompt for AI Agents

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

In `@units/Goccia.Values.FunctionBase.pas` around lines 258 - 263, BoundArgs is
allocated with TGocciaValueList.Create but never freed causing a memory leak;
after constructing the bound function with
TGocciaBoundFunctionValue.Create(AThisValue, BoundThis, BoundArgs) free the
original BoundArgs (e.g., call BoundArgs.Free or FreeAndNil(BoundArgs)) since
the constructor makes its own copy, ensuring you free BoundArgs in all paths
(use try..finally if needed).

🧹 Nitpick comments (1)

units/Goccia.Evaluator.pas (1)

248-286: New helper SpreadIterableIntoArgs for arguments collection.

The new procedure duplicates SpreadIterableInto logic but targets TGocciaArgumentsCollection instead of TGocciaValueList. While this creates code duplication, it's acceptable given the different target interfaces. Consider extracting the common iterator logic if more spread targets are added in the future.

🤖 Prompt for AI Agents

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

In `@units/Goccia.Evaluator.pas` around lines 248 - 286, The new
SpreadIterableIntoArgs duplicates the iterator/traversal logic present in
SpreadIterableInto; refactor by extracting the common iteration behavior into a
shared helper (e.g., IterateAndAddValues) that accepts an iterator and a
callback to add each yielded value so both SpreadIterableInto and
SpreadIterableIntoArgs call GetIteratorFromValue, use TGocciaGarbageCollector to
AddTempRoot/RemoveTempRoot, advance via AdvanceNext and check
PROP_DONE/PROP_VALUE, and invoke the callback to add into either
TGocciaArgumentsCollection or TGocciaValueList; keep ThrowTypeError('Spread
syntax requires an iterable') where iterator is nil.

🤖 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.Evaluator.pas`:
- Line 317: Remove the unused local variable Arguments: TGocciaValueList
declared in the EvaluateExpression routine; locate the EvaluateExpression
function and delete the Arguments declaration (and any related unused
references) so the function no longer contains the unused TGocciaValueList
variable.

In `@units/Goccia.Values.FunctionBase.pas`:
- Line 159: The declared but unused variable CallArgs of type TGocciaValueList
in the FunctionCall implementation should be removed; locate the CallArgs
declaration in the FunctionCall routine (symbol: CallArgs) and delete it so
there are no unused local variables left, and run the compiler to confirm no
remaining references to TGocciaValueList in that scope remain.

---

Outside diff comments:
In `@units/Goccia.Values.FunctionBase.pas`:
- Around line 258-263: BoundArgs is allocated with TGocciaValueList.Create but
never freed causing a memory leak; after constructing the bound function with
TGocciaBoundFunctionValue.Create(AThisValue, BoundThis, BoundArgs) free the
original BoundArgs (e.g., call BoundArgs.Free or FreeAndNil(BoundArgs)) since
the constructor makes its own copy, ensuring you free BoundArgs in all paths
(use try..finally if needed).

---

Nitpick comments:
In `@units/Goccia.Evaluator.pas`:
- Around line 248-286: The new SpreadIterableIntoArgs duplicates the
iterator/traversal logic present in SpreadIterableInto; refactor by extracting
the common iteration behavior into a shared helper (e.g., IterateAndAddValues)
that accepts an iterator and a callback to add each yielded value so both
SpreadIterableInto and SpreadIterableIntoArgs call GetIteratorFromValue, use
TGocciaGarbageCollector to AddTempRoot/RemoveTempRoot, advance via AdvanceNext
and check PROP_DONE/PROP_VALUE, and invoke the callback to add into either
TGocciaArgumentsCollection or TGocciaValueList; keep ThrowTypeError('Spread
syntax requires an iterable') where iterator is nil.