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

Jump to bottom

go/*,cmd/gofmt: guard support for generic code behind a "typeparams" build constraint #44933

Closed
findleyr opened this issue Mar 11, 2021 · 9 comments
Closed

go/*,cmd/gofmt: guard support for generic code behind a "typeparams" build constraint #44933

findleyr opened this issue Mar 11, 2021 · 9 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone
Go1.17

Comments

@findleyr
Copy link
Contributor

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:

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

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

@ianlancetaylor
Copy link
Contributor

See #42681, which was just accepted.

@mdempsky
Copy link
Member

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

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 build: move GOEXPERIMENT knob from make.bash to cmd/go #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 the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Mar 11, 2021
@findleyr findleyr added this to the Go1.17 milestone Mar 11, 2021
@mdempsky
Copy link
Member

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

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.

gopherbot pushed a commit that referenced this issue Apr 13, 2021
@findleyr
go/*,cmd/gofmt: guard AST changes with the typeparams build tag
efaf75a 
This CL changes our approach to guarding type parameter functionality
and API. Previously, we guarded type parameter functionality with the
parser.parseTypeParams parser mode, and were in the process of hiding
the type parameter API behind the go1.18 build constraint.

These mechanisms had several limitations:
 + Requiring the parser.parseTypeParams mode to be set meant that
   existing tooling would have to opt-in to type parameters in all
   places where it parses Go files.
 + The parseTypeParams mode value had to be copied in several places.
 + go1.18 is not specific to typeparams, making it difficult to set up
   the builders to run typeparams tests.

This CL addresses the above limitations, and completes the task of
hiding the AST API, by switching to a new 'typeparams' build constraint
and adding a new go/internal/typeparams helper package.

The typeparams build constraint is used to conditionally compile the new
AST changes. The typeparams package provides utilities for accessing and
writing the new AST data, so that we don't have to fragment our parser
or type checker logic across build constraints. The typeparams.Enabled
const is used to guard tests that require type parameter support.

The parseTypeParams parser mode is gone, replaced by a new
typeparams.DisableParsing mode with the opposite sense. Now, type
parameters are only parsed if go/parser is compiled with the typeparams
build constraint set AND typeparams.DisableParsing not set. This new
parser mode allows opting out of type parameter parsing for tests.

How exactly to run tests on builders is left to a follow-up CL.

Updates #44933

Change-Id: I3091e42a2e5e2f23e8b2ae584f415a784b9fbd65
Reviewed-on: https://go-review.googlesource.com/c/go/+/300649
Trust: Robert Findley <rfindley@google.com>
Run-TryBot: Robert Findley <rfindley@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Robert Griesemer <gri@golang.org>
@gopherbot
Copy link

Change https://golang.org/cl/310070 mentions this issue: go/ast: fix broken build with typeparams build constraint

@findleyr findleyr changed the title go/*,cmd/gofmt: add a new "typeparams" GOEXPERIMENT value go/*,cmd/gofmt: guard support for generic code behind a "typeparams" build constraint Apr 14, 2021
@gopherbot
Copy link

Change https://golang.org/cl/310071 mentions this issue: cmd/dist: add tests using the typeparams build tag

gopherbot pushed a commit that referenced this issue Apr 14, 2021
@findleyr
go/ast: fix broken build with typeparams build constraint
bcbde83 
My rebase of https://golang.org/cl/300649 before submitting broke the
build (and tests) when using the typeparams build constraint. In a
subsequent CL I add test coverage back to cmd/dist.

This time, I've tested by running:
 - go test -tags=typeparams go/...
 - go test -tags=typeparams cmd/gofmt

All tests pass except for the new TestResolution/typeparams.go2, which I
will fix in a follow-up CL.

For #44933

Change-Id: I439d387841604cf43a90e2ce41dbe6bbbdb0306d
Reviewed-on: https://go-review.googlesource.com/c/go/+/310070
Trust: Robert Findley <rfindley@google.com>
Trust: Robert Griesemer <gri@golang.org>
Run-TryBot: Robert Findley <rfindley@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Robert Griesemer <gri@golang.org>
@gopherbot
Copy link

Change https://golang.org/cl/317029 mentions this issue: go/types: expose more API under -tags=typeparams

gopherbot pushed a commit that referenced this issue May 4, 2021
@mdempsky
go/types: expose more API under -tags=typeparams
cb0658e 
Updates #44933.

Change-Id: I0c4c2a54f67b47771f4fa59f11c47fa7b0dde799
Reviewed-on: https://go-review.googlesource.com/c/go/+/317029
Trust: Matthew Dempsky <mdempsky@google.com>
Trust: Robert Griesemer <gri@golang.org>
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
Reviewed-by: Robert Griesemer <gri@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
@findleyr findleyr mentioned this issue May 6, 2021
Closed
@golang golang locked and limited conversation to collaborators May 4, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Milestone
Go1.17
Development

No branches or pull requests
4 participants
@mdempsky @ianlancetaylor @gopherbot @findleyr