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.

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

go/types: Unclear what typ param in NewTypeName is #22516

Closed
willfaught opened this issue Oct 31, 2017 · 7 comments
Closed

go/types: Unclear what typ param in NewTypeName is #22516

willfaught opened this issue Oct 31, 2017 · 7 comments

Comments

@willfaught
Copy link
Contributor

@willfaught willfaught commented Oct 31, 2017

https://golang.org/pkg/go/types/#NewTypeName:

func NewTypeName(pos token.Pos, pkg *Package, name string, typ Type) *TypeName

Is it the underlying type? If so, that's confusing, because the underlying type param for NewNamed is named "underlying" and this one is not.

https://golang.org/pkg/go/types/#NewNamed:

func NewNamed(obj *TypeName, underlying Type, methods []*Func) *Named

To construct a new named type, it's confusing why you have to supply a types.Type twice, especially if the typ param of NewTypeName isn't the underlying type:

// Make a new type with name Mytypename and underlying type string for package with path mypkgpath and name mypkgname:

var myPkg = types.NewPackage("mypkgpath", "mypkgname")
var myTypeName = types.NewTypeName(token.NoPos, myPkg, "Mytypename", ??????????)
var myType = types.NewNamed(myTypeName, types.Typ[types.String], nil)

What should the argument be for the typ param of the NewTypeName call above (where ?????????? is)? Why does TypeName need it?

Does this issue reproduce with the latest release (go1.9.2)?

Yes

System details

go version go1.9.1 darwin/amd64
GOARCH="amd64"
GOBIN=""
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOOS="darwin"
GOPATH="/Users/willfaught/Developer/go"
GORACE=""
GOROOT="/usr/local/Cellar/go/1.9.1/libexec"
GOTOOLDIR="/usr/local/Cellar/go/1.9.1/libexec/pkg/tool/darwin_amd64"
GCCGO="gccgo"
CC="clang"
GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/_1/ggvd2t1x7hz_185crsb36zlr0000gp/T/go-build024849979=/tmp/go-build -gno-record-gcc-switches -fno-common"
CXX="clang++"
CGO_ENABLED="1"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOROOT/bin/go version: go version go1.9.1 darwin/amd64
GOROOT/bin/go tool compile -V: compile version go1.9.1
uname -v: Darwin Kernel Version 17.0.0: Thu Aug 24 21:48:19 PDT 2017; root:xnu-4570.1.46~2/RELEASE_X86_64
ProductName:	Mac OS X
ProductVersion:	10.13
BuildVersion:	17A365
lldb --version: lldb-900.0.50.1
  Swift-4.0
@griesemer griesemer self-assigned this Oct 31, 2017
@ianlancetaylor ianlancetaylor added this to the Go1.10 milestone Oct 31, 2017
@griesemer
Copy link
Contributor

@griesemer griesemer commented Oct 31, 2017

First of all, we don't use the issue tracker for questions. Also, please, refrain from shouting questions by ending them in '???????...'; the design is not as insane as the '?'s imply... Thanks.

Regarding the documentation:

NewTypeName creates a new TypeName as the signature implies. That object, TypeName, represents a name for a named or alias type. That's in fact documented with TypeName, 5 lines above. Thus, the typ argument may or may not be the underlying type, and hence it's not called underlying, exactly because that would be confusing.

NewNamed creates a new NamedType, where NamedType is the historical name we used for what is now called a defined type in the spec. For backward-compatibility we couldn't change those names. A named (or defined) type has an underlying type, which is the type on which the new named type is based.

A NamedType points to its TypeName which points back to its NamedType. If you read the 8 lines of code in NewNamed you will see how that connection is established.

The go/types API is not really documented for general consumption (it would fill reams of pages); it does require a bit of digging and reading the code.

Loading

@willfaught
Copy link
Contributor Author

@willfaught willfaught commented Nov 1, 2017

Loading

@griesemer
Copy link
Contributor

@griesemer griesemer commented Nov 1, 2017

I see what you're saying. The many ?'s threw me off; I didn't see that they meant to be a place holder; sorry about that.

I understood it's a documentation issue, and so I left it open because of that. (Just saying that it's unclear from documentation what/which type you need to provide to NewTypeName would probably be enough; especially because that method actually is missing a doc string. It would have been pretty clear.)

But to your question: If you look at NewNamed, as I have alluded to in my prior comment, you will see that NewNamed actually connects back from the provided TypeName to the Named type, if necessary. So you can call NewTypeName with a nil type and then provide it to NewNamed which will then connect them.

Btw., I didn't say go/types isn't meant for general consumption; I said it's not documented for general consumption.

The primary use case for go/types is to produce type information for a given package, which then can be queried. The primary reason why methods and functions for constructing go/types data structures are exposed is so that we can write importers (which are in different packages). When we wrote it we (Alan Donovan and I) agonized how to get around this, well knowing that it would lead to endless troubles down the road exactly because proper use of these constructors requires intimate knowledge of how type graphs are constructed; something that we didn't meant to expose for general consumption. Hence the missing documentation.

Loading

@gopherbot
Copy link

@gopherbot gopherbot commented Nov 3, 2017

Change https://golang.org/cl/75595 mentions this issue: go/types: add missing documentation to Object factory functions

Loading

@dominikh
Copy link
Member

@dominikh dominikh commented Nov 3, 2017

For the record: I'm glad that these functions are exported and am using them to implement a custom, database-backed importer.

Loading

@willfaught
Copy link
Contributor Author

@willfaught willfaught commented Nov 3, 2017

Just saying that it's unclear from documentation what/which type you need to provide to NewTypeName would probably be enough; especially because that method actually is missing a doc string. It would have been pretty clear.

Makes sense.

If you look at NewNamed, as I have alluded to in my prior comment, you will see that NewNamed actually connects back from the provided TypeName to the Named type, if necessary. So you can call NewTypeName with a nil type and then provide it to NewNamed which will then connect them.

Makes sense. Sorry, I was on my phone and didn't have the source in front of me to check out.

Btw., I didn't say go/types isn't meant for general consumption; I said it's not documented for general consumption.

My mistake. I see what you mean, but in my mind, if a package is meant for general consumption, it should be documented as such.

well knowing that it would lead to endless troubles down the road exactly because proper use of these constructors requires intimate knowledge of how type graphs are constructed; something that we didn't meant to expose for general consumption. Hence the missing documentation.

If the constructors aren't meant to be used, it would be clearer if their documentation said so. Or perhaps it could say they're deprecated. Leaving them out there and not documented can be frustrating to newcomers who don't know.

There don't seem to be that many complications, though. Would the additional documentation be that much more? This documentation issue, for example, could be explained with two additional lines in the NewTypeName and NewNamed documentation:

// NewTypeName [...]. If the type name will be used to make a named type, the type
// should be nil, and NewNamed will set it.
func NewTypeName(pos token.Pos, pkg *Package, name string, typ Type) *TypeName

// NewNamed [...]. It sets the type name's type to the named type.
func NewNamed(obj *TypeName, underlying Type, methods []*Func) *Named

...and I just saw the associated changelist! Thank you. :)

Loading

@willfaught
Copy link
Contributor Author

@willfaught willfaught commented Nov 3, 2017

@dominikh Agreed.

Loading

@gopherbot gopherbot closed this in d593f85 Nov 3, 2017
@golang golang locked and limited conversation to collaborators Nov 3, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants