Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

flag.Parse() causes panic when printing help #1276

Closed
Bai-Yingjie opened this issue Oct 1, 2021 · 1 comment · Fixed by #1281
Closed

flag.Parse() causes panic when printing help #1276

Bai-Yingjie opened this issue Oct 1, 2021 · 1 comment · Fixed by #1281
Labels
area/core bug Something isn't working
Milestone
v0.13.x

Comments

@Bai-Yingjie
Copy link
Contributor

The following program sample.go triggers an unexpected result

package main

import (
	"flag"
	"fmt"
)

type customFlag struct{}

func (cf customFlag) String() string {
	return "custom flag"
}

func (cf customFlag) Set(string) error {
	return nil
}

func main() {
	flag.Var(customFlag{}, "cf", "custom flag")
	flag.Parse()
	fmt.Println("Hello, playground")
}

Expected result

$ go run cf.go -h
Usage of /tmp/go-build2330940394/b001/exe/cf:
  -cf value
        custom flag

Got

$ ./yaegi run cf.go -h
Usage of cf.go:
cf.go:19:2: panic
run: runtime error: invalid memory address or nil pointer dereferenc
goroutine 1 [running]:
runtime/debug.Stack(0x1, 0xc0002e0800, 0x40)
        /usr/local/go/src/runtime/debug/stack.go:24 +0x9f
github.com/traefik/yaegi/interp.(*Interpreter).Execute.func1(0xc000221b80)
        /repo/yingjieb/3rdparty/yaegi/interp/program.go:131 +0xc8
panic(0xe747c0, 0x15a1480)
        /usr/local/go/src/runtime/panic.go:965 +0x1b9
github.com/traefik/yaegi/interp.runCfg.func1(0xc0001bce70, 0xc00047d8c0, 0x0)
        /repo/yingjieb/3rdparty/yaegi/interp/run.go:193 +0x251
panic(0xe747c0, 0x15a1480)
        /usr/local/go/src/runtime/panic.go:965 +0x1b9
github.com/traefik/yaegi/stdlib._flag_Value.String(...)
        /repo/yingjieb/3rdparty/yaegi/stdlib/go1_16_flag.go:86
flag.isZeroValue(0xc0002d2bc0, 0xc00035e7d1, 0xb, 0xc00035e831)
        /usr/local/go/src/flag/flag.go:464 +0x104
flag.(*FlagSet).PrintDefaults.func1(0xc0002d2bc0)
        /usr/local/go/src/flag/flag.go:527 +0x22a
flag.(*FlagSet).VisitAll(0xc000184cc0, 0xc000221388)
        /usr/local/go/src/flag/flag.go:394 +0x62
flag.(*FlagSet).PrintDefaults(0xc000184cc0)
        /usr/local/go/src/flag/flag.go:510 +0x4e
flag.(*FlagSet).defaultUsage(0xc000184cc0)
        /usr/local/go/src/flag/flag.go:571 +0x85
flag.(*FlagSet).usage(0xc000184cc0)
        /usr/local/go/src/flag/flag.go:904 +0x2f
flag.(*FlagSet).parseOne(0xc000184cc0, 0xfaed01, 0x15f1890, 0xc0002215a0)
        /usr/local/go/src/flag/flag.go:946 +0x1df
flag.(*FlagSet).Parse(0xc000184cc0, 0xc000188070, 0x1, 0x1, 0x15f1890, 0x0)
        /usr/local/go/src/flag/flag.go:991 +0x65
flag.Parse()
        /usr/local/go/src/flag/flag.go:1022 +0x70
reflect.Value.call(0xe24220, 0xfabc18, 0x13, 0xf61c54, 0x4, 0x15f1890, 0x0, 0x0, 0x0, 0x0, ...)
        /usr/local/go/src/reflect/value.go:476 +0x8e7
reflect.Value.Call(0xe24220, 0xfabc18, 0x13, 0x15f1890, 0x0, 0x0, 0xf5ec60, 0x1, 0x15f1890)
        /usr/local/go/src/reflect/value.go:337 +0xb9
github.com/traefik/yaegi/interp.callBin.func1(0xe24220, 0xfabc18, 0x13, 0x15f1890, 0x0, 0x0, 0x0, 0x0, 0x0)
        /repo/yingjieb/3rdparty/yaegi/interp/run.go:1368 +0x65
github.com/traefik/yaegi/interp.callBin.func10(0xc0001bce70, 0xc000199b30)
        /repo/yingjieb/3rdparty/yaegi/interp/run.go:1537 +0x1c2
github.com/traefik/yaegi/interp.runCfg(0xc00047d8c0, 0xc0001bce70, 0xc00047cfc0, 0x0)
        /repo/yingjieb/3rdparty/yaegi/interp/run.go:201 +0xbe
github.com/traefik/yaegi/interp.(*Interpreter).run(0xc000224200, 0xc00047cfc0, 0xc0001bcd10)
        /repo/yingjieb/3rdparty/yaegi/interp/run.go:120 +0x2d2
github.com/traefik/yaegi/interp.(*Interpreter).Execute(0xc000224200, 0xc000482cc0, 0x0, 0x0, 0x0, 0x0, 0x0)
        /repo/yingjieb/3rdparty/yaegi/interp/program.go:157 +0x245
github.com/traefik/yaegi/interp.(*Interpreter).eval(0xc000224200, 0xc0001f2500, 0x128, 0x7ffe698dbb66, 0x5, 0x100, 0x129, 0x0, 0x0, 0x128, ...)
        /repo/yingjieb/3rdparty/yaegi/interp/interp.go:585 +0xd0
...

Yaegi Version

286d6c6

Additional Notes

When user inputs -h, flag.Parse() will call PrintDefaults(), this is where the panic triggered.

By studying package flag code, PrintDefaults() calls isZeroValue():

func isZeroValue(flag *Flag, value string) bool {
    ...
    z = reflect.Zero(typ)
    return value == z.Interface().(Value).String()
}

which calls the interface wrapper stdlib._flag_Value.String but with zero value receiver, which ends up with nil func call to W.WString

// _flag_Value is an interface wrapper for Value type
type _flag_Value struct {
	IValue  interface{}
	WSet    func(a0 string) error
	WString func() string
}

func (W _flag_Value) String() string      { return W.WString() }

A possible fix would be adding a nil checker in the template to generate code like this:

func (W _flag_Value) String() (r0 string) {
    if W.WString == nil {
        return
    }
    return W.WString()
}

I am new to yaegi and will be very happy to deliver this fix if it is acceptable.
@mvertes mvertes added area/core bug Something isn't working labels Oct 7, 2021
mvertes added a commit that referenced this issue Oct 7, 2021
@mvertes
interp: improve interface wrappers when used by reflect
4e83bd2 
When an interpreter type implementing an interface is
used by the runtime, the runtime can extract its type
and create new values using reflect, and call methods
on it. The problem is that there will be no interpreted
method counterpart in this case, which make wrapper panic.

Allow the String() method wrapper to always succeed and
return an empty string if no interpreted method is present.

This allow scripts to define custom flag.Value types on
which the runtime internally instantiates values using
reflect (see isZeroValue in Go src/flag/flag.go).

This workaround could be generalized to all wrappers if
necessary. At this moment, it is convenient to keep the
default behavior of expececting instantiated interpreter
methods, in order to catch interpreter bugs related to
interfaces.

Fixes #1276.
@mvertes mvertes mentioned this issue Oct 7, 2021
Merged
@mvertes
Copy link
Member

mvertes commented Oct 7, 2021 •
edited
Loading

@Bai-Yingjie Thanks, and congrats, your analysis is correct, but as wrappers are generated by extract using templates, I prefer to provide the fix myself in #1281. I fixed also another related problem due to unused arguments in wrapped methods, seen here in customFlag.Set method by:yaegi run ./cf.go -cf test.

Thanks

@mvertes mvertes added this to the v0.11.x milestone Oct 8, 2021
traefiker pushed a commit that referenced this issue Oct 8, 2021
@mvertes @traefiker
interp: improve interface wrappers when used by reflect
edfc252 
When an interpreter type implementing an interface is
used by the runtime, the runtime can extract its type
and create new values using reflect, and call methods
on it. The problem is that there will be no interpreted
method counterpart in this case, which make wrapper panic.

Allow the String() method wrapper to always succeed and
return an empty string if no interpreted method is present.

This allow scripts to define custom flag.Value types on
which the runtime internally instantiates values using
reflect (see isZeroValue in Go src/flag/flag.go).

This workaround could be generalized to all wrappers if
necessary. At this moment, it is convenient to keep the
default behavior of expececting instantiated interpreter
methods, in order to catch interpreter bugs related to
interfaces.

Fixes #1276.
traefiker pushed a commit that referenced this issue Oct 8, 2021
@mvertes
interp: improve interface wrappers when used by reflect
e56db3b 
When an interpreter type implementing an interface is
used by the runtime, the runtime can extract its type
and create new values using reflect, and call methods
on it. The problem is that there will be no interpreted
method counterpart in this case, which makes wrapper panic.

Allow the String() method wrapper to always succeed and
return an empty string if no interpreted method is present.

This allows scripts to define custom flag.Value types on
which the runtime internally instantiates values using
reflect (see isZeroValue in Go src/flag/flag.go).

This workaround could be generalized to all wrappers if
necessary. At this moment, it is convenient to keep the
default behavior of expecting instantiated interpreter
methods, in order to catch interpreter bugs related to
interfaces.

Fixes #1276.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
area/core bug Something isn't working
Projects
None yet
Milestone
v0.13.x
Development

Successfully merging a pull request may close this issue.

interp: improve interface wrappers when used by reflect traefik/yaegi
2 participants
@mvertes @Bai-Yingjie