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

nogo: instantiate type info for generic types when running under Go >=1.18 #3212

Merged
merged 1 commit into from Jul 19, 2022
Merged

nogo: instantiate type info for generic types when running under Go >=1.18 #3212

merged 1 commit into from Jul 19, 2022

Conversation

farhaven
Copy link
Contributor

@farhaven farhaven commented Jun 22, 2022 •
edited

This mirrors what golang.org/x/tools/go/packages does when loading packages.

Should fix #3211 (and probably #3164 as well).

What type of PR is this?

Bug fix

What does this PR do? Why is it needed?

This PR adds an initialization that is required for the package loader in nogo to load instantiation information for generic types. This takes a lot of inspiration from what the package loader in golang.org/x/tools does.

Which issues(s) does this PR fix?

Fixes #3211 and #3164

Other notes for review

My company signed the CLA, but it looks like that fact has to percolate through something at Google.

sluongng
sluongng reviewed Jun 23, 2022
View reviewed changes
Copy link
Contributor

@sluongng sluongng left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fmeum @achew22 @linzhp I don't think we need to stay compatible with Go 1.17 and below in rules_go? There are some precedent such as #3157 where we shipped features exclusively for 1.18

go/tools/builders/nogo_typeparams_go117.go Show resolved Hide resolved
go/tools/builders/nogo_typeparams_go118.go Show resolved Hide resolved
@farhaven
Copy link
Contributor Author

farhaven commented Jun 23, 2022 •
edited

I've added a test that's failing for two reasons:

  • the Go toolchain version needs to be bumped to >= 1.18. I think there's already a PR for that: Update Go to 1.18.3 #3179. That means that this PR will have to wait until Update Go to 1.18.3 #3179 has been merged. Fine with me :)
  • org_golang_x_tools needs to be updated to something >= v0.1.11, but I don't have (and don't want) the permission to upload packages to mirror.bazel.build. Update Go to 1.18.3 #3179 also seems to indicate that this would depend on a Gazelle release, so this would also be blocked by that.

@uhthomas
Copy link

uhthomas commented Jun 24, 2022 •
edited

Should the version of x/tools also be updated in this PR? Feels like it would be required for generics?

@sluongng sluongng mentioned this pull request Jun 24, 2022
Closed
@sluongng
Copy link
Contributor

created bazelbuild/bazel-gazelle#1283 to request for a newer Gazelle release

@linzhp
Copy link
Contributor

linzhp commented Jun 28, 2022

Gazelle 0.26.0 was released

@shlomi-dr shlomi-dr mentioned this pull request Jun 30, 2022
Closed
@sluongng sluongng mentioned this pull request Jun 30, 2022
Closed
@sluongng
Copy link
Contributor

sluongng commented Jul 7, 2022

d6138e4 was merged.
I suggest rebase this PR on top of latest origin/master so CI can retry.

@sluongng sluongng mentioned this pull request Jul 11, 2022
Merged
@farhaven
Copy link
Contributor Author

d6138e4 was merged. I suggest rebase this PR on top of latest origin/master so CI can retry.

Thanks for the patience, I was on vacation last week and only just got around to rebasing. Let's see how it works.

@farhaven
Copy link
Contributor Author

I've updated golang.org/x/tools used by the tests to v0.1.11, looks like the test runs now.

I used the --mirror=false flag to releaser when doing the update, so even though mirror.bazel.build is mentioned as an additional source for the dependency, that needn't be true. This doesn't hurt building/testing and such right now, but might break if the github mirror goes away.

@farhaven
Copy link
Contributor Author

For some reason, Github won't let me re-request a review, but from my perspective, this looks good to go:

  • golang.org/x/tools have been updated to v0.1.11
  • golang.org/x/mod has been updated to v0.6.2-dev-something (the same version mentioned in tools' go.mod)
  • Building and bazel test //... succeed on CI and (when disabling the CGo tests) on my M1 Mac.

linzhp
linzhp reviewed Jul 17, 2022
View reviewed changes
tests/core/nogo/generics/generics_test.go Outdated Show resolved Hide resolved
tests/core/nogo/generics/generics_test.go Outdated Show resolved Hide resolved
tests/core/nogo/generics/generics_test.go Outdated Show resolved Hide resolved
tests/core/nogo/generics/generics_test.go Outdated Show resolved Hide resolved
tests/core/nogo/generics/generics_test.go Outdated Show resolved Hide resolved
tests/core/nogo/generics/generics_test.go Outdated Show resolved Hide resolved
@farhaven
nogo: instantiate type info for generic types when running under Go >…
848477d 
…=1.18

This mirrors what golang.org/x/tools/go/packages does when loading packages.

Should fix bazelbuild#3211 (and probably bazelbuild#3164 as well).
@farhaven
Copy link
Contributor Author

I've applied the suggestions, rebased the patch on master and squashed the commits down to a single commit to make the history a bit cleaner than "Fix this, fix that, oops, forgot the other thing".

@farhaven farhaven requested a review from linzhp July 18, 2022 09:02
@linzhp linzhp merged commit 4e5aa2f into bazelbuild:master Jul 19, 2022
gcf-merge-on-green bot pushed a commit to googleapis/gapic-generator-go that referenced this pull request Jul 19, 2022
@renovate-bot
chore(deps): update dependency io_bazel_rules_go to v0.34.0 (#1076)
22aecf2 
[![Mend Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)

This PR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
| [io_bazel_rules_go](https://togithub.com/bazelbuild/rules_go) | http_archive | minor | `v0.33.0` -> `v0.34.0` |

---

### Release Notes

<details>
<summary>bazelbuild/rules_go</summary>

### [`v0.34.0`](https://togithub.com/bazelbuild/rules_go/releases/tag/v0.34.0)

[Compare Source](https://togithub.com/bazelbuild/rules_go/compare/v0.33.0...v0.34.0)

#### What's Changed

-   releaser: fix scrubbing timestamp from patch files by [@&#8203;sluongng](https://togithub.com/sluongng) in [bazelbuild/rules_go#3180
-   Replace Starlark JSON parser with json.decode by [@&#8203;fmeum](https://togithub.com/fmeum) in [bazelbuild/rules_go#3184
-   gopackagesdriver: separates "s" files in pkg info by [@&#8203;iamricard](https://togithub.com/iamricard) in [bazelbuild/rules_go#3165
-   Refactor away references to @&#8203;io_bazel_rules_go by [@&#8203;fmeum](https://togithub.com/fmeum) in [bazelbuild/rules_go#3185
-   Do not print to stderr if cgo linking succeeds after retry by [@&#8203;fmeum](https://togithub.com/fmeum) in [bazelbuild/rules_go#3187
-   Use param files with go-protoc by [@&#8203;fmeum](https://togithub.com/fmeum) in [bazelbuild/rules_go#3190
-   Don't include non-executable go_binary in dependent's runfiles by [@&#8203;fmeum](https://togithub.com/fmeum) in [bazelbuild/rules_go#3151
-   Link in native libraries of transitive dependencies in archive mode by [@&#8203;fmeum](https://togithub.com/fmeum) in [bazelbuild/rules_go#3186
-   runfiles: remove deprecated api by [@&#8203;sluongng](https://togithub.com/sluongng) in [bazelbuild/rules_go#3198
-   Fix failing open hermeticity test by [@&#8203;fmeum](https://togithub.com/fmeum) in [bazelbuild/rules_go#3206
-   Fix go_googleapis Gazelle patch by [@&#8203;nickgooding](https://togithub.com/nickgooding) in [bazelbuild/rules_go#3193
-   Exclude unsupported C/C++ features by [@&#8203;fmeum](https://togithub.com/fmeum) in [bazelbuild/rules_go#3189
-   Allow gomock to take Bazel common attributes by [@&#8203;linzhp](https://togithub.com/linzhp) in [bazelbuild/rules_go#3207
-   Transition on edges not self by [@&#8203;illicitonion](https://togithub.com/illicitonion) in [bazelbuild/rules_go#3116
-   Include go_transition_test in bazel aspect by [@&#8203;ian-h-chamberlain](https://togithub.com/ian-h-chamberlain) in [bazelbuild/rules_go#3160
-   Add an example for go_download_sdk.sdks by [@&#8203;fishy](https://togithub.com/fishy) in [bazelbuild/rules_go#3139
-   tests: nogo over generated code by [@&#8203;sluongng](https://togithub.com/sluongng) in [bazelbuild/rules_go#3214
-   test nogo/coverage: test generated code by [@&#8203;sluongng](https://togithub.com/sluongng) in [bazelbuild/rules_go#3213
-   Remove references to go_transition_test by [@&#8203;linzhp](https://togithub.com/linzhp) in [bazelbuild/rules_go#3215
-   Basic bzlmod setup by [@&#8203;fmeum](https://togithub.com/fmeum) in [bazelbuild/rules_go#3047
-   Run BCR tests against Bazel 6.0.0-pre.20220608.2 by [@&#8203;fmeum](https://togithub.com/fmeum) in [bazelbuild/rules_go#3223
-   Use repo-relative labels in MODULE.bazel by [@&#8203;fmeum](https://togithub.com/fmeum) in [bazelbuild/rules_go#3226
-   upkeep: upgrade to go 1.18.3 and gazelle v0.26.0 by [@&#8203;sluongng](https://togithub.com/sluongng) in [bazelbuild/rules_go#3220
-   nogo: ignore generated source files by [@&#8203;sluongng](https://togithub.com/sluongng) in [bazelbuild/rules_go#3216
-   asm: Pass package path with -p by [@&#8203;fmeum](https://togithub.com/fmeum) in [bazelbuild/rules_go#3231
-   bzlmod: Add support for gomock by [@&#8203;fmeum](https://togithub.com/fmeum) in [bazelbuild/rules_go#3232
-   test cgo: ensure helper script works by [@&#8203;sluongng](https://togithub.com/sluongng) in [bazelbuild/rules_go#3236
-   Fix //tests/legacy/examples/cgo:cgo_lib_test on M1 Macs by [@&#8203;fmeum](https://togithub.com/fmeum) in [bazelbuild/rules_go#3237
-   gopackagesdriver: Descend into go_proto_compiler's deps by [@&#8203;fmeum](https://togithub.com/fmeum) in [bazelbuild/rules_go#3240
-   new_library: remove unused resolver by [@&#8203;sluongng](https://togithub.com/sluongng) in [bazelbuild/rules_go#3219
-   nogo: instantiate type info for generic types when running under Go >=1.18 by [@&#8203;farhaven](https://togithub.com/farhaven) in [bazelbuild/rules_go#3212

#### New Contributors

-   [@&#8203;iamricard](https://togithub.com/iamricard) made their first contribution in [bazelbuild/rules_go#3165
-   [@&#8203;ian-h-chamberlain](https://togithub.com/ian-h-chamberlain) made their first contribution in [bazelbuild/rules_go#3160
-   [@&#8203;fishy](https://togithub.com/fishy) made their first contribution in [bazelbuild/rules_go#3139
-   [@&#8203;farhaven](https://togithub.com/farhaven) made their first contribution in [bazelbuild/rules_go#3212

**Full Changelog**: bazelbuild/rules_go@v0.33.0...v0.34.0

#### `WORKSPACE` code

    load("@&#8203;bazel_tools//tools/build_defs/repo:http.bzl", "http_archive")

    http_archive(
        name = "io_bazel_rules_go",
        sha256 = "16e9fca53ed6bd4ff4ad76facc9b7b651a89db1689a2877d6fd7b82aa824e366",
        urls = [
            "https://mirror.bazel.build/github.com/bazelbuild/rules_go/releases/download/v0.34.0/rules_go-v0.34.0.zip",
            "https://github.com/bazelbuild/rules_go/releases/download/v0.34.0/rules_go-v0.34.0.zip",
        ],
    )

    load("@&#8203;io_bazel_rules_go//go:deps.bzl", "go_register_toolchains", "go_rules_dependencies")

    go_rules_dependencies()

    go_register_toolchains(version = "1.18.4")

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

♻ **Rebasing**: Whenever PR is behind base branch, or you tick the rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update again.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, click this checkbox.

---

This PR has been generated by [Mend Renovate](https://www.mend.io/free-developer-tools/renovate/). View repository job log [here](https://app.renovatebot.com/dashboard#github/googleapis/gapic-generator-go).
<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzMi4xMTcuNCIsInVwZGF0ZWRJblZlciI6IjMyLjExNy40In0=-->
gcf-merge-on-green bot pushed a commit to googleapis/gapic-config-validator that referenced this pull request Jul 19, 2022
@renovate-bot
chore(deps): update dependency io_bazel_rules_go to v0.34.0 (#255)
b92b5b7 
[![Mend Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)

This PR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
| [io_bazel_rules_go](https://togithub.com/bazelbuild/rules_go) | http_archive | minor | `v0.33.0` -> `v0.34.0` |

---

### Release Notes

<details>
<summary>bazelbuild/rules_go</summary>

### [`v0.34.0`](https://togithub.com/bazelbuild/rules_go/releases/tag/v0.34.0)

[Compare Source](https://togithub.com/bazelbuild/rules_go/compare/v0.33.0...v0.34.0)

#### What's Changed

-   releaser: fix scrubbing timestamp from patch files by [@&#8203;sluongng](https://togithub.com/sluongng) in [bazelbuild/rules_go#3180
-   Replace Starlark JSON parser with json.decode by [@&#8203;fmeum](https://togithub.com/fmeum) in [bazelbuild/rules_go#3184
-   gopackagesdriver: separates "s" files in pkg info by [@&#8203;iamricard](https://togithub.com/iamricard) in [bazelbuild/rules_go#3165
-   Refactor away references to @&#8203;io_bazel_rules_go by [@&#8203;fmeum](https://togithub.com/fmeum) in [bazelbuild/rules_go#3185
-   Do not print to stderr if cgo linking succeeds after retry by [@&#8203;fmeum](https://togithub.com/fmeum) in [bazelbuild/rules_go#3187
-   Use param files with go-protoc by [@&#8203;fmeum](https://togithub.com/fmeum) in [bazelbuild/rules_go#3190
-   Don't include non-executable go_binary in dependent's runfiles by [@&#8203;fmeum](https://togithub.com/fmeum) in [bazelbuild/rules_go#3151
-   Link in native libraries of transitive dependencies in archive mode by [@&#8203;fmeum](https://togithub.com/fmeum) in [bazelbuild/rules_go#3186
-   runfiles: remove deprecated api by [@&#8203;sluongng](https://togithub.com/sluongng) in [bazelbuild/rules_go#3198
-   Fix failing open hermeticity test by [@&#8203;fmeum](https://togithub.com/fmeum) in [bazelbuild/rules_go#3206
-   Fix go_googleapis Gazelle patch by [@&#8203;nickgooding](https://togithub.com/nickgooding) in [bazelbuild/rules_go#3193
-   Exclude unsupported C/C++ features by [@&#8203;fmeum](https://togithub.com/fmeum) in [bazelbuild/rules_go#3189
-   Allow gomock to take Bazel common attributes by [@&#8203;linzhp](https://togithub.com/linzhp) in [bazelbuild/rules_go#3207
-   Transition on edges not self by [@&#8203;illicitonion](https://togithub.com/illicitonion) in [bazelbuild/rules_go#3116
-   Include go_transition_test in bazel aspect by [@&#8203;ian-h-chamberlain](https://togithub.com/ian-h-chamberlain) in [bazelbuild/rules_go#3160
-   Add an example for go_download_sdk.sdks by [@&#8203;fishy](https://togithub.com/fishy) in [bazelbuild/rules_go#3139
-   tests: nogo over generated code by [@&#8203;sluongng](https://togithub.com/sluongng) in [bazelbuild/rules_go#3214
-   test nogo/coverage: test generated code by [@&#8203;sluongng](https://togithub.com/sluongng) in [bazelbuild/rules_go#3213
-   Remove references to go_transition_test by [@&#8203;linzhp](https://togithub.com/linzhp) in [bazelbuild/rules_go#3215
-   Basic bzlmod setup by [@&#8203;fmeum](https://togithub.com/fmeum) in [bazelbuild/rules_go#3047
-   Run BCR tests against Bazel 6.0.0-pre.20220608.2 by [@&#8203;fmeum](https://togithub.com/fmeum) in [bazelbuild/rules_go#3223
-   Use repo-relative labels in MODULE.bazel by [@&#8203;fmeum](https://togithub.com/fmeum) in [bazelbuild/rules_go#3226
-   upkeep: upgrade to go 1.18.3 and gazelle v0.26.0 by [@&#8203;sluongng](https://togithub.com/sluongng) in [bazelbuild/rules_go#3220
-   nogo: ignore generated source files by [@&#8203;sluongng](https://togithub.com/sluongng) in [bazelbuild/rules_go#3216
-   asm: Pass package path with -p by [@&#8203;fmeum](https://togithub.com/fmeum) in [bazelbuild/rules_go#3231
-   bzlmod: Add support for gomock by [@&#8203;fmeum](https://togithub.com/fmeum) in [bazelbuild/rules_go#3232
-   test cgo: ensure helper script works by [@&#8203;sluongng](https://togithub.com/sluongng) in [bazelbuild/rules_go#3236
-   Fix //tests/legacy/examples/cgo:cgo_lib_test on M1 Macs by [@&#8203;fmeum](https://togithub.com/fmeum) in [bazelbuild/rules_go#3237
-   gopackagesdriver: Descend into go_proto_compiler's deps by [@&#8203;fmeum](https://togithub.com/fmeum) in [bazelbuild/rules_go#3240
-   new_library: remove unused resolver by [@&#8203;sluongng](https://togithub.com/sluongng) in [bazelbuild/rules_go#3219
-   nogo: instantiate type info for generic types when running under Go >=1.18 by [@&#8203;farhaven](https://togithub.com/farhaven) in [bazelbuild/rules_go#3212

#### New Contributors

-   [@&#8203;iamricard](https://togithub.com/iamricard) made their first contribution in [bazelbuild/rules_go#3165
-   [@&#8203;ian-h-chamberlain](https://togithub.com/ian-h-chamberlain) made their first contribution in [bazelbuild/rules_go#3160
-   [@&#8203;fishy](https://togithub.com/fishy) made their first contribution in [bazelbuild/rules_go#3139
-   [@&#8203;farhaven](https://togithub.com/farhaven) made their first contribution in [bazelbuild/rules_go#3212

**Full Changelog**: bazelbuild/rules_go@v0.33.0...v0.34.0

#### `WORKSPACE` code

    load("@&#8203;bazel_tools//tools/build_defs/repo:http.bzl", "http_archive")

    http_archive(
        name = "io_bazel_rules_go",
        sha256 = "16e9fca53ed6bd4ff4ad76facc9b7b651a89db1689a2877d6fd7b82aa824e366",
        urls = [
            "https://mirror.bazel.build/github.com/bazelbuild/rules_go/releases/download/v0.34.0/rules_go-v0.34.0.zip",
            "https://github.com/bazelbuild/rules_go/releases/download/v0.34.0/rules_go-v0.34.0.zip",
        ],
    )

    load("@&#8203;io_bazel_rules_go//go:deps.bzl", "go_register_toolchains", "go_rules_dependencies")

    go_rules_dependencies()

    go_register_toolchains(version = "1.18.4")

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

♻ **Rebasing**: Whenever PR is behind base branch, or you tick the rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update again.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, click this checkbox.

---

This PR has been generated by [Mend Renovate](https://www.mend.io/free-developer-tools/renovate/). View repository job log [here](https://app.renovatebot.com/dashboard#github/googleapis/gapic-config-validator).
<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzMi4xMTcuNCIsInVwZGF0ZWRJblZlciI6IjMyLjExNy40In0=-->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sluongng sluongng sluongng left review comments

@linzhp linzhp linzhp approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

nogo: compilepkg: panic: 0: got 0 arguments but 1 type parameters when checking Go package that uses generics
4 participants
@farhaven @uhthomas @sluongng @linzhp