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/types: add a Config.GoVersion field to set the target language version #46648

Closed
findleyr opened this issue Jun 8, 2021 · 10 comments
Closed

go/types: add a Config.GoVersion field to set the target language version #46648

findleyr opened this issue Jun 8, 2021 · 10 comments

Comments

@findleyr
Copy link
Contributor

@findleyr findleyr commented Jun 8, 2021

With the compiler type checker being rewritten based on go/types in cmd/compile/internal/types2, certain compiler features have been ported back to go/types. One such feature is the types.Config.goVersion field, which configures the target Go version being type checked, necessary for the -lang compiler flag. This issue proposes to export that field in go/types as types.Config.GoVersion. I think this could be a valuable change, but am not 100% confident that we should make it, so the purpose of this proposal is to hopefully start a discussion and get feedback.

Specifically, the GoVersion field could be set to a valid Go version string (e.g. "go1.N") to instruct the type checker to disallow language features that were added after Go 1.N. Note that this wouldn't prevent importing standard library APIs that were added after Go 1.N, with the exception of unsafe -- that would be the domain of the importer.

This may be particularly relevant with the large number of language changes landing for generics. Generics both introduce a large change to the type checker, and incentivize tools to break compatibility with older Go versions so that they can leverage generics in their own code.

In general there are always users whose deployment targets are bound to older Go versions, for a variety of reasons. It would be good if their tooling were not bound by the same constraint. Adding the types.Config.GoVersion field seems like a necessary (though not always sufficient) mechanism to avoid such coupling.

The only downside that I can see to this new API is that go/types will have to continue to support older versions of the language. But this is essentially already the case since we want go/types and cmd/compile/internal/types2 to stay in sync (and maybe even converge in some areas). New language features are by definition additive, so maintaining support for older versions Go is usually as simple as a version check and error message.

But this is not a change that should be made casually, as it signals a new model which, if endorsed, should be supported by other go/* packages. If there is consensus that this is a good direction to head, I will open a similar issue for go/parser, but I am holding off on this so as not to fragment discussion about the overall concept. I'm curious what other members of the tools community think: is this a universally good change? How valuable would it be?

CC @dominikh @mvdan: I know staticcheck and gofumpt already have a concept of target version, so perhaps you have an opinion on this change.

CC @griesemer @mdempsky

@dominikh
Copy link
Member

@dominikh dominikh commented Jun 9, 2021

This may be particularly relevant with the large number of language changes landing for generics. Generics both introduce a large change to the type checker, and incentivize tools to break compatibility with older Go versions so that they can leverage generics in their own code.

I don't understand how Config.GoVersion would change the impact of tools using generics in their code.

In general there are always users whose deployment targets are bound to older Go versions, for a variety of reasons. It would be good if their tooling were not bound by the same constraint. Adding the types.Config.GoVersion field seems like a necessary (though not always sufficient) mechanism to avoid such coupling.

It is already true that go/types can process older versions of Go code, as no backwards incompatible changes are made to the language. I don't see why Config.GoVersion would be needed to support the use case of a tool being compiled with a newer version of Go than the code that the tool is operating on. I don't think anyone relies on their tools to tell them that their code is using too new a feature; people rely on go build for that.

Loading

@findleyr
Copy link
Contributor Author

@findleyr findleyr commented Jun 9, 2021

@dominikh thanks, responses below.

I don't understand how Config.GoVersion would change the impact of tools using generics in their code.

This may have been overly speculative, but I was imagining a scenario (say the Go 1.19 release) where generics have been available for two major versions (I hope!), and staticcheck / gofumpt want to start using them in their code base. But this breaks the gopls build, and the gopls team still wants to support developing for Go 1.15 because it's still supported on AppEngine (disclaimer: I have no special knowledge of whether this scenario could happen). In that case we'd have to pin older gofumpt / staticcheck versions in order to support building at 1.15. And to be clear, we'd need to build with 1.15 because we want type T interface { int } to be a type checker error.

It is already true that go/types can process older versions of Go code, as no backwards incompatible changes are made to the language.

This is only true for valid Go code, which may be fine for staticcheck but is not fine for gopls.

I don't think anyone relies on their tools to tell them that their code is using too new a feature; people rely on go build for that.

I disagree with this, at least as I interpreted it. I very much rely on my editor to tell me this, by way of type checker errors.

However, as you point out Config.GoVersion only matters for tools that need to work with invalid Go code. At this point, I suspect there are few such tools other than gopls. It would be great if Gopls could support a target Go version, but for that types.Config.GoVersion is only one part of the puzzle.

Loading

@rsc
Copy link
Contributor

@rsc rsc commented Jun 9, 2021

We envision in the future being able to do things like remove string(1) = "\x01" based on Go version.
We also disallow newer things in older Go versions, like 0b010101 is not available if go.mod says an old version.

Loading

@rsc
Copy link
Contributor

@rsc rsc commented Jun 9, 2021

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

Loading

@rsc rsc moved this from Incoming to Active in Proposals Jun 9, 2021
@mvdan
Copy link
Member

@mvdan mvdan commented Jun 14, 2021

I think this makes sense. From the tool author's perspective it would perhaps be unfortunate to have to plumb a "go version" through to many libraries like go/parser, go/types, or go/packages, but I think it's the most reasonable path forward right now.

The main reason that gofumpt has the "go version" flag is to not do formatting changes that would be too early for a module. For example, if a module declares go 1.13 or later and gofumpt encounters 0666, it will turn that into 0o666, but not if the Go version is lower. So I'm pretty sure the gofumpt command-line tool would use the same Go version for APIs like go/parser, and that would be a good thing. Right now, like you say, it simply follows the Go version that the tool was built with, which for some people can be too new, forcing them to consider pinning older versions.

One interesting question is if this kind of flag would apply to go/printer and go/format as well, particularly in how major versions of Go sometimes change what the "canonical gofmt format" is. For example, a recent version of Go changed the heuristic for breaking the vertical alignment of inline Go comments, meaning that the older and newer versions of gofmt (and any tools using go/printer!) would now be incompatible with each other's output.

Another point to bring up is that the name "target" might be a bit confusing to end users. If I'm maintaining a Go module that supports Go 1.16.x and 1.15.x at the same time, I want to set go 1.15 in my go.mod, not go 1.16. Similarly, my Config.GoVersion should be 1.15. I think most people would call 1.16 the "target" version and 1.15 the "minimum" version, akin to Java. We've talked about having a good name for that version in go.mod with @bcmills @jayconrod @myitcv @rogpeppe before, but right now I don't recall if that ended up anywhere on GitHub.

Loading

@rsc
Copy link
Contributor

@rsc rsc commented Jun 16, 2021

Based on the discussion above, this proposal seems like a likely accept.
— rsc for the proposal review group

Loading

@rsc rsc moved this from Active to Likely Accept in Proposals Jun 16, 2021
@findleyr
Copy link
Contributor Author

@findleyr findleyr commented Jun 16, 2021

We discussed this in the external tools call today. Some notes and follow-up thoughts:

Regarding @mvdan's points in #46648 (comment):

  • We do need a better name for the version in the go.mod, but for the purpose of go/types (and go/parser), I think GoVersion is unambiguous: any language features added after this version are rejected. Perhaps we could refer to this as a "compatibility version".
  • This version therefore controls whether code is accepted by go/parser and go/types. I don't think it's necessary to have a configured version for go/printer and go/format. If GoVersion is added to go/parser and go/types, it will allow developers to use tools built at recent go versions, and therefore have the latest behavior of go/printer. As Dan pointed out in the call, it could get quite complex to support multiple versions of formatting algorithms.
    This obviously wouldn't work if go/format were to reformat 0666 ans 0o666, but AFAIK go/format doesn't make this type of change (and shouldn't). Dan points out that go fmt adds //go:build directives, but that is at least backward-compatible.

Regarding generics, @marwan-at-work pointed out that with the proposed go/ast changes to support generics (WIP draft), it would be good to have a mechanism that prevents go/parser from producing any new AST node types. This could be used to protect tools from panicking in the case of non-exhaustive type switches, until they are updated to support generics. A GoVersion configuration option for go/parser could serve as this mechanism.

Loading

@rsc
Copy link
Contributor

@rsc rsc commented Jul 14, 2021

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

Loading

@rsc rsc changed the title proposal: go/types: add a Config.GoVersion field to set the target language version go/types: add a Config.GoVersion field to set the target language version Jul 14, 2021
@rsc rsc removed this from the Proposal milestone Jul 14, 2021
@rsc rsc added this to the Backlog milestone Jul 14, 2021
@gopherbot
Copy link

@gopherbot gopherbot commented Jul 14, 2021

Change https://golang.org/cl/334533 mentions this issue: [dev.typeparams] go/types: export the Config.GoVersion field

Loading

gopherbot pushed a commit that referenced this issue Jul 14, 2021
Export the types.Config.GoVersion field, so that users can specify a
language compatibility version for go/types to enforce.

Updates #46648

Change-Id: I9e00122925faf0006cfb08c3f2d022619d5d54d5
Reviewed-on: https://go-review.googlesource.com/c/go/+/334533
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>
@findleyr
Copy link
Contributor Author

@findleyr findleyr commented Jul 14, 2021

Thanks for accepting! This change has been merged to the dev.typeparams branch, which will be merged back to master once the go1.18 development cycle begins. I'm going to close this issue now as I don't see a reason to leave it open.

Loading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Proposals
Accepted
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants