code review 774041: Add extensions to the generator.

Add extensions to the generator.

[dsymonds, 2010-03-26 12:07:15]

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

I'd like you to review this change.

[r, 2010-03-26 16:56:15]

can you please pull over the extensions test too? it might not be easy, so feel
free to push back.

http://codereview.appspot.com/774041/diff/7001/8001
File README (right):

http://codereview.appspot.com/774041/diff/7001/8001#newcode27
README:27: Extensions are not supported by the plugin in this release, although
fix this

http://codereview.appspot.com/774041/diff/7001/8002
File compiler/main.go (right):

http://codereview.appspot.com/774041/diff/7001/8002#newcode483
compiler/main.go:483: return &ExtensionDescriptor{common{file: file}, field,
parent}
doesn't seem worth the trouble of a function

http://codereview.appspot.com/774041/diff/7001/8003
File compiler/testdata/Makefile (right):

http://codereview.appspot.com/774041/diff/7001/8003#newcode47
compiler/testdata/Makefile:47: diff -u test.pb.go test.pb.go.golden
is this flag standard? never seen it.  feels to me like a portability issue; i'd
just drop it from the CL

[dsymonds, 2010-03-27 06:23:46]

The test is in, and updated to work in this environment, but it's failing at
present. I'll investigate what's going on (it may be something in the library).

http://codereview.appspot.com/774041/diff/7001/8001
File README (right):

http://codereview.appspot.com/774041/diff/7001/8001#newcode27
README:27: Extensions are not supported by the plugin in this release, although
On 2010/03/26 16:56:15, r wrote:
> fix this

Done.

http://codereview.appspot.com/774041/diff/7001/8002
File compiler/main.go (right):

http://codereview.appspot.com/774041/diff/7001/8002#newcode483
compiler/main.go:483: return &ExtensionDescriptor{common{file: file}, field,
parent}
On 2010/03/26 16:56:15, r wrote:
> doesn't seem worth the trouble of a function

Yep. It's gone.

http://codereview.appspot.com/774041/diff/7001/8003
File compiler/testdata/Makefile (right):

http://codereview.appspot.com/774041/diff/7001/8003#newcode47
compiler/testdata/Makefile:47: diff -u test.pb.go test.pb.go.golden
On 2010/03/26 16:56:15, r wrote:
> is this flag standard? never seen it.  feels to me like a portability issue;
i'd
> just drop it from the CL

It's very common these days (it produces a unified diff that's a ton easier to
read), but I'm fine with dropping it and putting it back in when I next need to
play with the golden file.

http://codereview.appspot.com/774041/diff/16001/17002
File compiler/main.go (right):

http://codereview.appspot.com/774041/diff/16001/17002#newcode233
compiler/main.go:233: func (d *FileDescriptor) needProtoImport() bool {
I opted to do this check at generate time instead of at wrap time, since it's
only going to get done once and would need params or fields scattered throughout
a few places to carry a boolean value around.

http://codereview.appspot.com/774041/diff/16001/17002#newcode920
compiler/main.go:920: extendedType := "*" +
g.TypeName(g.objectNamed(*ext.Extendee))
I fixed this up too. I somehow overlooked the TypeName and objectNamed methods.

[r, 2010-03-27 07:38:24]

seems good but let's get the test working

http://codereview.appspot.com/774041/diff/7001/8003
File compiler/testdata/Makefile (right):

http://codereview.appspot.com/774041/diff/7001/8003#newcode47
compiler/testdata/Makefile:47: diff -u test.pb.go test.pb.go.golden
i know of diffs that don't have it, so please drop it.  the output should be
empty anyway

[giles, 2010-03-27 09:49:41]

dsymonds@golang.org wrote:

> On 2010/03/26 16:56:15, r wrote:
> > is this flag standard? never seen it.  feels to me like a portability
> issue; i'd just drop it from the CL

It's not standardised by SUSv3 (the 2001 version of POSIX) and
is missing from at least some commercial Unixen.

OTOH Go already has build dependencies on bash, Python,
mercurial and some indirect dependencies those tools need such
including GNU make, so adding GNU diffutils wouldn't be the
end of the world, and 'diff -u' matches the 'hg diff' output
we're used to.

Sorry if this drifting too far off topic.

Giles

[dsymonds, 2010-03-29 03:39:50]

Tests fixed.

It turns out that the reflect.Type returned by reflect.Typeof differs
depending on the package path, even if it points to the same .6 file.
If I imported extension_base.pb as:
  import base "extension_base.pb"  // (A)
in the test then reflect.Typeof(base.BaseMessage) is different to if I
had imported it as:
  import base "./extension_base.pb".  // (B)

That caused the reflect.Typeof check in extensions.go (in
checkExtensionTypes) to fail.

This feels like unexpected and undesirable behavior. I can file an
issue about it if you agree. Regardless, this code review is ready to
proceed.


Dave.

[rsc, 2010-03-29 04:13:40]

> If I imported extension_base.pb as:
>  import base "extension_base.pb"  // (A)
> in the test then reflect.Typeof(base.BaseMessage) is different to if I
> had imported it as:
>  import base "./extension_base.pb".  // (B)

This is correct behavior: different import paths are
different package names, just as import "private/math"
is different from import "math".  The solution is not to do that.

Russ

[dsymonds, 2010-03-29 05:20:30]

On Mon, Mar 29, 2010 at 3:13 PM, Russ Cox <rsc@golang.org> wrote:

>> If I imported extension_base.pb as:
>>  import base "extension_base.pb"  // (A)
>> in the test then reflect.Typeof(base.BaseMessage) is different to if I
>> had imported it as:
>>  import base "./extension_base.pb".  // (B)
>
> This is correct behavior: different import paths are
> different package names, just as import "private/math"
> is different from import "math".  The solution is not to do that.

Yeah, I understand why it's happening, but it's still a weird
situation. It's not that I'm comparing import paths or package names;
I'm trying to compare types that are imported differently, but are
still the same type (and ultimately in the same .6 file). It seems
like it would be useful to have type equality well-defined in some way
that is modulo the import path. In the case here the programmer has no
control over one of the import paths (it's in generated code), and I
could easily imagine the same type coming via two different import
paths in a diamond-dependency situation.


Dave.

[dsymonds, 2010-03-29 05:24:15]

Furthermore, the docs do not imply that the import path matters (see
http://golang.org/pkg/reflect/#Type). In the description it is only
implying that types are scoped such that they are unique to a program,
not to an imported package. That may just be a shortcoming of that
doc, though.


Dave.

[rsc, 2010-03-29 05:28:31]

> that is modulo the import path. In the case here the programmer has no
> control over one of the import paths (it's in generated code), and I
> could easily imagine the same type coming via two different import
> paths in a diamond-dependency situation.

We made a conscious decision to be sticklers about always
importing a package by its canonical path.  If everything uses
the canonical import path, then this diamond won't happen.
It might be worth putting in a compiler warning or error if the
same file is reached by two different import paths, just to catch
things earlier on.

I don't quite understand your next message, about type scoping.
The fundamental issue is this: "extension_base.pb" and "./extension_base.pb"
are two different packages, just like "http/pprof" and "runtime/pprof"
are two different packages and so are "math" and "rpc".
Types from one are necessarily distinct from types from the other.
When you link the program, you get one set of everything from
"extension_base.pb" and a second set from "./extension_base.pb",
just like you get all of "math" and all of "rpc".

Russ

[dsymonds, 2010-03-29 05:44:23]

On Mon, Mar 29, 2010 at 4:28 PM, Russ Cox <rsc@golang.org> wrote:

> I don't quite understand your next message, about type scoping.
> The fundamental issue is this: "extension_base.pb" and "./extension_base.pb"
> are two different packages, just like "http/pprof" and "runtime/pprof"
> are two different packages and so are "math" and "rpc".
> Types from one are necessarily distinct from types from the other.
> When you link the program, you get one set of everything from
> "extension_base.pb" and a second set from "./extension_base.pb",
> just like you get all of "math" and all of "rpc".

I guess I was thinking that a type is a type is a type, and since
extension_base.pb.go should only need compiling once (hence the source
of much of Go's speed) then there will be only one underlying type
record for it so that two other pieces of code importing that will
have the same thing (e.g. they can do a reflect.Typeof(base.Blah) and
pass it around). I managed to track this down in this case, but it was
counter-intuitive that the same package imported different ways would
have different types. Does the linker duplicate the type info? Are the
compiled methods on those types all duplicated?


Dave.

[rsc, 2010-03-29 05:56:59]

You keep saying duplicate.  Yes they're duplicated, but that's
the wrong mental model.  They are different packages,
in exactly same way that "math" and "rpc" are different.
Just like the linker doesn't replace pieces of "math" with
declarations of the same name from "rpc", it doesn't replace
pieces of "extension_base.pb" with declarations of the same
name from "./extension_base.pb".

Russ

[dsymonds, 2010-04-03 01:49:13]

On Mon, Mar 29, 2010 at 2:39 PM, David Symonds <dsymonds@golang.org> wrote:

> Regardless, this code review is ready to proceed.

This code review got a bit side-tracked. Rob, can you take another
look when you get a chance?


Dave.

[r, 2010-04-06 18:46:03]

LGTM

[dsymonds, 2010-04-06 23:25:36]

*** Submitted as f6abe0eed582 ***

Add extensions to the generator.

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