Re: List of patches to review (mainly for named reference)

Hi Bart,

Great to hear that you could commit some of the patches. I'll have a look at the result, update my remaining patches and publish an updated list. I'm travelling the next few days and it will take me some time to remember all the details, so expect some delay.

A few quick replies below.

Finally had some time to look at this. I note you have not mentioned
workers/53791 in this summary.

The summary only included the patches that were prerequisites for my grand simplification of resolve_nameref and setscope, which was already plenty. That's why I didn't list workers/53791. There might be a few other patches that I have and that I didn't list (I will have to check).

Please clarify whether that patch is itself accomplished by applying
all the others listed here, or if it's yet another (not yet sent to
the list?) that depends on these? The only reference to it is
workers/53740 and I have the impression you've changed something since
then?

It's an additional patch. The latest version, which is indeed slightly different from workers/53740, can be seen in the link above (grand simplification ...) where it's built on top of all the patches listed in the summary. I'll produce an updated version based on your recent commits.

> - Don't follow namerefs when computing base scope of nameref (part 1). - workers/53782
> - Don't follow namerefs when computing base scope of nameref (part 2). - workers/53782
> - Only ever compute a base scope for non-upper references. - workers/53782

Almost gave up here because 53782 immediately threw a bunch of
conflicts, but I eventually figured out that the three diffs to
params.c (and corresponding tests) in that message were in the wrong
order ... by chopping it up and applying the first diff last, they all
succeed.

I vaguely remember that for some reason I reordered some patches in the branch where I included everything. I don't remember the details but that's probably why I listed them in the wrong order. Sorry about that.

> - Remove invalid upscope from createparam. - workers/53676

This has conflicts with 53797, which I have resolved in the most
obvious (and hopefully therefore correct) way.

Revisiting 53676 in the context of your other patches, I think the
[…]

I'll need more time to analyze and respond to this.

Philippe

On Mon, Oct 27, 2025 at 2:41 AM Bart Schaefer <schaefer@xxxxxxxxxxxxxxxx> wrote:

Finally had some time to look at this. I note you have not mentioned
workers/53791 in this summary.

On Sat, Jun 21, 2025 at 3:08 AM Philippe Altherr
<philippe.altherr@xxxxxxxxx> wrote:
>
> I refined my patch that simplifies resolve_nameref and setscope

Please clarify whether that patch is itself accomplished by applying
all the others listed here, or if it's yet another (not yet sent to
the list?) that depends on these? The only reference to it is
workers/53740 and I have the impression you've changed something since
then?

> - Don't allow exec in always blocks. - workers/53732

Applied that one first as it was the oldest and unrelated.

> The first 4 patches build each on top of the preceding ones:
>
> - Fix loading of autoload variables accessed via references. - workers/53781

This was fine.

> - Don't follow namerefs when computing base scope of nameref (part 1). - workers/53782
> - Don't follow namerefs when computing base scope of nameref (part 2). - workers/53782
> - Only ever compute a base scope for non-upper references. - workers/53782

Almost gave up here because 53782 immediately threw a bunch of
conflicts, but I eventually figured out that the three diffs to
params.c (and corresponding tests) in that message were in the wrong
order ... by chopping it up and applying the first diff last, they all
succeed.

> - Fix type conversions via assignments to references. - workers/53788

Holding this one for closer examination, for reasons of implementation
rather than outcome.

> - Remove bogus PM_LOCAL from call to resolve_nameref in setscope. - workers/53796
> - Remove bogus PM_LOCAL from the call to resolve_nameref in createparam. - workers/53797

These work fine.

> - Remove bogus self reference warning. - workers/53790
> - Fix imprecise invalid reference warning. - workers/53790

These are good too.

> - Fix error signaling for reference loops. - workers/53798

Also good.

> - Remove invalid upscope from createparam. - workers/53676

This has conflicts with 53797, which I have resolved in the most
obvious (and hopefully therefore correct) way.

Revisiting 53676 in the context of your other patches, I think the
concerns I had with createparam() have been made moot by the
introduction/use of upscope_upper(). However, (quoting from the patch
message):
> The patch changes the behavior of one regression test. The new behavior looks correct to me.
The test in question consists of two anonymous function calls, the
second of which was not meant to rely on the parameters assigned by
the first one. Your createparam() change alters the result of the
first call from failure to success, which as a side-effect alters the
behavior of the second call, so I'm no longer certain it's regressing
what was intended.

Inserting "unset zz" between the two calls accomplishes the original
intent of the second call, but then it's not obvious from "print"
what's being tested, so I think it's better to use "typeset -p". Here
is the hunk (not a full patch) that replaces that part of 53676:

======
@@ -1045,14 +1060,17 @@ F:relies on global TYPESET_TO_UNSET in %prep
() {
typeset -n foo; foo=zz
foo=zz || print -u2 foo: assignment failed
- print $bar $zz
+ typeset -p bar zz
}
- () { typeset -n foo; foo=zz; local zz; foo=zz; print $bar $zz }
+ # prior to workers/53676 the assignment failed
+ unset zz
+ () { typeset -n foo; foo=zz; local zz; foo=zz; typeset -p bar zz }
0:regression: local nameref may not in-scope a global parameter
F:previously this could create an infinite recursion and crash
->xx
->xx zz
-*?*foo: assignment failed
+>typeset -g bar=xx
+>typeset -g zz=zz
+>typeset -g bar=xx
+>typeset zz=zz

typeset -nm foo=bar
======