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/compile: add new range behind GOEXPERIMENT=range #61717

Closed
rsc opened this issue Aug 2, 2023 · 14 comments
Closed

cmd/compile: add new range behind GOEXPERIMENT=range #61717

rsc opened this issue Aug 2, 2023 · 14 comments
Assignees
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. Proposal Proposal-Accepted
Milestone

Comments

@rsc
Copy link
Contributor

rsc commented Aug 2, 2023

It would be good to get code for #61405 into the tree for easier use.
I propose we check in the current prototype behind GOEXPERIMENT=range.

@rsc rsc added this to Proposals Aug 2, 2023
@rsc rsc moved this to Incoming in Proposals Aug 2, 2023
@gopherbot gopherbot added this to the Proposal milestone Aug 2, 2023
@rsc
Copy link
Contributor Author

rsc commented Aug 2, 2023

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

@rsc rsc moved this from Incoming to Active in Proposals Aug 2, 2023
@earthboundkid
Copy link
Contributor

Would it make sense to have a experimental coro package users could try too?

@rsc
Copy link
Contributor Author

rsc commented Aug 9, 2023

No change in consensus, so accepted. 🎉
This issue now tracks the work of implementing the proposal.
— rsc for the proposal review group

@rsc rsc moved this from Active to Accepted in Proposals Aug 9, 2023
@rsc rsc changed the title proposal: cmd/compile: add new range behind GOEXPERIMENT=range cmd/compile: add new range behind GOEXPERIMENT=range Aug 9, 2023
@rsc rsc modified the milestones: Proposal, Backlog Aug 9, 2023
@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Aug 9, 2023
@aclements
Copy link
Member

Would it make sense to have a experimental coro package users could try too?

@carlmjohnson , I think it's pretty unclear what we want to do with coro. Note that @rsc's new proposal for the iter package (#61897) includes iter.Pull, which will probably use the coro techniques under the hood, though coro may not become a package in its own right.

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/518376 mentions this issue: go/ssa/interp: disable test for GOROOT/test/range.go

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/510541 mentions this issue: cmd/compile: implement range over func

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/510537 mentions this issue: cmd/compile, go/types: typechecking of range over int, func

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/510538 mentions this issue: cmd/compile: implement range over integer

@meling
Copy link

meling commented Aug 29, 2023

Will these changes make its way into gopls in some way, so that the "experiment" can be used from IDEs without compile errors?

@DeedleFake
Copy link

DeedleFake commented Aug 30, 2023

Is this version of the implementation going to have unused bool returns from the outer function removed?

Also, I wanted to just turn this on via gotip env -w GOEXPERIMENT=range so that it would be easier to play around with, but doing so breaks my usual Go installation. Every command just gives an unknown experiment error and fails. I tried setting it as an environment variable when running gotip download 510541, but that just failed with a claim that there was no valid Go toolchain available to build with. Is this expected behavior? That latter one in particular feels like a bug to me.

Edit: I tried building the toolchain with the environment variable already set again in Linux this time and it worked this time, except that the defaults are still what they already were.

@meling
Copy link

meling commented Aug 30, 2023

I get this:

% GOEXPERIMENT=range gotip version
go: unknown GOEXPERIMENT range
% gotip version
go version devel go1.22-51ed3cb702 Wed Aug 30 21:18:04 2023 +0000 darwin/arm64
% GOEXPERIMENT=range gotip download
Updating the go development tree...
From https://go.googlesource.com/go
 * branch                  master     -> FETCH_HEAD
HEAD is now at 51ed3cb702 net: retry TestDialTimeout subtests with progressively shorter timeouts
Building Go cmd/dist using /usr/local/go. (go1.21.0 darwin/arm64)
Building Go toolchain1 using /usr/local/go.
Building Go bootstrap cmd/go (go_bootstrap) using Go toolchain1.
Building Go toolchain2 using go_bootstrap and Go toolchain1.
go_bootstrap: unknown GOEXPERIMENT range
go tool dist: FAILED: /Users/meling/sdk/gotip/pkg/tool/darwin_arm64/go_bootstrap install -pgo=off cmd/asm cmd/cgo cmd/compile cmd/link: exit status 2
gotip: failed to build go: exit status 2

Anyone got this working on macOS?

@meling
Copy link

meling commented Aug 30, 2023

Ok. I had to download gotip again:

% go install golang.org/dl/gotip@latest

Now I can compile with GOEXPERIMENT=range.

@rsc
Copy link
Contributor Author

rsc commented Aug 31, 2023

It's not all checked in yet. When it is, I'll close this issue.

gopherbot pushed a commit that referenced this issue Sep 20, 2023
Add type-checking logic for range over integers and functions,
behind GOEXPERIMENT=range.

For proposal #61405 (but behind a GOEXPERIMENT).
For #61717.

Change-Id: Ibf78cf381798b450dbe05eb922df82af2b009403
Reviewed-on: https://go-review.googlesource.com/c/go/+/510537
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Auto-Submit: Russ Cox <rsc@golang.org>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
Reviewed-by: Robert Findley <rfindley@google.com>
gopherbot pushed a commit that referenced this issue Sep 20, 2023
Add compiler implementation of range over integers.
This is only reachable if GOEXPERIMENT=range is set,
because otherwise type checking will fail.

For proposal #61405 (but behind a GOEXPERIMENT).
For #61717.

Change-Id: I4e35a73c5df1ac57f61ffb54033a433967e5be51
Reviewed-on: https://go-review.googlesource.com/c/go/+/510538
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
Auto-Submit: Russ Cox <rsc@golang.org>
gopherbot pushed a commit that referenced this issue Sep 20, 2023
Add compiler support for range over functions.
See the large comment at the top of
cmd/compile/internal/rangefunc/rewrite.go for details.

This is only reachable if GOEXPERIMENT=range is set,
because otherwise type checking will fail.

For proposal #61405 (but behind a GOEXPERIMENT).
For #61717.

Change-Id: I05717f94e63089c503acc49b28b47edeb4e011b4
Reviewed-on: https://go-review.googlesource.com/c/go/+/510541
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
Auto-Submit: Russ Cox <rsc@golang.org>
@aclements
Copy link
Member

This is all checked in now (and has been for a while). Closing.

@github-project-automation github-project-automation bot moved this from In Progress to Done in Go Compiler / Runtime Jan 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. Proposal Proposal-Accepted
Projects
Status: Accepted
Development

No branches or pull requests

6 participants