code review 6499102: go/build: reject empty strings in Import

go/build: reject empty strings in Import

Fixes issue 3889.

[fss, 2012-09-12 20:24:31]

Hello rsc@golang.org (cc: golang-dev@googlegroups.com),

I'd like you to review this change to
https://code.google.com/p/go

[adg, 2012-09-12 23:36:58]

My intuition suggests that this should probably be a panic instead of
an error return.

[fss, 2012-09-13 00:30:20]

Hello rsc@golang.org, adg@golang.org (cc: golang-dev@googlegroups.com),

Please take another look.

[fss, 2012-09-13 00:30:57]

On Wed, Sep 12, 2012 at 8:36 PM, Andrew Gerrand <adg@golang.org> wrote:
> My intuition suggests that this should probably be a panic instead of
> an error return.

Indeed. One tool that uses this function is godoc. This is how it's
affected by issue 3889:

No panic:

% godoc ""
panic: runtime error: invalid memory address or nil pointer dereference
[signal 0xb code=0x1 addr=0x8 pc=0x1d22e]

goroutine 1 [running]:
main.main()
	/Users/f/lib/go/src/cmd/godoc/main.go:353 +0x340e

goroutine 2 [syscall]:
created by runtime.main
	/Users/f/lib/go/src/pkg/runtime/proc.c:220

goroutine 14 [runnable]:
text/template/parse.lexText(0xf840185070, 0x1c1da0, 0x397e0, 0xf840185070)
	/Users/f/lib/go/src/pkg/text/template/parse/lex.go:227 +0x345
text/template/parse.(*lexer).run(0xf840185070, 0x0)
	/Users/f/lib/go/src/pkg/text/template/parse/lex.go:197 +0x3b
created by text/template/parse.lex
	/Users/f/lib/go/src/pkg/text/template/parse/lex.go:190 +0x113

With panic:

% godoc ""
panic: Can't import "".

goroutine 1 [running]:
go/build.(*Context).Import(0x5e9050, 0x7fff5fbffa5e, 0xf800000000,
0x3cb95c, 0xf800000000, ...)
	/Users/f/lib/go/src/pkg/go/build/build.go:352 +0x63
----- stack segment boundary -----
go/build.Import(0x7fff5fbffa5e, 0x7fff00000000, 0x3cb95c, 0x0,
0xf800000001, ...)
	/Users/f/lib/go/src/pkg/go/build/build.go:658 +0x5d
main.main()
	/Users/f/lib/go/src/cmd/godoc/main.go:353 +0x33f9

We also could fix godoc to stop ignoring errors returned by
build.Import, but panicking fits too.

[adg, 2012-09-13 00:35:34]

http://codereview.appspot.com/6499102/diff/7002/src/pkg/go/build/build.go
File src/pkg/go/build/build.go (right):

http://codereview.appspot.com/6499102/diff/7002/src/pkg/go/build/build.go#new...
src/pkg/go/build/build.go:352: panic(`Can't import "".`)
panic(`go/build: tried to Import invalid path ""`)

[adg, 2012-09-13 00:37:05]

On 2012/09/13 00:35:34, adg wrote:
> http://codereview.appspot.com/6499102/diff/7002/src/pkg/go/build/build.go
> File src/pkg/go/build/build.go (right):
> 
>
http://codereview.appspot.com/6499102/diff/7002/src/pkg/go/build/build.go#new...
> src/pkg/go/build/build.go:352: panic(`Can't import "".`)
> panic(`go/build: tried to Import invalid path ""`)

actually, a simpler message:

panic(`go/build: cannot import empty path`)

[rsc, 2012-09-13 03:10:19]

On 2012/09/12 23:36:58, adg wrote:
> My intuition suggests that this should probably be a panic instead of
> an error return.

Can you explain why? I would have expected

return p, fmt.Errorf("import %q: invalid import path", path)

Functions that return errors usually do that in preference to panicking.

[adg, 2012-09-13 03:18:08]

On 13 September 2012 13:10,  <rsc@golang.org> wrote:
> On 2012/09/12 23:36:58, adg wrote:
>>
>> My intuition suggests that this should probably be a panic instead of
>> an error return.
>
>
> Can you explain why? I would have expected
>
> return p, fmt.Errorf("import %q: invalid import path", path)
>
> Functions that return errors usually do that in preference to panicking.

Do they? I have seen both in practice, and my understanding is that
inputs that are just plain wrong should cause a panic.

But I'm on the fence in this particular case, so I'll defer to your judgement.

[fss, 2012-09-13 13:49:22]

On 2012/09/13 03:18:08, adg wrote:
> On 13 September 2012 13:10,  <mailto:rsc@golang.org> wrote:
> > On 2012/09/12 23:36:58, adg wrote:
> >>
> >> My intuition suggests that this should probably be a panic instead of
> >> an error return.
> >
> >
> > Can you explain why? I would have expected
> >
> > return p, fmt.Errorf("import %q: invalid import path", path)
> >
> > Functions that return errors usually do that in preference to panicking.
> 
> Do they? I have seen both in practice, and my understanding is that
> inputs that are just plain wrong should cause a panic.
> 
> But I'm on the fence in this particular case, so I'll defer to your judgement.

Thinking again about it, I realize that it should be an error return. go/build
is a package, a library, that provides an exported function with the signature:
Import(string, string, ImportMode) (*Package, error).

If someone writes a program that can handle errors returned by Import, and
continue to run or even give a better error message for the user (like godoc
should do), the package function should return an error, not an unrecovered
panic.

As discussed in go-nuts multiple times, a library should not issue an
unrecovered panic. One exception is regexp.MustCompile, because it receives a
constant defined by the developer (like Import would do, in the normal case). If
I'd write a command line tool that compiles regular expressions, I would prefer
regexp.Compile over regexp.MustCompile.

For build.Import, having a `MustImport` doesn't make sense, just `Import` that
returns a non-nil error in case of failures.

[fss, 2012-09-13 13:49:53]

Hello rsc@golang.org, adg@golang.org (cc: golang-dev@googlegroups.com),

Please take another look.

[rsc, 2012-09-13 14:05:51]

Looks good except for test nits. Thanks.

http://codereview.appspot.com/6499102/diff/5003/src/pkg/go/build/build_test.go
File src/pkg/go/build/build_test.go (right):

http://codereview.appspot.com/6499102/diff/5003/src/pkg/go/build/build_test.g...
src/pkg/go/build/build_test.go:67: t.Fatal("Import should not accept empty
strings.")
Use Go syntax to explain what happened.

t.Fatalf(`Import("") returned nil error`)

http://codereview.appspot.com/6499102/diff/5003/src/pkg/go/build/build_test.g...
src/pkg/go/build/build_test.go:70: t.Fatal("Import should return non-nil package
containing partial information.")
t.Fatal(`Import("") returned nil Package`)

http://codereview.appspot.com/6499102/diff/5003/src/pkg/go/build/build_test.g...
src/pkg/go/build/build_test.go:73: t.Fatalf(`Wrong import path, want "", got
%s.`, p.ImportPath)
Please reverse the message to print in the same order you wrote the if
statement.

t.Fatalf("ImportPath=%q, want %q", p.ImportPath, "")

[fss, 2012-09-13 14:09:41]

Please take another look.

https://codereview.appspot.com/6499102/diff/5003/src/pkg/go/build/build_test.go
File src/pkg/go/build/build_test.go (right):

https://codereview.appspot.com/6499102/diff/5003/src/pkg/go/build/build_test....
src/pkg/go/build/build_test.go:67: t.Fatal("Import should not accept empty
strings.")
On 2012/09/13 14:05:51, rsc wrote:
> Use Go syntax to explain what happened.
> 
> t.Fatalf(`Import("") returned nil error`)

Done.

https://codereview.appspot.com/6499102/diff/5003/src/pkg/go/build/build_test....
src/pkg/go/build/build_test.go:70: t.Fatal("Import should return non-nil package
containing partial information.")
On 2012/09/13 14:05:51, rsc wrote:
> t.Fatal(`Import("") returned nil Package`)

Done.

https://codereview.appspot.com/6499102/diff/5003/src/pkg/go/build/build_test....
src/pkg/go/build/build_test.go:73: t.Fatalf(`Wrong import path, want "", got
%s.`, p.ImportPath)
On 2012/09/13 14:05:51, rsc wrote:
> Please reverse the message to print in the same order you wrote the if
> statement.
> 
> t.Fatalf("ImportPath=%q, want %q", p.ImportPath, "")

Done.

[rsc, 2012-09-13 14:22:57]

>> Functions that return errors usually do that in preference to panicking.
>
> Do they? I have seen both in practice, and my understanding is that
> inputs that are just plain wrong should cause a panic.

We panic in functions that get plain wrong inputs only when there is
no way to return an error. For example os.Open("") does not panic. As
another example, we just changed syscall.StringBytePtr to panic on an
invalid input, but only because we had no way to return an error from
it in the current API. But we also introduced BytePtrFromString which
does return an error, and then we deprecated StringBytePtr in favor of
it.

If you know of a function with an error return that panics instead of
using the error return, please point it out. We should fix it.

Thanks.
Russ

[rsc, 2012-09-13 14:24:28]

LGTM

[rsc, 2012-09-13 14:26:15]

*** Submitted as http://code.google.com/p/go/source/detail?r=77aaafa3b2e2 ***

go/build: reject empty strings in Import

Fixes issue 3889.

R=rsc, adg
CC=golang-dev
http://codereview.appspot.com/6499102

Committer: Russ Cox <rsc@golang.org>