Fix increment/decrement for computed member expressions by frostney · Pull Request #54 · frostney/GocciaScript

frostney · 2026-03-08T04:45:24Z

Fix increment/decrement for computed member expressions and array elements.

This PR addresses issue #52 by ensuring that increment/decrement operations on computed member expressions (e.g., arr[i]++, obj[key]--) correctly mutate values in both interpreter and bytecode modes. It also fixes a latent bug where DefinePropertyOnValue incorrectly handled array updates, now using AssignProperty to dispatch to the virtual SetProperty method.

Summary by CodeRabbit

Release Notes

Bug Fixes
- Fixed increment and decrement operators on object properties and computed array indices to properly return values and update the target location.
Tests
- Added comprehensive test coverage for pre- and post-increment/decrement operations on object properties, array indices, and computed member access patterns.

- Interpreter: Evaluate PropertyExpression for computed member operands in TGocciaIncrementExpression.Evaluate instead of reading empty PropertyName. Use AssignProperty instead of DefinePropertyOnValue so arrays are updated via virtual SetProperty dispatch. - Compiler: Add member expression support to CompileIncrement — non-computed members use OP_RT_GET_PROP/OP_RT_SET_PROP, computed members use OP_ARRAY_GET/OP_ARRAY_SET. - Tests: Add computed member, property access, and variable-key tests for both increment and decrement operators. Closes #52 Co-authored-by: Johannes Stein <frostney@users.noreply.github.com>

cursor · 2026-03-08T04:45:25Z

Cursor Agent can help with this pull request. Just @cursor in comments and I'll start working on changes in this branch.
_{Learn more about Cursor Agents}

coderabbitai · 2026-03-08T04:45:45Z

Warning

Rate limit exceeded

@frostney has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 1 minutes and 55 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 8fd249bf-bde0-4f0c-8012-df2952f38873

📥 Commits

Reviewing files that changed from the base of the PR and between 66af83d and 76a0308.

📒 Files selected for processing (3)

tests/language/expressions/arithmetic/decrement.js
tests/language/expressions/arithmetic/increment.js
units/Goccia.AST.Expressions.pas

📝 Walkthrough

Walkthrough

These changes extend increment and decrement operators to work with object properties and computed member expressions. New test coverage validates post- and pre-increment/decrement behavior on object properties, array indices, and dynamic keys. Corresponding runtime evaluation and compiler code generation support member expression operands.

Changes

Cohort / File(s)	Summary
Test Coverage for Member Expression Operators `tests/language/expressions/arithmetic/decrement.js`, `tests/language/expressions/arithmetic/increment.js`	Added test suites covering post- and pre-increment/decrement on object properties, computed members with numeric/variable/string keys, validating correct return values and updated state.
Runtime Evaluation `units/Goccia.AST.Expressions.pas`	Extended TGocciaIncrementExpression.Evaluate to handle member expression operands: evaluates the object and property expressions separately, resolves computed vs. non-computed properties, and uses AssignProperty for proper error handling.
Compiler Code Generation `units/Goccia.Compiler.Expressions.pas`	Added CompileIncrementMember and CompileIncrementComputedMember helper procedures for compiling member-based increment operations; extended CompileIncrement to delegate member expressions to appropriate helper procedures.

Sequence Diagram(s)

sequenceDiagram
    participant Source as Source Code
    participant Compiler as Compiler
    participant CodeGen as Code Generator
    participant Runtime as Runtime Engine

    Source->>Compiler: Parse member increment (obj.count++)
    Compiler->>Compiler: Detect TGocciaMemberExpression operand
    alt Computed Member
        Compiler->>CodeGen: CompileIncrementComputedMember
        CodeGen->>CodeGen: Emit get array element opcode
        CodeGen->>CodeGen: Emit increment opcode
        CodeGen->>CodeGen: Emit set array element opcode
    else Non-computed Member
        Compiler->>CodeGen: CompileIncrementMember
        CodeGen->>CodeGen: Emit get property opcode
        CodeGen->>CodeGen: Emit increment opcode
        CodeGen->>CodeGen: Emit set property opcode
    end
    CodeGen-->>Runtime: Compiled bytecode
    Runtime->>Runtime: Execute: evaluate object reference
    Runtime->>Runtime: Execute: retrieve current property value
    Runtime->>Runtime: Execute: increment and store back
    Runtime-->>Source: Return pre/post increment value

sequenceDiagram
    participant Evaluator as AST Evaluator
    participant Object as Object Context
    participant Property as Property Access

    Evaluator->>Evaluator: Detect member expression in increment
    Evaluator->>Object: Evaluate ObjectExpr
    Object-->>Evaluator: Object reference
    Evaluator->>Property: Resolve property name (computed or static)
    Property-->>Evaluator: Property name string
    Evaluator->>Object: Get current property value (OldValue)
    Object-->>Evaluator: Current value
    Evaluator->>Evaluator: Calculate new value (OldValue + 1)
    Evaluator->>Object: AssignProperty(Obj, PropName, NewValue)
    Object-->>Evaluator: Assignment result
    Evaluator-->>Evaluator: Return prefix/postfix result

Estimated Code Review Effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐰 Hop along, expressions bloom—
Member properties now make room,
Increment and decrement flow,
Through objects fast, their values grow!
From test to compiler, all aligned,
A patchwork of operators refined. ✨

🚥 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 increment/decrement for computed member expressions' directly and clearly summarizes the main change: fixing increment/decrement operations on computed member expressions (like arr[i]++ and obj[key]--), which aligns with the PR objectives and all file changes.
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 cursor/script-execution-path-ffd5

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

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

github-actions · 2026-03-08T04:52:19Z

Benchmark Results

254 benchmarks · 🟢 12 improved · 🔴 1 regressed · 241 unchanged · avg +1.4%

arraybuffer.js — 14 unchanged · avg +0.7%

Benchmark	Base (ops/sec)	PR (ops/sec)	Change
create ArrayBuffer(0)	427,926	425,540	-0.6%
create ArrayBuffer(64)	418,490	419,269	+0.2%
create ArrayBuffer(1024)	317,700	321,979	+1.3%
create ArrayBuffer(8192)	135,468	139,747	+3.2%
slice full buffer (64 bytes)	496,868	500,529	+0.7%
slice half buffer (512 of 1024 bytes)	425,052	426,631	+0.4%
slice with negative indices	432,353	424,875	-1.7%
slice empty range	479,431	471,624	-1.6%
byteLength access	1,425,780	1,466,196	+2.8%
Symbol.toStringTag access	1,081,602	1,130,629	+4.5%
ArrayBuffer.isView	718,262	714,089	-0.6%
clone ArrayBuffer(64)	376,822	380,072	+0.9%
clone ArrayBuffer(1024)	292,797	295,302	+0.9%
clone ArrayBuffer inside object	258,164	257,849	-0.1%

arrays.js — 19 unchanged · avg -0.5%

Benchmark	Base (ops/sec)	PR (ops/sec)	Change
Array.from length 100	13,529	13,577	+0.4%
Array.from 10 elements	225,694	227,200	+0.7%
Array.of 10 elements	310,877	303,261	-2.4%
spread into new array	332,946	331,553	-0.4%
map over 50 elements	26,856	26,016	-3.1%
filter over 50 elements	22,964	22,680	-1.2%
reduce sum 50 elements	26,024	25,985	-0.1%
forEach over 50 elements	22,312	21,963	-1.6%
find in 50 elements	34,142	33,548	-1.7%
sort 20 elements	11,667	11,578	-0.8%
flat nested array	116,384	115,485	-0.8%
flatMap	71,470	71,726	+0.4%
map inside map (5x5)	20,915	20,839	-0.4%
filter inside map (5x10)	15,335	15,400	+0.4%
reduce inside map (5x10)	18,696	18,796	+0.5%
forEach inside forEach (5x10)	15,774	15,903	+0.8%
find inside some (10x10)	13,428	13,343	-0.6%
map+filter chain nested (5x20)	5,192	5,168	-0.5%
reduce flatten (10x5)	37,923	38,066	+0.4%

async-await.js — 6 unchanged · avg -0.6%

Benchmark	Base (ops/sec)	PR (ops/sec)	Change
single await	333,689	330,209	-1.0%
multiple awaits	151,111	150,212	-0.6%
await non-Promise value	733,537	728,318	-0.7%
await with try/catch	325,276	323,384	-0.6%
await Promise.all	47,548	47,653	+0.2%
nested async function call	171,304	169,862	-0.8%

classes.js — 31 unchanged · avg +0.2%

Benchmark	Base (ops/sec)	PR (ops/sec)	Change
simple class new	114,146	114,497	+0.3%
class with defaults	91,471	91,688	+0.2%
50 instances via Array.from	5,476	5,492	+0.3%
instance method call	57,549	57,801	+0.4%
static method call	90,844	89,075	-1.9%
single-level inheritance	44,826	45,111	+0.6%
two-level inheritance	37,740	38,132	+1.0%
private field access	56,467	56,633	+0.3%
private methods	61,145	60,764	-0.6%
getter/setter access	63,047	64,066	+1.6%
class decorator (identity)	78,093	80,013	+2.5%
class decorator (wrapping)	45,538	46,145	+1.3%
identity method decorator	56,934	56,542	-0.7%
wrapping method decorator	46,692	47,092	+0.9%
stacked method decorators (x3)	32,844	33,135	+0.9%
identity field decorator	64,319	63,455	-1.3%
field initializer decorator	53,454	53,127	-0.6%
getter decorator (identity)	55,025	54,534	-0.9%
setter decorator (identity)	45,758	45,542	-0.5%
static method decorator	58,570	58,482	-0.2%
static field decorator	67,623	68,422	+1.2%
private method decorator	45,300	45,921	+1.4%
private field decorator	48,774	49,638	+1.8%
plain auto-accessor (no decorator)	82,168	84,849	+3.3%
auto-accessor with decorator	49,485	49,132	-0.7%
decorator writing metadata	40,085	40,170	+0.2%
static getter read	96,021	93,367	-2.8%
static getter/setter pair	71,194	71,441	+0.3%
inherited static getter	53,527	53,021	-0.9%
inherited static setter	57,786	56,652	-2.0%
inherited static getter with this binding	48,018	48,098	+0.2%

closures.js — 11 unchanged · avg -0.5%

Benchmark	Base (ops/sec)	PR (ops/sec)	Change
closure over single variable	119,308	115,350	-3.3%
closure over multiple variables	113,605	112,578	-0.9%
nested closures	114,107	112,488	-1.4%
function as argument	88,035	88,927	+1.0%
function returning function	109,438	107,833	-1.5%
compose two functions	64,200	64,378	+0.3%
fn.call	141,193	142,284	+0.8%
fn.apply	103,697	104,249	+0.5%
fn.bind	129,609	128,115	-1.2%
recursive sum to 50	11,346	11,263	-0.7%
recursive tree traversal	18,279	18,492	+1.2%

collections.js — 🔴 1 regressed, 11 unchanged · avg -0.4%

Benchmark	Base (ops/sec)	PR (ops/sec)	Change
add 50 elements	7,087	7,136	+0.7%
has lookup (50 elements)	92,790	94,375	+1.7%
delete elements	49,106	49,294	+0.4%
forEach iteration	15,405	15,229	-1.1%
spread to array	27,488	26,105	-5.0%
deduplicate array	39,176	38,712	-1.2%
set 50 entries	5,196	5,383	+3.6%
get lookup (50 entries)	93,753	91,645	-2.2%
has check	139,151	139,569	+0.3%
delete entries	47,812	49,412	+3.3%
forEach iteration	15,236	15,600	+2.4%
keys/values/entries	7,606	7,055	🔴 -7.3%

destructuring.js — 22 unchanged · avg +0.2%

Benchmark	Base (ops/sec)	PR (ops/sec)	Change
simple array destructuring	389,161	377,132	-3.1%
with rest element	262,310	254,990	-2.8%
with defaults	389,347	379,866	-2.4%
skip elements	413,564	393,277	-4.9%
nested array destructuring	164,007	157,719	-3.8%
swap variables	475,484	468,241	-1.5%
simple object destructuring	268,916	281,155	+4.6%
with defaults	325,038	332,840	+2.4%
with renaming	277,441	293,139	+5.7%
nested object destructuring	131,907	133,947	+1.5%
rest properties	163,062	173,407	+6.3%
object parameter	87,293	82,152	-5.9%
array parameter	114,606	114,392	-0.2%
mixed destructuring in map	33,448	33,416	-0.1%
forEach with array destructuring	61,190	60,659	-0.9%
map with array destructuring	62,450	61,886	-0.9%
filter with array destructuring	63,883	63,924	+0.1%
reduce with array destructuring	69,108	69,438	+0.5%
map with object destructuring	72,068	71,967	-0.1%
map with nested destructuring	60,066	62,466	+4.0%
map with rest in destructuring	35,424	35,826	+1.1%
map with defaults in destructuring	54,182	57,226	+5.6%

fibonacci.js — 8 unchanged · avg +1.0%

Benchmark	Base (ops/sec)	PR (ops/sec)	Change
recursive fib(15)	308	320	+4.0%
recursive fib(20)	28	28	-0.6%
recursive fib(15) typed	308	321	+4.2%
recursive fib(20) typed	28	28	-1.0%
iterative fib(20) via reduce	10,917	11,072	+1.4%
iterator fib(20)	8,942	8,712	-2.6%
iterator fib(20) via Iterator.from + take	8,326	8,584	+3.1%
iterator fib(20) last value via reduce	7,227	7,202	-0.3%

for-of.js — 7 unchanged · avg -2.4%

Benchmark	Base (ops/sec)	PR (ops/sec)	Change
for...of with 10-element array	44,889	42,914	-4.4%
for...of with 100-element array	5,189	4,940	-4.8%
for...of with string (10 chars)	32,511	32,783	+0.8%
for...of with Set (10 elements)	45,164	43,278	-4.2%
for...of with Map entries (10 entries)	28,534	27,826	-2.5%
for...of with destructuring	38,755	37,538	-3.1%
for-await-of with sync array	40,070	40,627	+1.4%

iterators.js — 🟢 7 improved, 13 unchanged · avg +5.8%

Benchmark	Base (ops/sec)	PR (ops/sec)	Change
Iterator.from({next}).toArray() — 20 elements	10,256	10,555	+2.9%
Iterator.from({next}).toArray() — 50 elements	4,420	4,657	+5.4%
spread pre-wrapped iterator — 20 elements	10,679	10,786	+1.0%
Iterator.from({next}).forEach — 50 elements	3,456	3,588	+3.8%
Iterator.from({next}).reduce — 50 elements	3,493	3,489	-0.1%
wrap array iterator	54,183	57,621	+6.3%
wrap plain {next()} object	7,108	7,364	+3.6%
map + toArray (50 elements)	2,804	3,050	🟢 +8.8%
filter + toArray (50 elements)	2,982	3,253	🟢 +9.1%
take(10) + toArray (50 element source)	15,228	16,096	+5.7%
drop(40) + toArray (50 element source)	4,109	4,295	+4.5%
chained map + filter + take (100 element source)	5,023	5,372	+6.9%
some + every (50 elements)	1,920	2,084	🟢 +8.6%
find (50 elements)	4,293	4,385	+2.2%
array.values().map().filter().toArray()	3,475	3,688	+6.1%
array.values().take(5).toArray()	57,165	61,463	🟢 +7.5%
array.values().drop(45).toArray()	15,477	16,769	🟢 +8.3%
map.entries() chained helpers	5,241	5,703	🟢 +8.8%
set.values() chained helpers	8,211	9,192	🟢 +12.0%
string iterator map + toArray	7,067	7,365	+4.2%

json.js — 🟢 3 improved, 17 unchanged · avg +3.5%

Benchmark	Base (ops/sec)	PR (ops/sec)	Change
parse simple object	165,300	161,074	-2.6%
parse nested object	99,833	100,911	+1.1%
parse array of objects	54,682	54,151	-1.0%
parse large flat object	47,693	48,786	+2.3%
parse mixed types	69,310	72,596	+4.7%
stringify simple object	149,689	150,710	+0.7%
stringify nested object	76,483	82,110	🟢 +7.4%
stringify array of objects	38,606	39,648	+2.7%
stringify mixed types	68,341	67,966	-0.5%
reviver doubles numbers	41,905	41,784	-0.3%
reviver filters properties	35,944	38,327	+6.6%
reviver on nested object	48,690	50,064	+2.8%
reviver on array	27,986	29,326	+4.8%
replacer function doubles numbers	43,251	45,870	+6.1%
replacer function excludes properties	55,654	57,803	+3.9%
array replacer (allowlist)	101,855	103,725	+1.8%
stringify with 2-space indent	76,333	81,076	+6.2%
stringify with tab indent	76,678	84,196	🟢 +9.8%
parse then stringify	44,154	47,814	🟢 +8.3%
stringify then parse	26,750	27,934	+4.4%

jsx.jsx — 21 unchanged · avg +1.5%

Benchmark	Base (ops/sec)	PR (ops/sec)	Change
simple element	197,932	192,157	-2.9%
self-closing element	200,164	199,738	-0.2%
element with string attribute	164,665	162,289	-1.4%
element with multiple attributes	137,423	138,145	+0.5%
element with expression attribute	155,499	155,915	+0.3%
text child	190,132	191,795	+0.9%
expression child	182,497	187,604	+2.8%
mixed text and expression	174,131	179,593	+3.1%
nested elements (3 levels)	70,940	71,928	+1.4%
sibling children	52,622	53,455	+1.6%
component element	137,287	138,292	+0.7%
component with children	82,704	84,347	+2.0%
dotted component	113,453	115,242	+1.6%
empty fragment	193,032	196,295	+1.7%
fragment with children	52,181	52,615	+0.8%
spread attributes	97,568	101,827	+4.4%
spread with overrides	85,918	89,613	+4.3%
shorthand props	138,335	143,530	+3.8%
nav bar structure	24,399	24,801	+1.6%
card component tree	27,943	29,166	+4.4%
10 list items via Array.from	13,330	13,348	+0.1%

numbers.js — 🟢 1 improved, 10 unchanged · avg +2.6%

Benchmark	Base (ops/sec)	PR (ops/sec)	Change
integer arithmetic	507,735	536,069	+5.6%
floating point arithmetic	549,043	561,626	+2.3%
number coercion	177,255	187,809	+6.0%
toFixed	101,164	108,591	🟢 +7.3%
toString	152,338	157,909	+3.7%
valueOf	218,247	222,857	+2.1%
toPrecision	139,058	142,514	+2.5%
Number.isNaN	294,120	290,760	-1.1%
Number.isFinite	278,734	281,906	+1.1%
Number.isInteger	289,666	282,927	-2.3%
Number.parseInt and parseFloat	236,607	239,058	+1.0%

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

Benchmark	Base (ops/sec)	PR (ops/sec)	Change
create simple object	429,930	446,565	+3.9%
create nested object	206,447	207,236	+0.4%
create 50 objects via Array.from	8,622	8,813	+2.2%
property read	576,280	606,052	+5.2%
Object.keys	272,884	278,875	+2.2%
Object.entries	100,565	103,046	+2.5%
spread operator	161,780	177,979	🟢 +10.0%

promises.js — 12 unchanged · avg +2.7%

Benchmark	Base (ops/sec)	PR (ops/sec)	Change
Promise.resolve(value)	485,247	488,107	+0.6%
new Promise(resolve => resolve(value))	177,531	180,064	+1.4%
Promise.reject(reason)	474,093	482,893	+1.9%
resolve + then (1 handler)	147,611	155,690	+5.5%
resolve + then chain (3 deep)	61,052	63,394	+3.8%
resolve + then chain (10 deep)	19,627	20,565	+4.8%
reject + catch + then	84,898	89,573	+5.5%
resolve + finally + then	69,875	74,048	+6.0%
Promise.all (5 resolved)	28,344	28,415	+0.2%
Promise.race (5 resolved)	31,070	29,916	-3.7%
Promise.allSettled (5 mixed)	23,851	24,810	+4.0%
Promise.any (5 mixed)	28,062	28,836	+2.8%

strings.js — 11 unchanged · avg +2.6%

Benchmark	Base (ops/sec)	PR (ops/sec)	Change
string concatenation	394,247	397,080	+0.7%
template literal	392,264	418,147	+6.6%
string repeat	390,788	404,688	+3.6%
split and join	134,388	133,643	-0.6%
indexOf and includes	159,879	163,438	+2.2%
toUpperCase and toLowerCase	249,110	253,240	+1.7%
slice and substring	155,420	157,433	+1.3%
trim operations	181,267	186,971	+3.1%
replace and replaceAll	198,037	207,528	+4.8%
startsWith and endsWith	128,443	135,574	+5.6%
padStart and padEnd	186,915	186,883	-0.0%

typed-arrays.js — 22 unchanged · avg +1.8%

Benchmark	Base (ops/sec)	PR (ops/sec)	Change
new Int32Array(0)	306,566	309,189	+0.9%
new Int32Array(100)	281,443	283,957	+0.9%
new Int32Array(1000)	164,969	163,605	-0.8%
new Float64Array(100)	251,903	251,012	-0.4%
Int32Array.from([...])	143,823	144,026	+0.1%
Int32Array.of(1, 2, 3, 4, 5)	306,523	300,968	-1.8%
sequential write 100 elements	3,530	3,610	+2.2%
sequential read 100 elements	3,548	3,583	+1.0%
Float64Array write 100 elements	3,207	3,301	+2.9%
fill(42)	44,998	45,093	+0.2%
slice()	196,844	205,489	+4.4%
map(x => x * 2)	7,639	7,986	+4.5%
filter(x => x > 50)	7,981	7,998	+0.2%
reduce (sum)	7,577	7,690	+1.5%
sort()	169,964	176,758	+4.0%
indexOf()	423,884	430,036	+1.5%
reverse()	325,407	340,346	+4.6%
create view over existing buffer	368,583	384,914	+4.4%
subarray()	423,758	438,624	+3.5%
set() from array	569,346	601,564	+5.7%
for-of loop	5,063	5,004	-1.2%
spread into array	17,594	17,756	+0.9%

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

coderabbitai

Caution

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

⚠️ Outside diff range comments (1)

units/Goccia.AST.Expressions.pas (1)
1145-1157: ⚠️ Potential issue | 🟠 Major

Computed-member updates currently drop symbol keys.

This branch coerces every computed key through ToStringLiteral and always writes via AssignProperty, so obj[sym]++ / obj[sym]-- will bypass the existing symbol-property path used by computed assignment and compound assignment in this same unit. Please mirror that symbol handling here for both the read and the write.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@units/Goccia.AST.Expressions.pas` around lines 1145 - 1157, The
computed-member update branch currently coerces computed keys via
PropertyExpression.Evaluate(...).ToStringLiteral and always uses AssignProperty,
which drops symbol keys; update this path in the block handling
MemberExpr.Computed so that after evaluating MemberExpr.PropertyExpression you
detect if the evaluated key is a symbol (rather than blindly calling
ToStringLiteral), and when it is use the same symbol-based lookup/write used
elsewhere (use Obj.GetProperty with a symbol-aware getter and the symbol-aware
assignment path used for computed assignment/compound assignment) for both the
read (OldValue) and the write (AssignProperty replacement for symbols),
otherwise keep the existing ToStringLiteral and string-key code; keep usage of
PerformIncrement and return TGocciaUndefinedLiteralValue.UndefinedValue on error
as before.

🧹 Nitpick comments (1)

tests/language/expressions/arithmetic/increment.js (1)
30-72: Add one failing-write regression for the new member-update path.

These tests cover successful writes, but they do not pin the behavior that motivated the interpreter change: routing updates through the real property-assignment path. Please add one error/boundary case here as well (for example, a getter-only or non-writable property) so a future fallback to the old direct-define behavior gets caught.

Based on learnings: "JavaScript tests should cover happy paths, edge cases, and error cases. Keep tests isolated and grouped by feature/filename."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@tests/language/expressions/arithmetic/increment.js` around lines 30 - 72, Add
a failing-write regression test that ensures member-update uses the real
property-assignment path by asserting an increment against a
non-writable/getter-only property throws; for example add a new test (e.g.
"post-increment on getter-only property throws") that creates an object with
Object.defineProperty(obj, "x", { get: () => 1 }) or a non-writable value via
defineProperty, then assert expect(() => obj.x++).toThrow(TypeError) (or
expect().toThrow()) so any fallback to direct-define behavior will fail the
test.

🤖 Prompt for all review comments with AI agents

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

Outside diff comments:
In `@units/Goccia.AST.Expressions.pas`:
- Around line 1145-1157: The computed-member update branch currently coerces
computed keys via PropertyExpression.Evaluate(...).ToStringLiteral and always
uses AssignProperty, which drops symbol keys; update this path in the block
handling MemberExpr.Computed so that after evaluating
MemberExpr.PropertyExpression you detect if the evaluated key is a symbol
(rather than blindly calling ToStringLiteral), and when it is use the same
symbol-based lookup/write used elsewhere (use Obj.GetProperty with a
symbol-aware getter and the symbol-aware assignment path used for computed
assignment/compound assignment) for both the read (OldValue) and the write
(AssignProperty replacement for symbols), otherwise keep the existing
ToStringLiteral and string-key code; keep usage of PerformIncrement and return
TGocciaUndefinedLiteralValue.UndefinedValue on error as before.

---

Nitpick comments:
In `@tests/language/expressions/arithmetic/increment.js`:
- Around line 30-72: Add a failing-write regression test that ensures
member-update uses the real property-assignment path by asserting an increment
against a non-writable/getter-only property throws; for example add a new test
(e.g. "post-increment on getter-only property throws") that creates an object
with Object.defineProperty(obj, "x", { get: () => 1 }) or a non-writable value
via defineProperty, then assert expect(() => obj.x++).toThrow(TypeError) (or
expect().toThrow()) so any fallback to direct-define behavior will fail the
test.

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 3d600438-3d99-4eff-9824-27032fd04bd7

📥 Commits

Reviewing files that changed from the base of the PR and between b6ad483 and 66af83d.

📒 Files selected for processing (4)

tests/language/expressions/arithmetic/decrement.js
tests/language/expressions/arithmetic/increment.js
units/Goccia.AST.Expressions.pas
units/Goccia.Compiler.Expressions.pas

Mirror the symbol-aware property read/write pattern from TGocciaComputedPropertyAssignmentExpression and TGocciaComputedPropertyCompoundAssignmentExpression so that obj[sym]++ / obj[sym]-- correctly routes through GetSymbolProperty/AssignSymbolProperty instead of coercing the symbol key via ToStringLiteral. Add regression tests for symbol-keyed increment/decrement and non-writable property increment/decrement. Made-with: Cursor

frostney · 2026-03-08T04:58:40Z

Addressing CodeRabbit review findings

1. Symbol key handling in computed member increment/decrement (Major — True Positive)

Fixed in 76a0308. The computed member path in TGocciaIncrementExpression.Evaluate now checks if the evaluated property key is a TGocciaSymbolValue and routes through GetSymbolProperty/AssignSymbolProperty — mirroring the existing pattern in TGocciaComputedPropertyAssignmentExpression and TGocciaComputedPropertyCompoundAssignmentExpression. Added tests for obj[sym]++ and obj[sym]--.

2. Regression test for non-writable property increment/decrement (Nitpick — Actionable)

Fixed in 76a0308. Added post-increment on non-writable property throws and post-decrement on non-writable property throws tests that verify TypeError is thrown and the value remains unchanged, pinning the AssignProperty routing behavior.

All 3,428 tests pass in both interpreted and bytecode modes.

frostney · 2026-03-08T04:58:49Z

Re: comment 4018335953

Benchmark regressions are within CI noise range — the 13 flagged regressions are scattered across unrelated benchmarks (strings, promises, JSX, etc.) with no correlation to the increment/decrement change. The -2.2% average is consistent with normal run-to-run variance on shared CI runners.

frostney · 2026-03-08T04:58:56Z

Re: comment 4018326427

No action needed — standard Cursor Agent introduction.

frostney marked this pull request as ready for review March 8, 2026 04:45

frostney changed the title ~~Script execution path~~ Fix increment/decrement for computed member expressions Mar 8, 2026

coderabbitai Bot reviewed Mar 8, 2026

View reviewed changes

frostney merged commit 61fa87b into main Mar 9, 2026
4 checks passed

frostney deleted the cursor/script-execution-path-ffd5 branch March 9, 2026 12:38

frostney added the bug Something isn't working label Apr 9, 2026

This was referenced May 6, 2026

Fix ++/-- on BigInt operands; add OP_INC, OP_DEC, OP_TO_NUMERIC #548

Merged

Sync upvalue cell after increment/decrement on captured locals #570

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix increment/decrement for computed member expressions #54

Fix increment/decrement for computed member expressions #54
frostney merged 2 commits into
mainfrom
cursor/script-execution-path-ffd5

frostney commented Mar 8, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

cursor Bot commented Mar 8, 2026

Uh oh!

coderabbitai Bot commented Mar 8, 2026 •

edited

Loading

Rate limit exceeded

Walkthrough

Changes

Sequence Diagram(s)

Estimated Code Review Effort

Poem

Uh oh!

github-actions Bot commented Mar 8, 2026 •

edited

Loading

Uh oh!

coderabbitai Bot left a comment

Uh oh!

frostney commented Mar 8, 2026

Uh oh!

frostney commented Mar 8, 2026

Uh oh!

frostney commented Mar 8, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

frostney commented Mar 8, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by CodeRabbit

Release Notes

Uh oh!

cursor Bot commented Mar 8, 2026

Uh oh!

coderabbitai Bot commented Mar 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Rate limit exceeded

Walkthrough

Changes

Sequence Diagram(s)

Estimated Code Review Effort

Poem

Uh oh!

github-actions Bot commented Mar 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Benchmark Results

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

frostney commented Mar 8, 2026

Addressing CodeRabbit review findings

Uh oh!

frostney commented Mar 8, 2026

Uh oh!

frostney commented Mar 8, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

frostney commented Mar 8, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Mar 8, 2026 •

edited

Loading

github-actions Bot commented Mar 8, 2026 •

edited

Loading