code review 5266054: reflect: disallow Interface method on Value obtained vi...

reflect: disallow Interface method on Value obtained via unexported name

Had been allowing it for use by fmt, but it is too hard to lock down.
Fix other packages not to depend on it.

[rsc, 2011-10-17 18:50:07]

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

I'd like you to review this change to
https://go.googlecode.com/hg/

[r, 2011-10-17 18:59:48]

http://codereview.appspot.com/5266054/diff/1001/src/pkg/fmt/print.go
File src/pkg/fmt/print.go (right):

http://codereview.appspot.com/5266054/diff/1001/src/pkg/fmt/print.go#newcode654
src/pkg/fmt/print.go:654: p.fmtBool(f, verb, field, reflect.Value{})
there's a lot of creation and passing of reflection values being added here. can
we store the thing in p and avoid that? i think we can, and it would simplify
(and speed up) this process.

http://codereview.appspot.com/5266054/diff/1001/src/pkg/fmt/print.go#newcode734
src/pkg/fmt/print.go:734: if value.CanInterface() {
surely there's a refactoring here. there are too many pieces. even just a couple
of helper functions might do it.

[rsc, 2011-10-17 19:01:59]

On Oct 17, 2011 at 14:59, r@golang.org wrote:
>
http://codereview.appspot.com/5266054/diff/1001/src/pkg/fmt/print.go#newcode654
> src/pkg/fmt/print.go:654: p.fmtBool(f, verb, field, reflect.Value{})
> there's a lot of creation and passing of reflection values being added
> here. can we store the thing in p and avoid that? i think we can, and it
> would simplify (and speed up) this process.

The only creation is of zero values (reflect.Value{}).
I don't think that is slowing us down.

I'm not thrilled with it but it seems cleaner than
hiding state in p that code might or might not
remember to keep up to date.

Russ

[rsc, 2011-10-17 19:13:04]

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

Please take another look.

[rsc, 2011-10-17 19:13:23]

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

Please take another look.

[rsc, 2011-10-17 19:49:23]

Moving exp/template/html change into its own CL,
so that this CL is only about reflect and fmt.

[r2, 2011-10-17 19:57:02]

can you please see how the fmt benchmarks are affected?

-rob

[r, 2011-10-17 19:59:49]

http://codereview.appspot.com/5266054/diff/12002/src/pkg/fmt/print.go
File src/pkg/fmt/print.go (right):

http://codereview.appspot.com/5266054/diff/12002/src/pkg/fmt/print.go#newcode667
src/pkg/fmt/print.go:667: p.fmtBool(f, verb, field, reflect.Value{})
this new repetition in already overrepetitious code is bothering me.  can you at
least make it a local with a shorter name? (and for the curious: do all these
reflect.Values take up frame space?)

http://codereview.appspot.com/5266054/diff/12002/src/pkg/reflect/value.go
File src/pkg/reflect/value.go (right):

http://codereview.appspot.com/5266054/diff/12002/src/pkg/reflect/value.go#new...
src/pkg/reflect/value.go:907: panic("reflect.Value.Interface: cannot return
value obtained from unexported field or method")
is it still possible to avoid this check working by hand, without using unsafe?

http://codereview.appspot.com/5266054/diff/12002/src/pkg/reflect/value.go#new...
src/pkg/reflect/value.go:911: // Won't recurse further because an interface
cannot contain an interface.
s/recurse/recur/
english is recur, incorrect nerdspeak is recurse. easy to be correct.

[rsc, 2011-10-17 20:00:44]

On Mon, Oct 17, 2011 at 15:59,  <r@golang.org> wrote:
> is it still possible to avoid this check working by hand, without using
> unsafe?

yes, you can call value.CanInterface before calling value.Interface.

[r2, 2011-10-17 20:01:22]

On Oct 17, 2011, at 1:00 PM, Russ Cox wrote:

> On Mon, Oct 17, 2011 at 15:59,  <r@golang.org> wrote:
>> is it still possible to avoid this check working by hand, without using
>> unsafe?
> 
> yes, you can call value.CanInterface before calling value.Interface.

no, i meant the other way around. can i still break the rules?

-rob

[rsc, 2011-10-17 20:01:56]

On Mon, Oct 17, 2011 at 16:01, Rob 'Commander' Pike <r@google.com> wrote:
> no, i meant the other way around. can i still break the rules?

i don't believe so.
the intent is definitely not to let people break the rules anymore.

[r2, 2011-10-17 20:07:33]

On Oct 17, 2011, at 1:01 PM, Russ Cox wrote:

> On Mon, Oct 17, 2011 at 16:01, Rob 'Commander' Pike <r@google.com> wrote:
>> no, i meant the other way around. can i still break the rules?
> 
> i don't believe so.
> the intent is definitely not to let people break the rules anymore.

i understand the intent, it's the implementation i'm trying to figure out.

-rob

[rsc, 2011-10-17 22:29:08]

> i understand the intent, it's the implementation i'm trying to figure out.

the only place i am aware of where reflect was letting
clients cheat was in Interface - the appropriate check
was there, just commented out because it broke fmt.
this CL puts the check back - if the flagRO bit is set,
it means that the value was obtained by using an
unexported field, so it and any values obtained by using
it must be treated as read-only.  returning an int from
Interface would be fine, since ints are inherently
read-only, but returning a *int would not be, since
you could then write through it.

russ

[rsc, 2011-10-17 22:38:36]

Benchmarked: it is about 5% slower.

It might be interesting to move more of the parameters
into p or p.fmt, but let's do that refactoring in another CL.
This CL is about fixing the reflect bugs, and I'd like to
keep it as straightforward as possible so that we can be
confident about backporting it into r60.

Russ

[r, 2011-10-17 22:42:57]

LGTM but i'm not happy

[rsc, 2011-10-17 22:48:48]

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

reflect: disallow Interface method on Value obtained via unexported name

Had been allowing it for use by fmt, but it is too hard to lock down.
Fix other packages not to depend on it.

R=r, r
CC=golang-dev
http://codereview.appspot.com/5266054

[r, 2011-10-17 22:53:29]

http://codereview.appspot.com/5266054/diff/18002/src/pkg/fmt/print.go
File src/pkg/fmt/print.go (right):

http://codereview.appspot.com/5266054/diff/18002/src/pkg/fmt/print.go#newcode667
src/pkg/fmt/print.go:667: p.fmtBool(f, verb, field, reflect.Value{})
i asked you to tidy this up.

ok, i'll do a pass to restore it to my comfort zone.

[rsc, 2011-10-18 12:41:14]

On Mon, Oct 17, 2011 at 18:53,  <r@golang.org> wrote:
> src/pkg/fmt/print.go:667: p.fmtBool(f, verb, field, reflect.Value{})
> i asked you to tidy this up.
>
> ok, i'll do a pass to restore it to my comfort zone.

sorry, i planned to do that in the refactoring i promised
in a later CL.  i was going to take a crack at that today
but i will assume it's in your court unless i hear otherwise.

as for reflect.Value{} vs declaring and using a zero,
the compiler should have an easier job in the former case,
because it can synthesize zeros more easily than copy them.
if it doesn't do that well, i should fix that.  i started on making
some of the init stuff a bit cleaner a while back but only got
as far as fixing globals.

russ