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

go/*,cmd/gofmt: add a new "typeparams" GOEXPERIMENT value #44933

Open
findleyr opened this issue Mar 11, 2021 · 6 comments
Open

go/*,cmd/gofmt: add a new "typeparams" GOEXPERIMENT value #44933

findleyr opened this issue Mar 11, 2021 · 6 comments
Milestone

Comments

@findleyr
Copy link
Contributor

@findleyr findleyr commented Mar 11, 2021

Our initial approach to hiding the type parameter API in go/* libraries merged from dev.typeparams was to guard things behind the go1.18 build constraint. Unfortunately, this makes it difficult to build and test a toolchain that supports type parameters.

As suggested by @bcmills in chat, using GOEXPERIMENT=typeparams satisfies our requirements:

  • As of https://golang.org/cl/222925, it automatically sets the goexperiment.typeparams build constraint.
  • It's straightforward to set up a builder to run with an experiment (as in #37937).

I've integrated GOEXPERIMENT=typeparams in https://golang.org/cl/300649, however I'm not sure if I'm holding it right. The location of the GOEXPERIMENT definition (in cmd/internal/objabi) as well as its historical use does not suggest that it's a generally available knob. It's also a bit confusing to have the GOEXPERIMENT=typeparams used for go/* libraries and -G=N used for the compiler, especially since GOEXPERIMENT has historically been used for the compiler.

All we really need is a build constraint and a builder that runs tests with that build constraint. We could probably do this without a GOEXPERIMENT value, but it seems silly to invent a new mechanism when GOEXPERIMENT exists.

CC @griesemer @mdempsky @dmitshur for discussion. Any opinions on using GOEXPERIMENT? Any other ideas?

@gopherbot
Copy link

@gopherbot gopherbot commented Mar 11, 2021

Change https://golang.org/cl/300649 mentions this issue: go/*,cmd/gofmt: add a new "typeparams" GOEXPERIMENT

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Mar 11, 2021

See #42681, which was just accepted.

@mdempsky
Copy link
Member

@mdempsky mdempsky commented Mar 11, 2021

For purely standard library features, we've usually just given them new build tags; e.g., "faketime", "nethttpomithttp2", "x509omitbundledroots", "osusergo", "netgo", "math_big_pure_go", "compiler_bootstrap", "libfuzzer", ... I think it would be fine to just declare another one for hiding go/*'s typeparams support.

As you point out, GOEXPERIMENT currently is more of a toolchain knob to control what experimental compiler/runtime features should be used for compiling the target program. It would be a new use of it to instead control what experimental standard library features are available. However, I'm not opposed to doing that if it plays more nicely with the build infrastructure or something.

As for how typeparams support is enabled in cmd/compile, I'd be fine with changing that to use GOEXPERIMENT too. Though whether a Go program itself uses type params and whether it wants to use go/* to process other Go code that use type params are somewhat two orthogonal issues. Again, I don't have any preference on whether these are controlled by a single knob or independently with two different knobs.

@findleyr
Copy link
Contributor Author

@findleyr findleyr commented Mar 11, 2021

I'd be fine with simply using a build tag, but do think it's important to have a builder running tests with typeparams enabled. Due to how integrated type parameters are with the type checker, it's possible for seemingly unrelated changes to affect type parameter checking.

Looking through x/build, I don't think it would be easy to run tests with e.g. -tags=typeparams. It looks like we'd have to do some non-trivial plumbing through cmd/dist and x/build. For example, I tried running go install -tags=goexperiment.typeparams std before running go tool dist test "go_test:go/types", and dist rebuilds without the tag before testing.

So my questions are:

  • Is my assessment of the effort required in x/build to test with an arbitrary build tag correct? (this is mostly addressed to the release team)
  • Does using GOEXPERIMENT for non compiler/runtime related experiments set a dangerous precedent?
  • Should we "officially" expand the scope of GOEXPERIMENT to include standard library experiments, now that #42681 is accepted?

My current feeling is that the quick-and-dirty solution of defining a fake experiment requires approximately no work, and is probably justified for type parameter library changes due to the nature and scope of work on generics. I'm concerned that the use of GOEXPERIMENT will be confusing, but hopefully we can mitigate that confusion with commentary/documentation. Changing cmd/compile to also use GOEXPERIMENT would reduce confusion, but IMO is not strictly necessary.

CC @golang/release

@findleyr findleyr added this to the Go1.17 milestone Mar 11, 2021
@mdempsky
Copy link
Member

@mdempsky mdempsky commented Mar 11, 2021

I agree there should be builder test coverage. But can't you just add another stanza like this in cmd/dist/test.go?

go/src/cmd/dist/test.go

Lines 468 to 475 in 0a65559

t.tests = append(t.tests, distTest{
name: "osusergo",
heading: "os/user with tag osusergo",
fn: func(dt *distTest) error {
t.addCmd(dt, "src", t.goTest(), t.timeout(300), "-tags=osusergo", "os/user")
return nil
},
})

@findleyr
Copy link
Contributor Author

@findleyr findleyr commented Mar 11, 2021

Thanks, that's a good idea -- I'm not that familiar with cmd/dist. I was thinking about this in terms of adding a single linux-amd64-typeparams builder, but this might be better.

Well, for now I'm going to update my CL to just use a unique build constraint. We can independently solve the problem of how to test it.

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
4 participants