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

cmd/go: SWIG causes generated C++ sources in CompiledGoFiles #37098

Open
noahgoldman opened this issue Feb 7, 2020 · 8 comments
Open

cmd/go: SWIG causes generated C++ sources in CompiledGoFiles #37098

noahgoldman opened this issue Feb 7, 2020 · 8 comments

Comments

@noahgoldman
Copy link

@noahgoldman noahgoldman commented Feb 7, 2020

What version of Go are you using (go version)?

$ go version
go version go1.13.7 darwin/amd64

Does this issue reproduce with the latest release?

Yes

What operating system and processor architecture are you using (go env)?

go env Output
$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/ngoldman/Library/Caches/go-build"
GOENV="/Users/ngoldman/Library/Application Support/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GONOPROXY=""
GONOSUMDB=""
GOOS="darwin"
GOPATH="/Users/ngoldman/projects/golang"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/Cellar/go/1.13.7/libexec"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/Cellar/go/1.13.7/libexec/pkg/tool/darwin_amd64"
GCCGO="gccgo"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD="/Users/ngoldman/projects/pixelserver/go.mod"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/vz/db5rfql17xndlv94d_cxrw1c0000gr/T/go-build075487118=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

The function (golang.org/x/tools/go/packages).Load was called on a package that uses Swig to generate C++ interfaces. The minimal case to reproduce this behavior is to create a Go source with nothing but a package declaration package x, as well as an empty Swig interface file with the extension .swigcxx.

I can reproduce this with the following test case (utilizing the packagestest package).

package swigtest

import (
	"go/parser"
	"go/token"
	"io/ioutil"
	"testing"

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

func TestSwigCompiledFilesCXX(t *testing.T) { packagestest.TestAll(t, testSwigCompiledFilesCXX) }
func testSwigCompiledFilesCXX(t *testing.T, exporter packagestest.Exporter) {
	exported := packagestest.Export(t, exporter, []packagestest.Module{{
		Name: "golang.org/fake",
		Files: map[string]interface{}{
			"a/a.go":      "package a",
			"a/a.swigcxx": "",
		}}})
	defer exported.Cleanup()

	initial, err := packages.Load(exported.Config, "golang.org/fake/a")
	if err != nil {
		t.Fatalf("failed to load the package: %v", err)
	}
	// Try and parse each of the files
	for _, pkg := range initial {
		for _, file := range pkg.CompiledGoFiles {
			fset := token.NewFileSet()
			_, err := parser.ParseFile(fset, file, nil, parser.ImportsOnly)
			if err != nil {
				t.Errorf("failed to parse file '%s': %v", file, err)

				contents, err := ioutil.ReadFile(file)
				if err != nil {
					t.Fatalf("failed to read the un-parsable file '%s': %v", file, err)
				}

				t.Logf("Contents of un-parsable file: %s", contents[:1000])
			}
		}
	}
}

What did you expect to see?

I'd expect that that (golang.org/x/tools/go/packages).Load would return *Package types, for which each value of CompiledGoFiles would be a Go source file, not a C++ source.

The discussion in #28749 and the subsequent change to golang.org/x/tools/go/packages indicates that the intention is to only contain Go sources in (*Package).CompiledGoFiles. The current behavior of removing sources present in (*Package).OtherFiles from (*Package).CompiledGoFiles (see golist.go#L546) will filter out any files defined in most of the other fields returned from go list ... (see golist.go#L400).

What did you see instead?

The output of (golang.org/x/tools/go/packages).Load on a package that uses Swig to generate C++ interfaces results in the package containing Swig-generated C++ in (*Package).CompiledGoFiles.

Here are the results of running the test case above. The test case prints the first 1000 characters of the non-Go sources to show that they are C++ files.

--- FAIL: TestSwigCompiledFilesCXX (1.49s)
    --- FAIL: TestSwigCompiledFilesCXX/GOPATH (0.75s)
        swig_test.go:33: failed to parse file '/Users/ngoldman/Library/Caches/go-build/bf/bfad65d54097c367b8e59c1820fb6d949ec7321895ea57c2f5748e961325b7c5-d': /Users/ngoldman/Library/Caches/go-build/bf/bfad65d54097c367b8e59c1820fb6d949ec7321895ea57c2f5748e961325b7c5-d:13:1: illegal character U+0023 '#'
        swig_test.go:40: Contents of un-parsable file: /* ----------------------------------------------------------------------------
             * This file was automatically generated by SWIG (http://www.swig.org).
             * Version 4.0.1
             *
             * This file is not intended to be easily readable and contains a number of
             * coding conventions designed to improve portability and efficiency. Do not make
             * changes to this file unless you know what you are doing--modify the SWIG
             * interface file instead.
             * ----------------------------------------------------------------------------- */
            
            // source: a.swigcxx
            
            #define SWIGMODULE a
            
            #ifdef __cplusplus
            /* SwigValueWrapper is described in swig.swg */
            template<typename T> class SwigValueWrapper {
              struct SwigMovePointer {
                T *ptr;
                SwigMovePointer(T *p) : ptr(p) { }
                ~SwigMovePointer() { delete ptr; }
                SwigMovePointer& operator=(SwigMovePointer& rhs) { T* oldptr = ptr; ptr = 0; delete oldptr; ptr = rhs.ptr; rhs.ptr = 0; return *this; }
              } pointer;
              SwigValueWrapper& operator=(const SwigValueWrapper<T>
    --- FAIL: TestSwigCompiledFilesCXX/Modules (0.74s)
        swig_test.go:33: failed to parse file '/Users/ngoldman/Library/Caches/go-build/bf/bfad65d54097c367b8e59c1820fb6d949ec7321895ea57c2f5748e961325b7c5-d': /Users/ngoldman/Library/Caches/go-build/bf/bfad65d54097c367b8e59c1820fb6d949ec7321895ea57c2f5748e961325b7c5-d:13:1: illegal character U+0023 '#'
        swig_test.go:40: Contents of un-parsable file: /* ----------------------------------------------------------------------------
             * This file was automatically generated by SWIG (http://www.swig.org).
             * Version 4.0.1
             *
             * This file is not intended to be easily readable and contains a number of
             * coding conventions designed to improve portability and efficiency. Do not make
             * changes to this file unless you know what you are doing--modify the SWIG
             * interface file instead.
             * ----------------------------------------------------------------------------- */
            
            // source: a.swigcxx
            
            #define SWIGMODULE a
            
            #ifdef __cplusplus
            /* SwigValueWrapper is described in swig.swg */
            template<typename T> class SwigValueWrapper {
              struct SwigMovePointer {
                T *ptr;
                SwigMovePointer(T *p) : ptr(p) { }
                ~SwigMovePointer() { delete ptr; }
                SwigMovePointer& operator=(SwigMovePointer& rhs) { T* oldptr = ptr; ptr = 0; delete oldptr; ptr = rhs.ptr; rhs.ptr = 0; return *this; }
              } pointer;
              SwigValueWrapper& operator=(const SwigValueWrapper<T>
FAIL
@gopherbot gopherbot added this to the Unreleased milestone Feb 7, 2020
@gopherbot gopherbot added the Tools label Feb 7, 2020
@cagedmantis
Copy link
Contributor

@cagedmantis cagedmantis commented Feb 7, 2020

/cc @matloob

@matloob
Copy link
Contributor

@matloob matloob commented Feb 7, 2020

This looks like a valid problem. I wonder why this is happening? The SwigCXXFiles should be put in other sources along with the rest of the non-go source files.

@heschik heschik changed the title x/tools/packages: Swig causes generated C++ sources in CompiledGoFiles cmd/go: SWIG causes generated C++ sources in CompiledGoFiles Mar 4, 2020
@heschik heschik added the GoCommand label Mar 4, 2020
@heschik
Copy link
Contributor

@heschik heschik commented Mar 4, 2020

@stamblerre stamblerre removed the Tools label Apr 14, 2020
@matloob
Copy link
Contributor

@matloob matloob commented Apr 14, 2020

Hi, @noahgoldman, would you like to contribute your repro case to packages_test with a t.Skip()?

That will make it a bit easier for us to take the next steps on getting to a fix.

Thanks!

@gopherbot
Copy link

@gopherbot gopherbot commented Apr 16, 2020

Change https://golang.org/cl/228637 mentions this issue: go/packages: add failing test for golang/go#37098

@noahgoldman
Copy link
Author

@noahgoldman noahgoldman commented Apr 16, 2020

Hi, @noahgoldman, would you like to contribute your repro case to packages_test with a t.Skip()?

@matloob I've created a review for this (see comment above). Please let me know if this looks correct!

@matloob
Copy link
Contributor

@matloob matloob commented Apr 20, 2020

Thanks!

gopherbot pushed a commit to golang/tools that referenced this issue Jul 23, 2020
When SWIG is used in a package, the returned "(*package.Package)" from
"packages.Load" contains C++ sources in "CompiledGoFiles".  This adds a
skipped failing test case that reproduces this issue.

Updates golang/go#37098.

Change-Id: I56a875c6ad9d18508413b00c34d6b6ac0b116fda
Reviewed-on: https://go-review.googlesource.com/c/tools/+/228637
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Michael Matloob <matloob@golang.org>
myitcv added a commit to govim/govim that referenced this issue Jul 29, 2020
* internal/lsp/source: add extra nil check in searchForEnclosing df70183b
* internal/lsp: fix logged message for gc_details e8769ccb
* internal/lsp: remove uses of crypto/sha1 in gopls 0dd562f3
* internal/lsp: show compiler optimization decisions 95780ea8
* internal/lsp: de-duplicate logic for canExtractVariable edd3c8e9
* internal/lsp: check URIs of all workspace folders 342ee105
* internal/lsp/source: add a unit test for searchForEnclosing e7a7e3a8
* internal/lsp: remove PackageHandle b6476686
* internal/lsp: replace ParseGoHandle with concrete data a9439ae9
* internal/lsp/cache: fix parseKey 4d1d9acc
* internal/lsp: pass snapshot/view to memoize.Functions 72051f79
* internal/lsp/cache: remove files from the memoize.Store 60da08ac
* internal/lsp: minimize PackageHandle interface 051e64e6
* internal/lsp: handle bad formatting with CRLF line endings 2ad651e9
* internal/lsp: support return statements in extract function 3c048e20
* internal/lsp: update code for LSP protocol cd83430b
* internal/lsp: allow narrower scope for convenience CodeActions 55644ead
* internal/lsp: correctly marshal arguments for upgrade code lens 7b4c4ad3
* internal/lsp: correctly invalidate metadata for batched changes 59c6fc0b
* internal/lsp: support refactor.extract through commands 92670837
* internal/lsp: fix hover link for embedded fields and methods eaaaedc6
* internal/lsp: fix GlobPattern for watching files 102e7d35
* internal/lsp: add parse errors as diagnostics even when parsing fails b5fc9d35
* internal/lsp: fix hover for implicit type switch variable declarations 7017fd6b
* internal/lsp: add errors for missing modules that don't map to an import cbb3c69a
* internal/lsp: move undeclaredname suggested fix out of analysis 37a045f3
* internal/lsp/regtest: fix WithoutWorkspaceFolders option 0e1f6f5c
* internal/lsp/regtest: add a simple stress test 93b64502
* internal/lsp/regtest: add run options to support stress testing 84d0e3d1
* internal/lsp/fake: move to a struct for configuring the sandbox a5e28d8d
* internal/lsp: lift env out of the sandbox into the editor 1ff154e6
* internal/lsp: show errors in indirect dependencies as diagnostics d2c1b4b2
* internal/lsp: treat the module root as the workspace root, if available 0cdb17d1
* internal/lsp/cache: copy *packages.Config environment completely b3d141dd
* internal/lsp: don't publish duplicate diagnostics in one report 80cb7970
* go/packages: add failing test for golang/go#37098 188b3828
* go/packages: find filenames in error strings if not in position a7c6fd06
* internal/lsp: separate test and benchmark codelens bd1e9de8
* go/analysis/passes/unmarshal: Add check for asn1.Unmarshal 70419130
* internal/lsp: support `go mod tidy` on save without diagnostics 6123e778
* internal/lsp: try to parse diagnostics out of `go list` errors b42efcd1
* internal/lsp: handle unknown revision in go.mod file 0a5cd101
* internal/lsp: handle on-disk file deletions for opened files acdb8c15
* internal/lsp: change the way that we pass arguments to command 5ea36318
* internal/lsp: return the actual range from convenience code actions b8e13e1a
myitcv added a commit to govim/govim that referenced this issue Jul 29, 2020
* gopls/integration/govim: enable running at main 1c30660f
* internal/lsp: fix a few staticcheck suggestions, some cleanup 6b78e25f
* internal/lsp: don't show diagnostic in go.mod for direct dependencies 8afe7e08
* internal/lsp: properly check for nil on i.enclosing 449c5851
* internal/lsp/source: move initial extract function logic to lsp/command 1c1ec420
* internal/lsp/source: add extra nil check in searchForEnclosing df70183b
* internal/lsp: fix logged message for gc_details e8769ccb
* internal/lsp: remove uses of crypto/sha1 in gopls 0dd562f3
* internal/lsp: show compiler optimization decisions 95780ea8
* internal/lsp: de-duplicate logic for canExtractVariable edd3c8e9
* internal/lsp: check URIs of all workspace folders 342ee105
* internal/lsp/source: add a unit test for searchForEnclosing e7a7e3a8
* internal/lsp: remove PackageHandle b6476686
* internal/lsp: replace ParseGoHandle with concrete data a9439ae9
* internal/lsp/cache: fix parseKey 4d1d9acc
* internal/lsp: pass snapshot/view to memoize.Functions 72051f79
* internal/lsp/cache: remove files from the memoize.Store 60da08ac
* internal/lsp: minimize PackageHandle interface 051e64e6
* internal/lsp: handle bad formatting with CRLF line endings 2ad651e9
* internal/lsp: support return statements in extract function 3c048e20
* internal/lsp: update code for LSP protocol cd83430b
* internal/lsp: allow narrower scope for convenience CodeActions 55644ead
* internal/lsp: correctly marshal arguments for upgrade code lens 7b4c4ad3
* internal/lsp: correctly invalidate metadata for batched changes 59c6fc0b
* internal/lsp: support refactor.extract through commands 92670837
* internal/lsp: fix hover link for embedded fields and methods eaaaedc6
* internal/lsp: fix GlobPattern for watching files 102e7d35
* internal/lsp: add parse errors as diagnostics even when parsing fails b5fc9d35
* internal/lsp: fix hover for implicit type switch variable declarations 7017fd6b
* internal/lsp: add errors for missing modules that don't map to an import cbb3c69a
* internal/lsp: move undeclaredname suggested fix out of analysis 37a045f3
* internal/lsp/regtest: fix WithoutWorkspaceFolders option 0e1f6f5c
* internal/lsp/regtest: add a simple stress test 93b64502
* internal/lsp/regtest: add run options to support stress testing 84d0e3d1
* internal/lsp/fake: move to a struct for configuring the sandbox a5e28d8d
* internal/lsp: lift env out of the sandbox into the editor 1ff154e6
* internal/lsp: show errors in indirect dependencies as diagnostics d2c1b4b2
* internal/lsp: treat the module root as the workspace root, if available 0cdb17d1
* internal/lsp/cache: copy *packages.Config environment completely b3d141dd
* internal/lsp: don't publish duplicate diagnostics in one report 80cb7970
* go/packages: add failing test for golang/go#37098 188b3828
* go/packages: find filenames in error strings if not in position a7c6fd06
* internal/lsp: separate test and benchmark codelens bd1e9de8
* go/analysis/passes/unmarshal: Add check for asn1.Unmarshal 70419130
* internal/lsp: support `go mod tidy` on save without diagnostics 6123e778
* internal/lsp: try to parse diagnostics out of `go list` errors b42efcd1
* internal/lsp: handle unknown revision in go.mod file 0a5cd101
* internal/lsp: handle on-disk file deletions for opened files acdb8c15
* internal/lsp: change the way that we pass arguments to command 5ea36318
* internal/lsp: return the actual range from convenience code actions b8e13e1a
myitcv added a commit to govim/govim that referenced this issue Jul 29, 2020
* gopls/integration/govim: enable running at main 1c30660f
* internal/lsp: fix a few staticcheck suggestions, some cleanup 6b78e25f
* internal/lsp: don't show diagnostic in go.mod for direct dependencies 8afe7e08
* internal/lsp: properly check for nil on i.enclosing 449c5851
* internal/lsp/source: move initial extract function logic to lsp/command 1c1ec420
* internal/lsp/source: add extra nil check in searchForEnclosing df70183b
* internal/lsp: fix logged message for gc_details e8769ccb
* internal/lsp: remove uses of crypto/sha1 in gopls 0dd562f3
* internal/lsp: show compiler optimization decisions 95780ea8
* internal/lsp: de-duplicate logic for canExtractVariable edd3c8e9
* internal/lsp: check URIs of all workspace folders 342ee105
* internal/lsp/source: add a unit test for searchForEnclosing e7a7e3a8
* internal/lsp: remove PackageHandle b6476686
* internal/lsp: replace ParseGoHandle with concrete data a9439ae9
* internal/lsp/cache: fix parseKey 4d1d9acc
* internal/lsp: pass snapshot/view to memoize.Functions 72051f79
* internal/lsp/cache: remove files from the memoize.Store 60da08ac
* internal/lsp: minimize PackageHandle interface 051e64e6
* internal/lsp: handle bad formatting with CRLF line endings 2ad651e9
* internal/lsp: support return statements in extract function 3c048e20
* internal/lsp: update code for LSP protocol cd83430b
* internal/lsp: allow narrower scope for convenience CodeActions 55644ead
* internal/lsp: correctly marshal arguments for upgrade code lens 7b4c4ad3
* internal/lsp: correctly invalidate metadata for batched changes 59c6fc0b
* internal/lsp: support refactor.extract through commands 92670837
* internal/lsp: fix hover link for embedded fields and methods eaaaedc6
* internal/lsp: fix GlobPattern for watching files 102e7d35
* internal/lsp: add parse errors as diagnostics even when parsing fails b5fc9d35
* internal/lsp: fix hover for implicit type switch variable declarations 7017fd6b
* internal/lsp: add errors for missing modules that don't map to an import cbb3c69a
* internal/lsp: move undeclaredname suggested fix out of analysis 37a045f3
* internal/lsp/regtest: fix WithoutWorkspaceFolders option 0e1f6f5c
* internal/lsp/regtest: add a simple stress test 93b64502
* internal/lsp/regtest: add run options to support stress testing 84d0e3d1
* internal/lsp/fake: move to a struct for configuring the sandbox a5e28d8d
* internal/lsp: lift env out of the sandbox into the editor 1ff154e6
* internal/lsp: show errors in indirect dependencies as diagnostics d2c1b4b2
* internal/lsp: treat the module root as the workspace root, if available 0cdb17d1
* internal/lsp/cache: copy *packages.Config environment completely b3d141dd
* internal/lsp: don't publish duplicate diagnostics in one report 80cb7970
* go/packages: add failing test for golang/go#37098 188b3828
* go/packages: find filenames in error strings if not in position a7c6fd06
* internal/lsp: separate test and benchmark codelens bd1e9de8
* go/analysis/passes/unmarshal: Add check for asn1.Unmarshal 70419130
* internal/lsp: support `go mod tidy` on save without diagnostics 6123e778
* internal/lsp: try to parse diagnostics out of `go list` errors b42efcd1
* internal/lsp: handle unknown revision in go.mod file 0a5cd101
* internal/lsp: handle on-disk file deletions for opened files acdb8c15
* internal/lsp: change the way that we pass arguments to command 5ea36318
* internal/lsp: return the actual range from convenience code actions b8e13e1a
@stamblerre
Copy link
Contributor

@stamblerre stamblerre commented Sep 9, 2020

@bcmills @jayconrod @matloob: Is there a chance of this getting investigated for 1.16? This is the root cause of a gopls bug as well: #37660.

I was able to reproduce using the trivial example from the test in the CL above:

-- a/a.go --
package a
-- a/a.swigcxx --
$ go list -e -json -compiled ./a/...
{
        "Dir": "/Users/rstambler/modules/mod5/a",
        "ImportPath": "mod5.com/a",
        "Name": "a",
        "Root": "/Users/rstambler/modules/mod5",
        "Module": {
                "Path": "mod5.com",
                "Main": true,
                "Dir": "/Users/rstambler/modules/mod5",
                "GoMod": "/Users/rstambler/modules/mod5/go.mod",
                "GoVersion": "1.14"
        },
        "Match": [
                "./a/..."
        ],
        "Stale": true,
        "StaleReason": "stale dependency: runtime/cgo",
        "GoFiles": [
                "a.go"
        ],
        "CompiledGoFiles": [
                "a.go",
                "/Users/rstambler/Library/Caches/go-build/ed/ed57ccdf14d088f69b4137adac071cb62249bd3774af5d883da93359bb7b8287-d",
                "/Users/rstambler/Library/Caches/go-build/3b/3bcc108c50267c602f4bc7ddbd72a6dc9445e33e775225962d6276241ba5996b-d",
                "/Users/rstambler/Library/Caches/go-build/fc/fc14d9c8299495b9365f2ccc6e8977b38a53d8cac1824f8ffe1a1b1fdc8518d7-d",
                "/Users/rstambler/Library/Caches/go-build/bf/bfad65d54097c367b8e59c1820fb6d949ec7321895ea57c2f5748e961325b7c5-d"
        ],
        "SwigCXXFiles": [
                "a.swigcxx"
        ],
        "Imports": [
                "unsafe",
                "runtime/cgo",
                "syscall",
                "sync"
        ],
        "Deps": [
                "errors",
                "internal/bytealg",
                "internal/cpu",
                "internal/oserror",
                "internal/race",
                "internal/reflectlite",
                "internal/unsafeheader",
                "runtime",
                "runtime/cgo",
                "runtime/internal/atomic",
                "runtime/internal/math",
                "runtime/internal/sys",
                "sync",
                "sync/atomic",
                "syscall",
                "unsafe"
        ]
}

Out of these, the last one (/Users/rstambler/Library/Caches/go-build/bf/bfad65d54097c367b8e59c1820fb6d949ec7321895ea57c2f5748e961325b7c5-d) was a C++ file.

@jayconrod jayconrod modified the milestones: Unreleased, Go1.16 Sep 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
7 participants
You can’t perform that action at this time.