Skip to content

add spec test for nested subshell parse error#2399

Merged
andychu merged 1 commit into
soil-stagingfrom
nested-subshell-spec
Aug 29, 2025
Merged

add spec test for nested subshell parse error#2399
andychu merged 1 commit into
soil-stagingfrom
nested-subshell-spec

Conversation

@melvinw
Copy link
Copy Markdown
Collaborator

@melvinw melvinw commented Aug 26, 2025

gzip has a utility script called zdiff. It gets used at some point in the check step when building gzip from source. It also makes use of nested subshells in a few places. The osh parser is tripping over these. Here's an example from a simple script.

((echo foo) | tr o 0)
     ^~~
_tmp/foo.sh:1: Unexpected token after arithmetic expression (Id.Word_Compound != Id.Arith_RParen)

Other shells seem to handle this case, so in preparation for an inbound fix, this commit adds a new spec test to cover it.

Related to #2398

@melvinw melvinw force-pushed the nested-subshell-spec branch from dd66ba4 to 23ecb4d Compare August 26, 2025 00:30
@melvinw melvinw changed the base branch from master to soil-staging August 26, 2025 00:39
@andychu
Copy link
Copy Markdown
Contributor

andychu commented Aug 26, 2025

Oh yes I saw this in the logs ... I wonder if this is the same as #2337

Awhile ago I added a couple tests in test/spec.sh divergence


I think this one might require some design; there is the issue of

  • (( vs ( ( with space
  • $(( vs $( ( with space - similar bug I think I may have seen

In theory I think regtest/aports/cause.awk can help us de-dupe these bugs, though probably we don't have a smooth process yet

@melvinw
Copy link
Copy Markdown
Collaborator Author

melvinw commented Aug 26, 2025

Oh great! We can close this then

@melvinw melvinw closed this Aug 26, 2025
@andychu
Copy link
Copy Markdown
Contributor

andychu commented Aug 28, 2025

I was thinking about this issue, and I realized it does help to have all the real examples encoded in the spec tests

The one I added was this

# spaces help
good() {
  cputype=`( ( (grep cpu /proc/cpuinfo | cut -d: -f2) ; ($PRTDIAG -v |grep -i sparc) ; grep -i cpu /var/run/dmesg.boot ) | head -n 1) 2> /dev/null`
}

bad() {
  cputype=`(((grep cpu /proc/cpuinfo | cut -d: -f2) ; ($PRTDIAG -v |grep -i sparc) ; grep -i cpu /var/run/dmesg.boot ) | head -n 1) 2> /dev/null`
  #echo cputype=$cputype
}

The good one has spaces for ( ( (


But this one is slightly different, so I think it's useful to have it too. And to note that it came from gzip. (hm I didn't note which package the other error is from)

Because we don't know what the exact solution is yet.

But they should be in the same file, I guess spec/divergence is OK

@andychu
Copy link
Copy Markdown
Contributor

andychu commented Aug 28, 2025

So yeah I think having the exact line helps, and then we can reduce to something more minimal as we fix it

The reducing process is often how I figure out what the bug actually is

@melvinw
Copy link
Copy Markdown
Collaborator Author

melvinw commented Aug 29, 2025

Ah ok. I'll reopen this and plop it in spec/divergence

@melvinw melvinw reopened this Aug 29, 2025
gzip has a utility script called zdiff. It gets used at some point in
the check step when building gzip from source. It also makes use of
nested subshells in a few places. The osh parser is tripping over these.
Here's an example from a simple script.

    ((echo foo) | tr o 0)
         ^~~
    _tmp/foo.sh:1: Unexpected token after arithmetic expression (Id.Word_Compound != Id.Arith_RParen)

Other shells seem to handle this case, so in preparation for an inbound
fix, this commit adds a new spec test to cover it.
@melvinw melvinw force-pushed the nested-subshell-spec branch from 23ecb4d to d7f11fd Compare August 29, 2025 00:10
@andychu andychu merged commit 8438843 into soil-staging Aug 29, 2025
19 checks passed
@andychu
Copy link
Copy Markdown
Contributor

andychu commented Aug 29, 2025

great thanks!

@melvinw melvinw deleted the nested-subshell-spec branch August 29, 2025 18:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants