go icon indicating copy to clipboard operation
go copied to clipboard

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

Open xrxtzz opened this issue 1 year ago • 3 comments

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/[email protected]/1.22.7/libexec'
GOSUMDB='sum.golang.google.cn'
GOTMPDIR=''
GOTOOLCHAIN='local'
GOTOOLDIR='/opt/homebrew/Cellar/[email protected]/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.

xrxtzz avatar Oct 18 '24 07:10 xrxtzz

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.

adonovan avatar Oct 18 '24 18:10 adonovan

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)...

xrxtzz avatar Oct 19 '24 07:10 xrxtzz

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

adonovan avatar Oct 21 '24 17:10 adonovan

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 avatar Nov 01 '24 18:11 matloob

@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.

matloob avatar Nov 01 '24 18:11 matloob