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.

Sign up for GitHub

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

Jump to bottom

x/tools/go/packages: types.Package for all main packages has Path()="main" when Mode=LoadTypes #70742

Closed
adonovan opened this issue Dec 9, 2024 · 9 comments
Closed

x/tools/go/packages: types.Package for all main packages has Path()="main" when Mode=LoadTypes #70742

adonovan opened this issue Dec 9, 2024 · 9 comments
Assignees
@adonovan
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Tools This label describes issues relating to any tools in the x/tools repository.
Milestone
Unreleased

Comments

@adonovan
Copy link
Member

adonovan commented Dec 9, 2024

The patch below causes the go/packages/gopackages command to additionally log the values of types.Package.{Name,Path} fields. These are not the same as the go/packages.Package.{Name,PkgPath} fields, which are ordinarily printed!

diff --git a/go/packages/gopackages/main.go b/go/packages/gopackages/main.go
index aab3362db..2d78d8af3 100644
--- a/go/packages/gopackages/main.go
+++ b/go/packages/gopackages/main.go
@@ -192,6 +192,9 @@ func (app *application) print(lpkg *packages.Package) {
        if lpkg.Types != nil && lpkg.IllTyped && len(lpkg.Errors) == 0 {
                fmt.Printf("\thas an error among its dependencies\n")
        }
+       if lpkg.Types != nil {
+               fmt.Printf("\tTypes.Name=%q Types.Path=%s\n", lpkg.Types.Name(), lpkg.Types.Path())
+       }
 
        // source files
        for _, src := range lpkg.GoFiles {
diff --git a/go/packages/packages.go b/go/packages/packages.go
index 9dedf9777..e966c60dc 100644
--- a/go/packages/packages.go
+++ b/go/packages/packages.go
@@ -1044,6 +1044,9 @@ func (ld *loader) loadPackage(lpkg *loaderPackage) {
        // Call NewPackage directly with explicit name.
        // This avoids skew between golist and go/types when the files'
        // package declarations are inconsistent.
+       if lpkg.Name == "main" {
+               log.Printf("NewPackage %q %q %#v", lpkg.PkgPath, lpkg.Name, *lpkg.Package)
+       }
        lpkg.Types = types.NewPackage(lpkg.PkgPath, lpkg.Name)
        lpkg.Fset = ld.Fset

Observe that although packages.Package.Path has the correct value, types.Package.Path is "main" for gomodindex, and every other main package:

$ go run ./go/packages/gopackages -mode=types golang.org/x/tools/internal/modindex/...
2024/12/09 16:12:25 NewPackage "golang.org/x/tools/internal/modindex/gomodindex" "main" packages.Package{ID:"golang.org/x/tools/internal/modindex/gomodindex", Name:"main", PkgPath:"golang.org/x/tools/internal/modindex/gomodindex", ...}
...
Go command "golang.org/x/tools/internal/modindex/gomodindex":
	module golang.org/x/tools@
	package main
	has complete exported type info
	Types.Name="main" Types.Path=main
...

It should be an invariant that the types.Package fields (Name and Path) match the packages.Package fields.
@gopherbot gopherbot added the Tools This label describes issues relating to any tools in the x/tools repository. label Dec 9, 2024
@gopherbot gopherbot added this to the Unreleased milestone Dec 9, 2024
@gabyhelp
Copy link

gabyhelp commented Dec 9, 2024

Related Issues

(Emoji vote if this was helpful or unhelpful; more detailed feedback welcome in this discussion.)

@adonovan
Copy link
Member Author

adonovan commented Dec 9, 2024

It looks like cmd/compile emits path="main" into the export data for main packages, because that's the prefix for linker symbols in that package; so when you request NeedTypes, that's what you get. But that's not how go/types interprets the Package.Path, which is as a disambiguating mechanism that keeps various main packages' symbols from colliding.

The relevant types.NewPackage call is in xtools/internal/gcimporter/goreader.doPkg. This seems to be an effective workaround:

diff --git a/internal/gcimporter/ureader_yes.go b/internal/gcimporter/ureader_yes.go
index 1db408613..be6dc1082 100644
--- a/internal/gcimporter/ureader_yes.go
+++ b/internal/gcimporter/ureader_yes.go
@@ -266,7 +266,7 @@ func (pr *pkgReader) pkgIdx(idx pkgbits.Index) *types.Package {
 func (r *reader) doPkg() *types.Package {
        path := r.String()
        switch path {
-       case "":
+       case "main", "":
                path = r.p.PkgPath()
        case "builtin":
                return nil // universe

@timothy-king any thoughts?

@timothy-king
Copy link
Contributor

Interesting. I'd like to confirm where the "main" path string is coming from. I suspect src/cmd/compile/internal/gc/main.go:214. I'll keep looking.

Fix looks reasonable. What is the output of the modified command go run ./go/packages/gopackages -mode=types golang.org/x/tools/internal/modindex/... after the fix?

@adonovan
Copy link
Member Author

adonovan commented Dec 9, 2024

What is the output of the modified command go run ./go/packages/gopackages -mode=types golang.org/x/tools/internal/modindex/... after the fix?

The types.Packages have the correct Path (not "main").

@cagedmantis cagedmantis added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Dec 9, 2024
@timothy-king
Copy link
Contributor

Looks like the actual source of this is pkgPath in src/cmd/go/internal/work/gc.go on this example. It looks like the correct data would be filled back in by the patch.

Tech debt wise, the patch should be safe for the unified IR reader in noder as "main" is not importable.

All LGTM.

@adonovan
Copy link
Member Author

Tech debt wise, the patch should be safe for the unified IR reader in noder as "main" is not importable.

Do you mean that the analogous patch to cmd/compile would be safe? The patch above is to x/tools' ureader.

@timothy-king
Copy link
Contributor

Do you mean that the analogous patch to cmd/compile would be safe? The patch above is to x/tools' ureader.

Yes. I believe the analogous change to cmd/compile would be safe. Applying the change to both would help minimize the maintenance burden.

@adonovan adonovan self-assigned this Dec 10, 2024
@adonovan
Copy link
Member Author

Yes. I believe the analogous change to cmd/compile would be safe.

Thanks. CL on its way.

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/634922 mentions this issue: internal/gcimporter: set correct Package.Path for main packages

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@adonovan adonovan

Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Tools This label describes issues relating to any tools in the x/tools repository.
Projects
None yet
Milestone
Unreleased
Development

No branches or pull requests
5 participants
@cagedmantis @timothy-king @adonovan @gopherbot @gabyhelp