search

mvdan / sh

A shell parser, formatter, and interpreter with bash support; includes shfmt
https://pkg.go.dev/mvdan.cc/sh/v3
BSD 3-Clause "New" or "Revised" License
7.27k stars 345 forks source link

interp: panics on extglob nodes #841

Closed theclapp closed 2 years ago

theclapp commented 2 years ago

*(/) is zsh for "* but only for directories". Which it is of course fine if gosh doesn't handle, but it shouldn't panic, I wouldn't think.

% gosh
$ echo *(/)

panic: unhandled word part: *syntax.ExtGlob

goroutine 1 [running]:
mvdan.cc/sh/v3/expand.(*Config).wordFields(0xc000092000, {0xc000134450, 0x1, 0xc000028200})
    /Users/lmc/src/goget/src/github.com/mvdan/sh/expand/expand.go:637 +0x9e6
mvdan.cc/sh/v3/expand.Fields(0x1013746, {0xc000096010, 0x2, 0x0})
    /Users/lmc/src/goget/src/github.com/mvdan/sh/expand/expand.go:405 +0x212
mvdan.cc/sh/v3/interp.(*Runner).fields(0xc00012c280, {0xc000096010, 0xc00001c170, 0xc000132ba0})
    /Users/lmc/src/goget/src/github.com/mvdan/sh/interp/runner.go:170 +0x2d
mvdan.cc/sh/v3/interp.(*Runner).cmd(0xc00012c280, {0x11811b8, 0xc00001c170}, {0x1180af0, 0xc000132b70})
    /Users/lmc/src/goget/src/github.com/mvdan/sh/interp/runner.go:342 +0x1e35
mvdan.cc/sh/v3/interp.(*Runner).stmtSync(0xc00012c280, {0x11811b8, 0xc00001c170}, 0xc000136108)
    /Users/lmc/src/goget/src/github.com/mvdan/sh/interp/runner.go:288 +0x2b5
mvdan.cc/sh/v3/interp.(*Runner).stmt(0xc00012c280, {0x11811b8, 0xc00001c170}, 0xc000136108)
    /Users/lmc/src/goget/src/github.com/mvdan/sh/interp/runner.go:269 +0x176
mvdan.cc/sh/v3/interp.(*Runner).Run(0xc00012c280, {0x11811b8, 0xc00001c170}, {0x1180758, 0xc000136108})
    /Users/lmc/src/goget/src/github.com/mvdan/sh/interp/api.go:543 +0x11f
mvdan commented 2 years ago

FYI this is bash's extglob, which you can find in its man page; we don't support zsh yet.

The interpreter doesn't support expanding that syntax, simply because this feature is very rarely used, so it's not been a priority. I guess we could make the interpreter error instead of panic, though - would that help? I would happily approve and merge a PR doing that, as I agree that having the interpreter panic over a known TODO is a bit dramatic.

mvdan commented 2 years ago

For the sake of clarity, I'd be happy to review a PR implementing this change with tests, if anyone feels like taking a stab.