Align array and scalar semantics with gawk - #534
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9d372d3d88
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (rawValueArgument) { | ||
| return reference.currentValue(); |
There was a problem hiding this comment.
When an untyped caller variable is passed through a user-function parameter and then supplied to an indirectly invoked @JawkRawValue extension, such as function f(x) { cb = "typeof"; print @cb(x) } BEGIN { f(a) }, reference.currentValue() returns the caller's nested ArgumentReference rather than the underlying untyped value. That private helper object reaches GawkExtension.typeof(), which consequently reports string instead of untyped; raw arguments need to be recursively unwrapped without scalarizing the value.
Useful? React with 👍 / 👎.
|
@codex please review again |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6cf94ebf2e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (snapshot instanceof ArgumentReference) { | ||
| return resolveUserFunctionArgument(snapshot); | ||
| } | ||
| return isUntyped(snapshot) ? reference : snapshot; |
There was a problem hiding this comment.
For an unset actual parameter that becomes scalar before the formal is first read, this returns a live reference whose currentValue() exposes the caller's new scalar value. For example, function f(x) { a = 5; print "[" x "]" } BEGIN { f(a) } must print [] like gawk because scalar arguments are passed by value, but this path resolves x to the newly assigned 5 and prints [5]. The deferred reference must propagate array typing conflicts without aliasing subsequent scalar values.
Useful? React with 👍 / 👎.
|
|
||
| @Override | ||
| public Object currentValue() { | ||
| return JRT.getAssocArrayValue(map, key); |
There was a problem hiding this comment.
When an untyped subarray argument is deleted before the formal is materialized, this lookup autovivifies the deleted key again. In function f(x) { delete a[1]; x[2] = 3 } BEGIN { f(a[1]); print (1 in a) }, gawk prints 0 because x retains the now-detached argument node, whereas getAssocArrayValue(map, key) recreates a[1] and the later setValue stores the new array there, producing 1.
Useful? React with 👍 / 👎.
|
@codex please review again |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b840329db8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| @Override | ||
| public void setScalarValue(Object value) { | ||
| // Scalar arguments are copied at call time. | ||
| } |
There was a problem hiding this comment.
When an untyped bare variable is passed and the callee first uses the parameter as a scalar—for example, function f(x){print x} BEGIN{f(a); a[1]=1}—gawk fixes the caller's a as scalar, so the later array use is fatal. Here setScalarValue is a no-op, leaving a untyped and allowing a[1] to succeed. Materialize the caller as blank when it remains untyped, and cover this ordering with an AwkTestSupport regression.
AGENTS.md reference: AGENTS.md:L23-L26
Useful? React with 👍 / 👎.
| @Override | ||
| public void setScalarValue(Object value) { | ||
| setValue(value); |
There was a problem hiding this comment.
If an untyped element is passed and the caller-visible element changes before the parameter's first scalar use—for example, function f(x){a[1]=7; print x} BEGIN{f(a[1]); print a[1]}—gawk prints the call-time blank for x while preserving a[1] == 7. This unconditional setValue instead replaces the current element with the old blank snapshot; it can likewise overwrite a newly created subarray. Scalar resolution should retain the snapshot without overwriting a now-typed actual, with an AwkTestSupport regression for this case.
AGENTS.md reference: AGENTS.md:L23-L26
Useful? React with 👍 / 👎.
|
@codex please review again |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 319ce8e9e3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
|
|
||
| private Object resolveArgumentReference(ArgumentReference reference, boolean arrayContext) { | ||
| Object value = arrayContext ? reference.currentValue() : reference.snapshot(); |
There was a problem hiding this comment.
When an untyped actual becomes an array after the call, scalar resolution still uses only the call-time snapshot. For example, function f(x,y){x[1]=1; print y} BEGIN{f(a,a)} prints a blank and continues, whereas gawk reports an array-in-scalar-context error because both parameters alias the now-array a. Preserve scalar call-time values, but check currentValue() for an array transition before returning the snapshot.
Useful? React with 👍 / 👎.
| } | ||
|
|
||
| private Object resolveRawArgumentReference(ArgumentReference reference) { | ||
| Object value = reference.snapshot(); |
There was a problem hiding this comment.
Raw-value builtins always inspect the argument's snapshot, so they miss an array created through the caller after entry. With function f(x){a[1]=1; print typeof(x), isarray(x)} BEGIN{f(a)}, Jawk reports untyped 0, while gawk reports array 1. Raw reads should retain the snapshot for scalar changes but observe when the referenced value has become an array.
Useful? React with 👍 / 👎.
| if (!isUntyped(value)) { | ||
| return value; |
There was a problem hiding this comment.
A typed array is returned unchanged even when arrayContext is false, relying on a later coercion to notice the invalid scalar use. This misses discarded expression results: BEGIN { a[1]=1; a; print "continued" } prints continued in Jawk, while gawk terminates at the bare scalar use of a. Since expression statements are now accepted, scalar dereference itself needs to reject a Map rather than returning it.
Useful? React with 👍 / 👎.
|
@codex please review again |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2a0b6403a4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| public Object currentValue() { | ||
| return JRT.containsAwkKey(map, key) ? | ||
| JRT.getAssocArrayValue(map, key) : detachedValue; |
There was a problem hiding this comment.
When an untyped element is passed to a function and the caller deletes and recreates that key before the parameter's first array use, this lookup attaches the parameter to the replacement element. For example, f(a[1]) with f executing delete a[1]; a[1][9] = 9; x[2] = 2 incorrectly adds 2 to the new a[1]; gawk keeps x detached once the original element is deleted. Track that the original mapping disappeared rather than deciding attachment solely from current key presence, and cover delete-then-reinsert through AwkTestSupport.
AGENTS.md reference: AGENTS.md:L19-L26
Useful? React with 👍 / 👎.
|
@codex please review again |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
@codex please review again |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1c5e87b11a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (!arrayContext) { | ||
| checkScalar(readCurrentArgumentValue(reference)); |
There was a problem hiding this comment.
When an untyped actual becomes an array before length() reads its formal, this unconditional scalar check rejects a context that accepts both scalars and arrays. For example, function f(x,y){y[1]=1; print length(x)} BEGIN{f(a,a)} prints 1 in gawk, but x reaches this check through the ordinary identifier dereference and throws because its live value is now a map. length() needs a type-neutral, reference-aware dereference, with an AwkTestSupport regression covering this alias transition.
AGENTS.md reference: AGENTS.md:L23-L26
Useful? React with 👍 / 👎.
| private void detachMissingArrayArgumentReferences(Map<Object, Object> map) { | ||
| for (IndirectArrayArgumentReference reference : indirectArrayArgumentReferences) { | ||
| reference.detachIfMissing(map); | ||
| } |
There was a problem hiding this comment.
Fresh evidence beyond the earlier explicit delete/reinsert case is that extension mutations bypass every call to this detachment helper. In function f(x){b[3][4]=9; asort(b,a); x[2]=2; print (2 in a[1])} BEGIN{f(a[1])}, GawkExtension.sort() clears and repopulates a with a new subarray at key 1; gawk keeps x attached to the removed node and prints 0, whereas this reference still sees the replacement map and adds key 2 to it, printing 1. Detachment must also cover extension-driven clear/repopulation, with an AwkTestSupport regression.
AGENTS.md reference: AGENTS.md:L23-L26
Useful? React with 👍 / 👎.
|
@codex please review again |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 65cb012628
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| break; | ||
| } | ||
| if (!(o instanceof Map)) { | ||
| throw new AwkRuntimeException( | ||
| position.lineNumber(), | ||
| "Cannot get a key list (via 'in') of a non associative array. arg = " + o.getClass() + ", " + o); | ||
| throw new AwkRuntimeException("Attempting to use a scalar as an array."); |
There was a problem hiding this comment.
When an untyped variable has already been fixed as an assigned-blank scalar, the preceding KEYLIST branch treats its UninitializedObject value as an empty array instead of reaching this type error. Consequently, BEGIN { print "[" a "]"; for (i in a); print "continued" } continues in Jawk, whereas gawk fails at the for loop; the analogous exemption in IS_IN incorrectly returns 0. Distinguish a truly untyped value from an assigned blank in these array-only paths, and cover both iteration and membership with AwkTestSupport.
AGENTS.md reference: AGENTS.md:L23-L26
Useful? React with 👍 / 👎.
| if (currentValue instanceof Map) { | ||
| return currentValue; | ||
| } | ||
| Object value = reference.snapshot(); |
There was a problem hiding this comment.
Fresh evidence beyond the array-transition cases is that scalarization through another aliased parameter is discarded here because only a current Map supersedes the snapshot. With function f(x,y) { print x; print typeof(y) } BEGIN { f(a,a) }, reading x fixes the shared actual as an unassigned scalar, so gawk reports unassigned for y, but Jawk still reports the call-time untyped. Raw reads must observe this scalar type transition without adopting later concrete scalar assignments, with an AwkTestSupport regression for the alias ordering.
AGENTS.md reference: AGENTS.md:L23-L26
Useful? React with 👍 / 👎.
|
@codex please review again |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 46a071d612
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } else if (getAst1() instanceof IDAst) { | ||
| IDAst idAst = (IDAst) getAst1(); | ||
| tuples.dereference(idAst.offset, true, idAst.isGlobal); |
There was a problem hiding this comment.
When the array base is a JRT-managed scalar such as FS or NR, this bypasses IDAst.populateTuples() and dereferences its ordinary global slot instead of reading the special value. Consequently, BEGIN { FS[1] = 2; print FS } silently creates an inaccessible array and continues, whereas gawk rejects using FS as an array; the analogous direct dereference in populateArrayOperandTuples() also makes cases such as 1 in FS, for (k in FS), and split("a", FS) incorrectly succeed. Preserve the dedicated special-variable read so runtime array validation sees the scalar, and add an AwkTestSupport regression.
AGENTS.md reference: AGENTS.md:L23-L26
Useful? React with 👍 / 👎.
Special scalars such as FS or NR used where an array is required now go through their JRT-managed read instead of a plain global dereference, so runtime array validation rejects them like gawk does (FS[1]=2, 1 in FS, for (k in NR), split(s, FS)). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@codex please review again |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Passing array elements or variables to user functions allocated a live reference per argument and registered element references in a weakly held set that every delete, split, and extension call swept, making such loops quadratic between garbage collections (32k iterations took 62s; 100k did not finish). Typed arguments are now materialized directly when pushed, so a reference is only created when the argument is genuinely untyped, and the remaining element references are adopted by the receiving call frame and released when it pops. The tracking stack is empty unless an untyped element argument is in flight, making the sweeps no-ops for normal scripts: 1M-iteration loops now run in a few seconds. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Pop user-function arguments directly into the callee frame instead of materializing an Object[] per call, and skip the argument-reference check when reading global variables, which can never hold one. Brings call-heavy and variable-heavy scripts back to pre-runtime-typing throughput (within benchmark noise). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
1 participant
Summary
Testing