Just reading your descriptions without looking at code yet, this seems
reasonable, but also seems like the sort of thing I'd rather put off
until the other threads we've been pursuing have been put to rest.
On Wed, Nov 19, 2025 at 6:25 AM Philippe Altherr
<philippe.altherr@xxxxxxxxx> wrote:
>
> Here is something that took me quite some time to analyze and understand the affected code. The proposed changes are simple but their correctness is not necessarily easy to understand. Hence the long explanations below.
>
> When Oliver suggested in workers/54053 to replace my new field isrefslice field with a new flag in the flags field, I thought that I could maybe do the same with the isarr field since its name suggests that it's a plain boolean. However, a quick look at the code showed that it's more than a boolean; it's also a set of SCANPM flags. Confusingly, there exist isarr variables here and here that are NOT sets of SCANPM flags. Even worse, there are places where such isarr variables are assigned to and from isarr fields. Furthermore there are places where isarr fields and variables are initialized with values that don't seem to match their specifications.
>
> After much head scratching, I came to the conclusion that some code should be changed such that the various isarr fields and variables can benefit from simpler specifications. Here is how I think they should be specified and what code should be changed:
>
> value field
>
> Specification: set of SCANPM flags that also plays the role of a boolean; a non-zero value signals that the value struct designates an array or an associative array.
>
> Rewrite v.isarr = (PM_TYPE(v.pm->node.flags) & (PM_ARRAY|PM_HASHED))
> This code is nonsensical; it assigns a PM flag to a field containing SCANPM flags. Assigning SCANPM_WANTVALS in case PM_ARRAY or PM_HASHED are set would be more correct. However, since the code is part of scanparamvals, which is only ever called with elements of associative arrays, which can only ever be scalars, the current code is in practice equivalent to v.isarr = 0.
>
> Rewrite v.isarr = 1
> Nothing fundamentally wrong here but an equivalent and cleaner version is v.isarr = SCANPM_WANTVALS. Even cleaner is (PM_TYPE(v.pm->node.flags) & PM_HASHED) ? SCANPM_WANTVALS : SCANPM_ARRONLY. where SCANPM_ARRONLY is a SCANPM flag dedicated to signal plain arrays.
>
> Patch: fix-isarr-1-value-field.txt
>
> multsub parameter
>
> Specification: an output "is-array" boolean that can be set to 0 or 1 as specified here.
>
> Fix *isarr = SCANPM_MATCHMANY
> This code signals that the result is an array. For some reason it uses a SCANPM constant while it should simply use 1 to conform to the specification of the isarr parameter.
>
> Patch: fix-isarr-2-multsub-parameter.txt
>
> paramsubst variable
>
> Specification: an extended "is-array" boolean that can be set to one of -1, 0, 1, or 2 as specified here.
>
> Fix v->isarr = isarr
> This code is nonsensical; it assigns the extended boolean isarr to the set of SCANPM flags v->isarr. Instead, the code should assign v->isarr with one or more SCANPM flags if isarr is non-zero and with zero otherwise. But which flags? The variable v is only passed to getindex and to getarrvalue and we know that v->pm points to an array if isarr is non-zero. If v->pm is an array then getarrvalue doesn't read v->isarr and getindex (almost) only cares about the boolean aspect of v->isarr. Given this, the best flag to initialize v->isarr seems to be SCANPM_ARRONLY whose purpose is to signal that v->pm is an array.
>
> I say that getindex almost only cares about the boolean aspect of v->isarr because it's possible to initialize v->isarr with a non-zero value that triggers a different behavior. For example, if v->isarr is initialized with (SCANPM_MATCHKEY | SCANPM_MATCHMANY) then array=(aaa bbb); echo ${array[1,2][1][1]} prints aaa instead of just a because then this condition fails to trigger for the second subscript of ${array[1,2][1][1]}. The fact that in this case.v->isarr isn't a pure boolean looks more like a bug than a feature to me. I suspect, but I haven't tried, that it would be possible to build an example where the current code produces a bogus result because a bad set of SCANPM flags (coming from the assignment discussed below) are assigned (in the assignment discussed here) to v->isarr.
>
> Fix isarr = v->isarr
> This code is nonsensical; it assigns the set of SCANPM flags v->isarr to the extended boolean isarr. An analysis of the code seems to indicate that the only cases that matter are whether v->isarr is non-zero and whether it includes the flag SCANPM_ISVAR_AT. If it includes that flag isarr must be set to -1. Otherwise, if v->isarr is non-zero, isarr must be set to 1 and if v->isarr is zero, isarr must be set to 0. Fixing this also removes the necessity for the constant SCANPM_ISVAR_AT to be negative. Instead, it can be defined using the next available bit.
>
> Patch: fix-isarr-3-paramsubst-variable.txt
>
> I noticed that the documentation of the flags parameter of getindex is outdated. The parameter is passed to getarg, which may check for SCANPM_NOEXEC and/or SCANPM_CHECKING. Patch: fix-isarr-4-documentation.txt.
>
> I also noticed that some code doesn't initialize all the fields of the value struct. Patch: fix-isarr-5-init-all-value-fields.txt.
>
> Finally, in order to avoid future confusion between plain/extended boolean isarr variables and the set of SCANPM flags in the isarr field of the value struct, I think that it would make sense to rename the isarr field into scanflags. However, if we do so, then we should also rename the already existing field flags into valflags, to avoid any confusion between the two flags fields. Patches: fix-isarr-6-rename-flags-field.txt and fix-isarr-7-rename-isarr-field.txt.
>
> All the patches are reviewable on GitHub: fix-isarr-code.
>
> In case you agree with everything, you can use the patch fix-isarr-all-in-one.txt, which includes the 7 patches listed above.
>
> Philippe
>
>
>