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

x/tools/go/packages: missing TypesInfo when NeedTypesInfo was set while NeedSyntax & NeedTypes were not #69931

Open
xrxtzz opened this issue Oct 18, 2024 · 6 comments
Labels
NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. Tools This label describes issues relating to any tools in the x/tools repository.
Milestone

Comments

@xrxtzz
Copy link

xrxtzz commented Oct 18, 2024

Go version

go1.22.7 darwin/arm64

Output of go env in your module/workspace:

GO111MODULE='on'
GOARCH='arm64'
GOBIN=''
GOCACHE='/Users/xrxtzz/Library/Caches/go-build'
GOENV='/Users/xrxtzz/Library/Application Support/go/env'
GOEXE=''
GOEXPERIMENT=''
GOFLAGS=''
GOHOSTARCH='arm64'
GOHOSTOS='darwin'
GOINSECURE=''
GOMODCACHE='/Users/xrxtzz/go/pkg/mod'
GONOPROXY='`*.everphoto.cn,git.smartisan.com'
GONOSUMDB='`*.everphoto.cn,git.smartisan.com'
GOOS='darwin'
GOPATH='/Users/xrxtzz/go'
GOPRIVATE='`*.everphoto.cn,git.smartisan.com'
GOPROXY='https://goproxy.cn,direct'
GOROOT='/opt/homebrew/Cellar/go@1.22/1.22.7/libexec'
GOSUMDB='sum.golang.google.cn'
GOTMPDIR=''
GOTOOLCHAIN='local'
GOTOOLDIR='/opt/homebrew/Cellar/go@1.22/1.22.7/libexec/pkg/tool/darwin_arm64'
GOVCS=''
GOVERSION='go1.22.7'
GCCGO='gccgo'
AR='ar'
CC='cc'
CXX='c++'
CGO_ENABLED='1'
GOMOD='/Users/xrxtzz/Documents/code/code_analysis/irfronts/go/go.mod'
GOWORK=''
CGO_CFLAGS='-O2 -g'
CGO_CPPFLAGS=''
CGO_CXXFLAGS='-O2 -g'
CGO_FFLAGS='-O2 -g'
CGO_LDFLAGS='-O2 -g'
PKG_CONFIG='pkg-config'
GOGCCFLAGS='-fPIC -arch arm64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -ffile-prefix-map=/var/folders/lk/nhh_2w2n2sv2kdzk75mfbrcr0000gp/T/go-build3609621157=/tmp/go-build -gno-record-gcc-switches -fno-common'

What did you do?

The NeedTypesInfo option indicates that packages.Load would add TypesInfo field to result packages. But it turns out that when NeedTypesInfo was used separately from NeedSyntax or NeedTypes, the TypesInfo fields would simply be nil.

package main

import (
	"fmt"

	"golang.org/x/tools/go/packages"
)

func main() {
	packs, _ := loadProgramPackageWithTypesInfo("strings")
	fmt.Println(packs[0].TypesInfo)
}

func loadProgramPackageWithTypesInfo(pattern string) ([]*packages.Package, error) {
	cfg := packages.Config{
		Mode: packages.NeedName | packages.NeedFiles | packages.NeedCompiledGoFiles |
			packages.NeedImports | packages.NeedTypesInfo | packages.NeedTypesSizes,
	}
	return packages.Load(&cfg, pattern)
}

What did you see happen?

When NeedTypesInfo was used separately from NeedSyntax or NeedTypes, in LoadMode of packages.Load, the TypesInfo fields would simply be nil.

What did you expect to see?

The TypesInfo were produced properly.

@gopherbot gopherbot added the Tools This label describes issues relating to any tools in the x/tools repository. label Oct 18, 2024
@gopherbot gopherbot added this to the Unreleased milestone Oct 18, 2024
@adonovan
Copy link
Member

types.Info is, fundamentally, the set of type-checker annotations on the syntax tree. You can neither compute it nor meaningfully use it without syntax trees. Indeed, the keys of the types.Info.Scopes mapping is a superset of the list of ast.Files in Packages.Syntax, so if you have Packages.TypesInfo you could derive Packages.Syntax from it. How did you plan to use TypesInfo without Syntax?

While the Need interface of go/packages is complicated and certainly has bugs, in this case I think it is working as intended. Perhaps the documentation could be improved.

@xrxtzz
Copy link
Author

xrxtzz commented Oct 19, 2024

Indeed, the keys of the types.Info.Scopes mapping is a superset of the list of ast.Files in Packages.Syntax, so if you have Packages.TypesInfo you could derive Packages.Syntax from it. How did you plan to use TypesInfo without Syntax?

Agree on that, but the behaviors of different combinations of LoadModes still make no sense, for example:

  • NeedSyntax & NeedTypesInfo won't actually give TypesInfo, though we do have Syntax here and can use TypesInfo well.
  • ...but NeedTypes & NeedTypesInfo can output TypesInfo, and in this case no Syntax was obtained.

Reason for above two cases is probably because *loader.loadPackage would simply return on tools/go/packages/packages.go:1162 unless NeedTypes was set. When I look into it deeper I found that indeed the LoadMode controlling here is pretty complicated and buggy (as indicated in other related issues)...

@adonovan
Copy link
Member

Possibly the right fix here is just:

--- a/go/packages/packages.go
+++ b/go/packages/packages.go
@@ -1158,7 +1158,7 @@ func (ld *loader) loadPackage(lpkg *loaderPackage) {
        }
 
        lpkg.Syntax = files
-       if ld.Config.Mode&NeedTypes == 0 {
+       if ld.Config.Mode&(NeedTypes|NeedTypesInfo) == 0 {
                return
        }

It's a little weird to ask for only TypesInfo but not Syntax or Types, but I don't see any reason we can't honor the request.

@matloob

@prattmic prattmic added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Oct 21, 2024
@tklauser tklauser changed the title x/tools/go/pacakges: missing TypesInfo when NeedTypesInfo was set while NeedSyntax & NeedTypes were not x/tools/go/packages: missing TypesInfo when NeedTypesInfo was set while NeedSyntax & NeedTypes were not Oct 22, 2024
@matloob
Copy link
Contributor

matloob commented Nov 1, 2024

Yeah it sounds like we should make that change. I agree that it's weird to ask for TypesInfo but not Types but that we should honor the request.

@matloob
Copy link
Contributor

matloob commented Nov 1, 2024

@adonovan I think we also need to request the necessary fields for type checking from the driver as is done in @xrxtzz's CL?

We should also update the documentation for TypesInfo to remove the line that says that it's set only if Syntax is set.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. Tools This label describes issues relating to any tools in the x/tools repository.
Projects
None yet
Development

No branches or pull requests

6 participants