Skip to content

Commit 6291adc

Browse files
MagicalTuxclaude
MagicalTux
and
claude
committed
vm: fix array-compound regressions via OP_ARRAY_PRE_CHECK_LOCAL
Commit ab63482 introduced OP_ARRAY_COMPOUND_ASSIGN_LOCAL and OP_ARRAY_INC_DEC_LOCAL but missed three AST corner cases: * assign_dim_op_undef — `$a[$b] += 1` on undefined $a should warn "Undefined variable $a" + "Deprecated null offset" + "Undefined array key", in that order. The native opcode evaluated the offset first and let arrayGet's "Trying to access array offset on null" replace the proper undefined-variable warning. * bug53432 — `$str[0] += 1` on a string should throw "Cannot use assign-op operators with string offsets" BEFORE the offset is evaluated. The native path was running arrayGet + arraySetLocal through the string-offset machinery instead. * bug70662 — when arrayGet's undefined-key warning triggers an error handler that creates the key, the compound write must be suppressed so the handler's value remains visible. Adds OP_ARRAY_PRE_CHECK_LOCAL which runs BEFORE the offset is pushed. It mirrors runArrayAccess.Run's ZtNull / ZtString compound-write-context branches: warn + auto-vivify undefined containers, vivify explicit nulls, and throw "Cannot use assign-op operators with string offsets" for string containers. Warning order now matches the AST. OP_ARRAY_COMPOUND_ASSIGN_LOCAL and OP_ARRAY_INC_DEC_LOCAL now also: * emit "Using null as an array offset is deprecated" in the read phase when the offset is null and the container is array/string/ object (matches compile-array.go:712-717). * snapshot key existence pre-arrayGet and re-check the slot post-op. If the slot was replaced by a scalar (handler trashed it) or the key now exists when it didn't before (handler created it), the write is skipped (compile-array.go:826-862, bug70662 semantics). Verified bit-identical to AST on assign_dim_op_undef, bug53432, bug70662, plus the existing compound/incdec smoke tests. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
1 parent ba82884 commit 6291adc

3 files changed

Lines changed: 174 additions & 2 deletions

File tree

‎core/vm/opcode.go‎

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -665,6 +665,27 @@ const (
665665
// C bit 0 = keep result on stack
666666
OpArrayIncDecLocal
667667

668+
// OP_ARRAY_PRE_CHECK_LOCAL runs BEFORE the offset is evaluated in
669+
// a `$local[offset] OP= rhs` or `$local[offset]++` operation. It
670+
// mirrors the AST's compound-write-context handling of the container
671+
// in runArrayAccess.Run so that warnings about an undefined container
672+
// fire BEFORE warnings emitted by offset evaluation. Specifically:
673+
// - Slot is nil (undefined): warn "Undefined variable $X" and
674+
// vivify the slot to an empty array.
675+
// - Slot holds ZtNull: vivify to empty array (no warning — explicit
676+
// null assignment, like the AST's auto-vivify path).
677+
// - Slot holds ZtString: throw "Cannot use assign-op operators with
678+
// string offsets" before the offset is touched (bug53432).
679+
// Other container types (array, object, bool true, scalar) are left
680+
// untouched and any error/auto-vivify is handled downstream by
681+
// arrayGet/arraySetLocal.
682+
//
683+
// Stack: no changes (no operands popped or pushed).
684+
//
685+
// Encoding:
686+
// A = local slot index
687+
OpArrayPreCheckLocal
688+
668689
// Sentinel — keep last.
669690
opLast
670691
)
@@ -818,4 +839,5 @@ var opNames = [...]string{
818839
OpStoreLocalRef: "STORE_LOCAL_REF",
819840
OpArrayCompoundAssignLocal: "ARRAY_COMPOUND_ASSIGN_LOCAL",
820841
OpArrayIncDecLocal: "ARRAY_INC_DEC_LOCAL",
842+
OpArrayPreCheckLocal: "ARRAY_PRE_CHECK_LOCAL",
821843
}

‎core/vm/vm.go‎

Lines changed: 142 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -918,10 +918,58 @@ func (f *Frame) runUntilError(ctx phpv.Context) (retVal *phpv.ZVal, finished boo
918918
return nil, false, err
919919
}
920920

921+
case OpArrayPreCheckLocal:
922+
// `$local[expr] OP= …` / `$local[expr]++` pre-check, runs
923+
// BEFORE the offset is evaluated so warnings about the
924+
// container fire in the right order relative to the offset's
925+
// own evaluation warnings.
926+
//
927+
// Mirrors the AST's compound-write-context handling in
928+
// runArrayAccess.Run (compile-array.go:486-528): undefined
929+
// variables warn and auto-vivify, ZtNull auto-vivifies, and
930+
// string containers reject compound ops with the same
931+
// "Cannot use assign-op operators with string offsets" Error
932+
// the AST throws before touching the offset (bug53432).
933+
{
934+
name := f.fn.Locals[ins.A()]
935+
container := f.locals[ins.A()]
936+
wasUndefined := container == nil
937+
if container == nil && !f.fn.SlotOnly {
938+
if v, found, _ := ctx.OffsetCheck(ctx, name); found && v != nil {
939+
container = v
940+
wasUndefined = false
941+
}
942+
}
943+
if container != nil && container.GetType() == phpv.ZtString {
944+
return nil, false, phpobj.ThrowError(ctx, phpobj.Error,
945+
"Cannot use assign-op operators with string offsets")
946+
}
947+
if container == nil || container.GetType() == phpv.ZtNull {
948+
if wasUndefined {
949+
if err := ctx.Warn("Undefined variable $%s", string(name), logopt.NoFuncName(true)); err != nil {
950+
return nil, false, err
951+
}
952+
}
953+
newArr := phpv.NewZArrayTracked(ctx.Global().MemMgrTracker())
954+
container = newArr.ZVal()
955+
f.locals[ins.A()] = container
956+
if !f.fn.SlotOnly {
957+
if err := ctx.OffsetSet(ctx, name, container); err != nil {
958+
return nil, false, err
959+
}
960+
}
961+
}
962+
}
963+
921964
case OpArrayCompoundAssignLocal:
922965
// `$local[offset] OP= rhs` on a simple-local container.
923966
// Mirrors OpObjectCompoundAssign's read+snapshot+op+write
924967
// flow but with the array element as the LHS slot.
968+
//
969+
// The container is assumed to already be array-shaped (or a
970+
// scalar that arrayGet/arraySetLocal know how to reject) —
971+
// OP_ARRAY_PRE_CHECK_LOCAL emitted earlier has vivified null
972+
// containers and rejected string ones.
925973
rhs := f.pop()
926974
offset := f.pop()
927975
op := tokenizer.ItemType(ins.B())
@@ -933,6 +981,28 @@ func (f *Frame) runUntilError(ctx phpv.Context) (retVal *phpv.ZVal, finished boo
933981
if container == nil {
934982
container = phpv.ZNULL.ZVal()
935983
}
984+
// PHP 8.1: deprecate null-as-offset in the read phase for
985+
// array/string/object containers, matching the AST
986+
// (compile-array.go:712-717). arraySetLocal does NOT also
987+
// emit this on the write phase (would double-warn).
988+
if offset != nil && offset.GetType() == phpv.ZtNull {
989+
ct := container.GetType()
990+
if ct == phpv.ZtArray || ct == phpv.ZtString || ct == phpv.ZtObject {
991+
if err := ctx.Deprecated("Using null as an array offset is deprecated, use an empty string instead", logopt.NoFuncName(true)); err != nil {
992+
return nil, false, err
993+
}
994+
}
995+
}
996+
// bug70662: track whether the key existed BEFORE arrayGet.
997+
// If arrayGet's "Undefined array key" warning triggers a
998+
// user error handler that creates the key, PHP's compound
999+
// op suppresses the write (the handler's value wins).
1000+
var keyWasMissing bool
1001+
if container.GetType() == phpv.ZtArray && offset != nil {
1002+
zarr := container.AsArray(ctx)
1003+
_, exists, _ := zarr.OffsetCheck(ctx, offset.Value())
1004+
keyWasMissing = !exists
1005+
}
9361006
cur, err := arrayGet(ctx, container, offset)
9371007
if err != nil {
9381008
return nil, false, err
@@ -949,6 +1019,33 @@ func (f *Frame) runUntilError(ctx phpv.Context) (retVal *phpv.ZVal, finished boo
9491019
if err != nil {
9501020
return nil, false, err
9511021
}
1022+
// Re-fetch container — an error handler triggered during
1023+
// arrayGet (undef-key) or fn (__toString) could have
1024+
// replaced the slot with something incompatible.
1025+
postContainer := f.locals[ins.A()]
1026+
if postContainer == nil || (postContainer.GetType() != phpv.ZtArray &&
1027+
postContainer.GetType() != phpv.ZtObject &&
1028+
postContainer.GetType() != phpv.ZtString &&
1029+
postContainer.GetType() != phpv.ZtNull) {
1030+
// Container was replaced by a scalar — abort write to
1031+
// match AST's WriteValue behavior (compile-array.go:826-832).
1032+
if ins.C()&1 != 0 {
1033+
f.push(res)
1034+
}
1035+
break
1036+
}
1037+
// bug70662: if the key was missing during the read phase but
1038+
// now exists in the container, an error handler created it.
1039+
// Suppress the write so the handler's value remains visible.
1040+
if keyWasMissing && offset != nil && postContainer.GetType() == phpv.ZtArray {
1041+
zarr := postContainer.AsArray(ctx)
1042+
if _, exists, _ := zarr.OffsetCheck(ctx, offset.Value()); exists {
1043+
if ins.C()&1 != 0 {
1044+
f.push(res)
1045+
}
1046+
break
1047+
}
1048+
}
9521049
if err := arraySetLocal(ctx, f, ins.A(), offset, res); err != nil {
9531050
return nil, false, err
9541051
}
@@ -962,6 +1059,9 @@ func (f *Frame) runUntilError(ctx phpv.Context) (retVal *phpv.ZVal, finished boo
9621059
// warns on undefined keys, matching the AST), Dup's so
9631060
// DoInc's in-place mutation doesn't escape, applies DoInc,
9641061
// writes back via arraySetLocal.
1062+
//
1063+
// OP_ARRAY_PRE_CHECK_LOCAL emitted earlier vivifies null
1064+
// containers and rejects string ones.
9651065
offset := f.pop()
9661066
inc := ins.B()&1 != 0
9671067
post := ins.B()&2 != 0
@@ -970,6 +1070,21 @@ func (f *Frame) runUntilError(ctx phpv.Context) (retVal *phpv.ZVal, finished boo
9701070
if container == nil {
9711071
container = phpv.ZNULL.ZVal()
9721072
}
1073+
// PHP 8.1 null-offset deprecation (same as compound path).
1074+
if offset != nil && offset.GetType() == phpv.ZtNull {
1075+
ct := container.GetType()
1076+
if ct == phpv.ZtArray || ct == phpv.ZtString || ct == phpv.ZtObject {
1077+
if err := ctx.Deprecated("Using null as an array offset is deprecated, use an empty string instead", logopt.NoFuncName(true)); err != nil {
1078+
return nil, false, err
1079+
}
1080+
}
1081+
}
1082+
var keyWasMissing bool
1083+
if container.GetType() == phpv.ZtArray && offset != nil {
1084+
zarr := container.AsArray(ctx)
1085+
_, exists, _ := zarr.OffsetCheck(ctx, offset.Value())
1086+
keyWasMissing = !exists
1087+
}
9731088
cur, err := arrayGet(ctx, container, offset)
9741089
if err != nil {
9751090
return nil, false, err
@@ -985,6 +1100,33 @@ func (f *Frame) runUntilError(ctx phpv.Context) (retVal *phpv.ZVal, finished boo
9851100
if err := compiler.DoInc(ctx, cur, inc); err != nil {
9861101
return nil, false, err
9871102
}
1103+
postContainer := f.locals[ins.A()]
1104+
if postContainer == nil || (postContainer.GetType() != phpv.ZtArray &&
1105+
postContainer.GetType() != phpv.ZtObject &&
1106+
postContainer.GetType() != phpv.ZtString &&
1107+
postContainer.GetType() != phpv.ZtNull) {
1108+
if keep {
1109+
if post {
1110+
f.push(pre)
1111+
} else {
1112+
f.push(cur)
1113+
}
1114+
}
1115+
break
1116+
}
1117+
if keyWasMissing && offset != nil && postContainer.GetType() == phpv.ZtArray {
1118+
zarr := postContainer.AsArray(ctx)
1119+
if _, exists, _ := zarr.OffsetCheck(ctx, offset.Value()); exists {
1120+
if keep {
1121+
if post {
1122+
f.push(pre)
1123+
} else {
1124+
f.push(cur)
1125+
}
1126+
}
1127+
break
1128+
}
1129+
}
9881130
if err := arraySetLocal(ctx, f, ins.A(), offset, cur); err != nil {
9891131
return nil, false, err
9901132
}

‎core/vm/vmcompiler/emit_expr.go‎

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1142,13 +1142,19 @@ func (e *emitter) emitCompoundAssign(n operatorNode, op tokenizer.ItemType) erro
11421142
if v, ok := aa.ArrayAccessContainer().(variableNode); ok &&
11431143
!aa.ArrayAccessIsNullSafe() && aa.ArrayAccessOffset() != nil {
11441144
stmtCtx := e.stmtCtx
1145+
idx := e.localIndex(v.VariableName())
1146+
// Pre-check the container BEFORE evaluating the offset so
1147+
// undefined-variable / null-vivify warnings fire in the
1148+
// right order relative to offset-evaluation warnings, and
1149+
// so string containers reject the compound op before any
1150+
// offset side effects (bug53432, assign_dim_op_undef).
1151+
e.emit(vm.OpArrayPreCheckLocal, idx, 0, 0)
11451152
if err := e.withSubexpr(func() error { return e.emitExpr(aa.ArrayAccessOffset()) }); err != nil {
11461153
return err
11471154
}
11481155
if err := e.withSubexpr(func() error { return e.emitExpr(n.OperatorB()) }); err != nil {
11491156
return err
11501157
}
1151-
idx := e.localIndex(v.VariableName())
11521158
var c int32
11531159
if !stmtCtx {
11541160
c = 1
@@ -1299,10 +1305,12 @@ func (e *emitter) emitIncDec(n operatorNode, inc bool) error {
12991305
if v, ok := aa.ArrayAccessContainer().(variableNode); ok &&
13001306
!aa.ArrayAccessIsNullSafe() && aa.ArrayAccessOffset() != nil {
13011307
stmtCtx := e.stmtCtx
1308+
idx := e.localIndex(v.VariableName())
1309+
// Same pre-check as compound assign (see comment there).
1310+
e.emit(vm.OpArrayPreCheckLocal, idx, 0, 0)
13021311
if err := e.withSubexpr(func() error { return e.emitExpr(aa.ArrayAccessOffset()) }); err != nil {
13031312
return err
13041313
}
1305-
idx := e.localIndex(v.VariableName())
13061314
var b uint16
13071315
if inc {
13081316
b |= 1

0 commit comments

Comments
 (0)