Merged
Conversation
- Parse, compile, and evaluate `&&=` and `||=` - Add end-to-end tests for assignments, short-circuiting, and errors - Update docs to list logical assignment support
Contributor
📝 WalkthroughWalkthroughAdds ES logical assignment operators Changes
Sequence DiagramsequenceDiagram
autonumber
participant Source as Source Code
participant Lexer
participant Parser
participant Compiler
participant VM as Runtime/Evaluator
Source->>Lexer: Tokenize `a &&= b`
Lexer->>Lexer: match '&&', check for '='
Lexer->>Parser: emit `gttLogicalAndAssign`
Parser->>Parser: parse Assignment -> CompoundAssignment AST
Parser->>Compiler: send AST
Compiler->>Compiler: IsShortCircuitAssignment(op)
Compiler->>Compiler: ShortCircuitJumpOp(op)
Compiler->>Compiler: emit LOAD LHS
Compiler->>Compiler: emit JUMP_IF_FALSE (patchable)
Compiler->>Compiler: compile RHS
Compiler->>Compiler: emit ASSIGN target
Compiler->>VM: load bytecode
VM->>VM: evaluate LHS
VM->>VM: ToBoolean(LHS)
alt LHS falsy
VM->>VM: short-circuit return LHS
else LHS truthy
VM->>VM: evaluate RHS
VM->>VM: assign RHS to target
VM->>VM: return assigned value
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
frostney
added
spec compliance
Contributor
Benchmark Results306 benchmarks Interpreted: 🟢 155 improved · 🔴 88 regressed · 63 unchanged · avg +2.4% arraybuffer.js — Interp: 🟢 6, 🔴 3, 5 unch. · avg +2.5% · Bytecode: 🟢 14 · avg +11.3%
arrays.js — Interp: 🟢 10, 🔴 7, 2 unch. · avg +0.8% · Bytecode: 🟢 19 · avg +9.7%
async-await.js — Interp: 🔴 2, 4 unch. · avg -0.6% · Bytecode: 🟢 6 · avg +7.9%
base64.js — Interp: 🟢 3, 🔴 6, 1 unch. · avg +2.8% · Bytecode: 🟢 10 · avg +12.8%
classes.js — Interp: 🟢 31 · avg +8.1% · Bytecode: 🟢 20, 11 unch. · avg +9.1%
closures.js — Interp: 🟢 10, 🔴 1 · avg +3.8% · Bytecode: 🟢 8, 3 unch. · avg +5.7%
collections.js — Interp: 🟢 3, 🔴 8, 1 unch. · avg -0.1% · Bytecode: 🟢 11, 1 unch. · avg +7.5%
destructuring.js — Interp: 🟢 22 · avg +8.0% · Bytecode: 🟢 22 · avg +10.7%
fibonacci.js — Interp: 🟢 1, 🔴 5, 2 unch. · avg -2.1% · Bytecode: 🟢 8 · avg +14.5%
for-of.js — Interp: 🟢 4, 🔴 1, 2 unch. · avg +1.1% · Bytecode: 🟢 7 · avg +9.4%
helpers/bench-module.js — Interp: 0 · Bytecode: 0
iterators.js — Interp: 🟢 8, 🔴 22, 12 unch. · avg -1.8% · Bytecode: 🟢 42 · avg +11.5%
json.js — Interp: 🟢 1, 🔴 9, 10 unch. · avg -1.3% · Bytecode: 🟢 20 · avg +12.3%
jsx.jsx — Interp: 🟢 18, 3 unch. · avg +5.2% · Bytecode: 🟢 21 · avg +6.1%
modules.js — Interp: 🟢 1, 🔴 8 · avg -5.5% · Bytecode: 🟢 8, 1 unch. · avg +10.9%
numbers.js — Interp: 🟢 4, 🔴 5, 2 unch. · avg +0.7% · Bytecode: 🟢 11 · avg +21.4%
objects.js — Interp: 🟢 6, 🔴 1 · avg +7.7% · Bytecode: 🟢 7 · avg +5.7%
promises.js — Interp: 🟢 5, 🔴 3, 4 unch. · avg +0.7% · Bytecode: 🟢 12 · avg +6.2%
regexp.js — Interp: 🟢 2, 🔴 3, 6 unch. · avg -0.0% · Bytecode: 🟢 10, 1 unch. · avg +5.6%
strings.js — Interp: 🟢 7, 🔴 1, 3 unch. · avg +2.3% · Bytecode: 🟢 10, 🔴 1 · avg +4.4%
typed-arrays.js — Interp: 🟢 13, 🔴 3, 6 unch. · avg +6.2% · Bytecode: 🟢 21, 1 unch. · avg +12.4%
Measured on ubuntu-latest x64. Benchmark ranges compare cached main-branch min/max ops/sec with the PR run; overlapping ranges are treated as unchanged noise. Percentage deltas are secondary context. |
Contributor
Suite Timing
Measured on ubuntu-latest x64. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
units/Goccia.Evaluator.pas (1)
2684-2699:⚠️ Potential issue | 🟠 MajorPreserve getter-only private accessor errors in the short-circuit write path.
When
??=,&&=, or||=actually needs to assign to an instance private member, this branch skips theHasPrivateGettercheck that still exists in the normal compound-assignment path at Lines 2728-2729. A getter-only private accessor will therefore be written viaSetPrivateProperty(...)instead of throwing the expectedTypeError.💡 Proposed fix
if ObjectValue is TGocciaInstanceValue then begin if AccessClass.HasPrivateSetter(APrivatePropertyCompoundAssignmentExpression.PrivateName) then begin SetterFn := AccessClass.PrivatePropertySetter[APrivatePropertyCompoundAssignmentExpression.PrivateName]; SetterArgs := TGocciaArgumentsCollection.Create; try SetterArgs.Add(Value); SetterFn.Call(SetterArgs, Instance); finally SetterArgs.Free; end; end + else if AccessClass.HasPrivateGetter(APrivatePropertyCompoundAssignmentExpression.PrivateName) then + ThrowPrivateSetterMissingError(APrivatePropertyCompoundAssignmentExpression.PrivateName) else Instance.SetPrivateProperty(APrivatePropertyCompoundAssignmentExpression.PrivateName, Result, AccessClass); end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@units/Goccia.Evaluator.pas` around lines 2684 - 2699, The short-circuit write path handling TGocciaInstanceValue skips the HasPrivateGetter check and can call Instance.SetPrivateProperty for getter-only private accessors; update the branch that currently tests AccessClass.HasPrivateSetter(APrivatePropertyCompoundAssignmentExpression.PrivateName) (and then either calls SetterFn.Call or Instance.SetPrivateProperty) to first check AccessClass.HasPrivateGetter for the same PrivateName and, if a getter-only accessor exists (HasPrivateGetter true and HasPrivateSetter false), raise the same TypeError the normal compound-assignment path uses; only call the setter or Instance.SetPrivateProperty when the getter check permits writing.
🤖 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.Evaluator.pas`:
- Around line 2684-2699: The short-circuit write path handling
TGocciaInstanceValue skips the HasPrivateGetter check and can call
Instance.SetPrivateProperty for getter-only private accessors; update the branch
that currently tests
AccessClass.HasPrivateSetter(APrivatePropertyCompoundAssignmentExpression.PrivateName)
(and then either calls SetterFn.Call or Instance.SetPrivateProperty) to first
check AccessClass.HasPrivateGetter for the same PrivateName and, if a
getter-only accessor exists (HasPrivateGetter true and HasPrivateSetter false),
raise the same TypeError the normal compound-assignment path uses; only call the
setter or Instance.SetPrivateProperty when the getter check permits writing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f9f6747f-7ace-4a15-8826-59f2db2ccb11
📒 Files selected for processing (11)
README.mddocs/language-restrictions.mdtests/language/expressions/logical/logical-and-assignment.jstests/language/expressions/logical/logical-or-assignment.jsunits/Goccia.AST.Expressions.pasunits/Goccia.Compiler.Context.pasunits/Goccia.Compiler.Expressions.pasunits/Goccia.Evaluator.pasunits/Goccia.Lexer.pasunits/Goccia.Parser.pasunits/Goccia.Token.pas
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
units/Goccia.Evaluator.pas (1)
2684-2699:⚠️ Potential issue | 🟠 MajorPreserve getter-only private accessor errors in the short-circuit write path.
When
#x ||= .../#x &&= .../#x ??= ...actually needs to assign on an instance, this branch skips the getter-only check and falls through toInstance.SetPrivateProperty(...). That diverges from bothEvaluatePrivatePropertyAssignment(Lines 2581-2598) and the non-short-circuit compound path (Lines 2716-2729), which correctly throwThrowPrivateSetterMissingErrorfor getter-only private accessors.💡 Proposed fix
if ObjectValue is TGocciaInstanceValue then begin if AccessClass.HasPrivateSetter(APrivatePropertyCompoundAssignmentExpression.PrivateName) then begin SetterFn := AccessClass.PrivatePropertySetter[APrivatePropertyCompoundAssignmentExpression.PrivateName]; SetterArgs := TGocciaArgumentsCollection.Create; try SetterArgs.Add(Value); SetterFn.Call(SetterArgs, Instance); finally SetterArgs.Free; end; end + else if AccessClass.HasPrivateGetter(APrivatePropertyCompoundAssignmentExpression.PrivateName) then + ThrowPrivateSetterMissingError( + APrivatePropertyCompoundAssignmentExpression.PrivateName) else Instance.SetPrivateProperty(APrivatePropertyCompoundAssignmentExpression.PrivateName, Result, AccessClass); end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@units/Goccia.Evaluator.pas` around lines 2684 - 2699, The short-circuit instance assignment path bypasses the getter-only check and directly calls Instance.SetPrivateProperty, so replicate the same checks as EvaluatePrivatePropertyAssignment: when ObjectValue is TGocciaInstanceValue, first test AccessClass.HasPrivateSetter(APrivatePropertyCompoundAssignmentExpression.PrivateName) and call the private setter (SetterFn.Call) if present; otherwise if the property has a private getter but no setter (e.g., AccessClass.HasPrivateGetter for APrivatePropertyCompoundAssignmentExpression.PrivateName) then invoke ThrowPrivateSetterMissingError with the PrivateName to preserve the getter-only error; only when neither private getter/setter metadata indicates an accessor should you fall back to Instance.SetPrivateProperty. Ensure you reference TGocciaInstanceValue, AccessClass.HasPrivateSetter/HasPrivateGetter, APrivatePropertyCompoundAssignmentExpression.PrivateName, SetterFn, Instance.SetPrivateProperty and ThrowPrivateSetterMissingError so the logic mirrors EvaluatePrivatePropertyAssignment and the non-short-circuit compound path.
🤖 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.Evaluator.pas`:
- Around line 2684-2699: The short-circuit instance assignment path bypasses the
getter-only check and directly calls Instance.SetPrivateProperty, so replicate
the same checks as EvaluatePrivatePropertyAssignment: when ObjectValue is
TGocciaInstanceValue, first test
AccessClass.HasPrivateSetter(APrivatePropertyCompoundAssignmentExpression.PrivateName)
and call the private setter (SetterFn.Call) if present; otherwise if the
property has a private getter but no setter (e.g., AccessClass.HasPrivateGetter
for APrivatePropertyCompoundAssignmentExpression.PrivateName) then invoke
ThrowPrivateSetterMissingError with the PrivateName to preserve the getter-only
error; only when neither private getter/setter metadata indicates an accessor
should you fall back to Instance.SetPrivateProperty. Ensure you reference
TGocciaInstanceValue, AccessClass.HasPrivateSetter/HasPrivateGetter,
APrivatePropertyCompoundAssignmentExpression.PrivateName, SetterFn,
Instance.SetPrivateProperty and ThrowPrivateSetterMissingError so the logic
mirrors EvaluatePrivatePropertyAssignment and the non-short-circuit compound
path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 56b25227-f4d5-49b3-b352-385fce1153c9
📒 Files selected for processing (11)
README.mddocs/language-restrictions.mdtests/language/expressions/logical/logical-and-assignment.jstests/language/expressions/logical/logical-or-assignment.jsunits/Goccia.AST.Expressions.pasunits/Goccia.Compiler.Context.pasunits/Goccia.Compiler.Expressions.pasunits/Goccia.Evaluator.pasunits/Goccia.Lexer.pasunits/Goccia.Parser.pasunits/Goccia.Token.pas
- Throw when `&&=`/`||=` target a private accessor without a setter - Add regression coverage for short-circuit and TypeError cases
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
units/Goccia.Evaluator.pas (1)
2655-2705:⚠️ Potential issue | 🟠 MajorTemp-root the private receiver before evaluating the RHS.
Line 2681 can allocate or trigger GC while the target instance/class is only kept in
ObjectValue/Instance/ClassValuelocals here. A temporary receiver can be reclaimed before the laterSetPrivateProperty/AssignPrivateMemberOnClasswrite-back runs.Please guard the full read/short-circuit/write window with
AddTempRoot/RemoveTempRootfor the receiver, ideally once for the whole function since the non-short-circuit path below has the same lifetime shape.As per coding guidelines, "Values held only by Pascal code (not in any GocciaScript scope) must be protected with AddTempRoot/RemoveTempRoot".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@units/Goccia.Evaluator.pas` around lines 2655 - 2705, The code must protect the receiver across the evaluate/short-circuit/write window: wrap the receiver value(s) (ObjectValue / Instance / ClassValue) with AddTempRoot before calling EvaluateExpression and other GC-triggering ops and RemoveTempRoot after the eventual write-back (SetPrivateProperty or AssignPrivateMemberOnClass); ideally call AddTempRoot once at the start of the function handling APrivatePropertyCompoundAssignmentExpression and RemoveTempRoot at all exit points (including short-circuit returns) so the receiver cannot be reclaimed while running EvaluateExpression, calling private setters (PrivatePropertySetter.Call) or performing Instance.SetPrivateProperty / AssignPrivateMemberOnClass.
🤖 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.Evaluator.pas`:
- Around line 2655-2705: The code must protect the receiver across the
evaluate/short-circuit/write window: wrap the receiver value(s) (ObjectValue /
Instance / ClassValue) with AddTempRoot before calling EvaluateExpression and
other GC-triggering ops and RemoveTempRoot after the eventual write-back
(SetPrivateProperty or AssignPrivateMemberOnClass); ideally call AddTempRoot
once at the start of the function handling
APrivatePropertyCompoundAssignmentExpression and RemoveTempRoot at all exit
points (including short-circuit returns) so the receiver cannot be reclaimed
while running EvaluateExpression, calling private setters
(PrivatePropertySetter.Call) or performing Instance.SetPrivateProperty /
AssignPrivateMemberOnClass.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 201c5bea-66cb-49ce-9264-864b9bb3a452
📒 Files selected for processing (3)
tests/language/expressions/logical/logical-and-assignment.jstests/language/expressions/logical/logical-or-assignment.jsunits/Goccia.Evaluator.pas
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/language/expressions/logical/logical-or-assignment.js
- tests/language/expressions/logical/logical-and-assignment.js
This was referenced May 1, 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
&&=and||=across lexer, parser, evaluator, and bytecode compiler.Testing
tests/language/expressions/logical/for&&=and||=.