go/types: Export Info.FileVersions for access to file-specific version information

[griesemer]

This proposal has been updated. See this comment for the most recent proposed changes.

---

Background

Tools (such as the compiler) need access to per-file version information as provided in the processed Go source such that they (the tools) can do correct semantic analysis. For instance, this information is needed for correct scoping rules for the loop variable scoping change (#60078). The type checker (go/types) can collect this information during type-checking and provide it to clients.

Proposal

We propose to export the following additional data structures from go/types:

We add a new map to the go/types.Info struct which will be populated if present:

// FileVersions maps a file to the file's Go version.
// If the file doesn't specify a version and Config.GoVersion is not
// given, the reported version is the zero version (Major, Minor = 0, 0).
FileVersions map[*token.File]Version

The Version type will be defined as follows:

// A Version represents a released Go version.
type Version struct {
	Major int
	Minor int
}

The Version type may also define/export the following methods for convenience:

// String returns a string representation of v in the form "goMajor.Minor".
func (v Version) String() string

// Equal reports whether v and u are the same version.
func (v Version) Equal(u Version) bool

// Before reports whether version v is before version u.
func (v Version) Before(u Version) bool

// After reports whether version v is after version u.
func (v Version) After(u Version) bool

These methods are not necessary (they are trivial to implement if needed), but unexported versions of them exist in the type checker and there are no strong reasons to withhold them from being exported.

Implementation

This proposal has been implemented in types2 for use by the compiler and the code is present in unexported form in go/types, see CL 515135. If there are no objections to the proposed API we can simply export the functionality. If there are viable alternative proposals, we can adjust the existing implementation as needed.

[griesemer]

cc: @findleyr

[mvdan]

I seem to understand that this information is already available via the GoVersion field in https://pkg.go.dev/go/ast#File, just in string form.

Is this new API about parsing the version so that it can be used and compared more easily? That would remind me of #62039, so I wonder if we could deduplicate.

Or is it about accessing the version information when one loads go/types without go/ast, such as using https://pkg.go.dev/golang.org/x/tools/go/packages with NeedTypesInfo but without NeedSyntax?

[griesemer]

@mvdan You are correct that clients can find out about the respective Go version already, but that would involve duplicating parsing the string and various decisions. For instance, the File.GoVersion field will be empty ("") if the file doesn't contain the respective //go:build line and then the version comes from the go.mod file, if one is specified, etc.

The type checker needs to make all these decisions already internally. By exporting the data via FileVersions, clients can get the same information consistently, w/o having to redo the work again.

[mvdan]

Ah, so go/types would "flatten" this information beyond just parsing it out of the go/ast string - that makes sense then.

It might still make sense to reuse the logic from #62039 if possible, or somehow merge the proposals. That API is still in flux, but it would be a bit weird for the x/mod/gover API to be entirely separate or wildly different than that of go/types.Info.FileVersions, since they ultimately talk about the same kind of versions.

[timothy-king]

Maybe the Version should be a string to match ast.File.GoVersion?

[griesemer]

@timothy-king A string doesn't lend itself well to before/after comparisons.

[findleyr]

I think the key of the map should be *ast.File, for consistency with other maps in types.Info.

I think there's meandering history that led us to token.Pos (it was originally *token.File), but reconsidering it now, *ast.File makes the most sense.

[rsc]

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]

We should probably do something to fit better with #62039. Maybe that proposal should become a new go/version package, and then this package can just use a plain string and direct people to the go/version package for operations.

If we do that, the only addition in this proposal is the FileVersions map added to Info, of type map[*File]string.

[gopherbot]

Change https://go.dev/cl/532580 mentions this issue: go/ssa: new range var semantics

[gopherbot]

Change https://go.dev/cl/533056 mentions this issue: internal/versions: add a new versions package

[griesemer]

Updated proposal, per internal discussion and comments here and here.

We propose to export the following additional map from go/types which will be populated if present:

// FileVersions maps a file to the file's Go version string.
// If the file doesn't specify a version and Config.GoVersion is not
// given, the reported version is the empty string.
FileVersions map[*ast.File]string

The version string can be compared against with functionality as suggested here.

Open question: Should we only have a map entry for files that actually specify a version? That way we don't have to decide what to report if neither the file nor the module specify a version.

[gopherbot]

Change https://go.dev/cl/533975 mentions this issue: all: apply versions.InitFileVersions in x/tools.

[gopherbot]

Change https://go.dev/cl/534018 mentions this issue: go/types: update FileVersions API to match proposal changes

[findleyr]

Open question: Should we only have a map entry for files that actually specify a version?

By analogy, didn't we regret not having declaring identifiers in types.Info.Uses? (not a rhetorical question -- I can't recall the history here). Another analogy: we record the type of Idents even though it could be looked up in the Uses map.

I don't have a strong opinion either way, but it seems simpler to populate every file in the FileVersions map if it exists -- the redundant allocations are hardly a concern, and it saves callers from having to write a VersionOf helper.

[griesemer]

@findleyr Agreed. Removed the open question.

[mdempsky]

I think the key of the map should be *ast.File, for consistency with other maps in types.Info.

FWIW, I think *token.File would be easier for users in practice than *ast.File.

With the go/* APIs, you almost always need the *token.FileSet handy anyway, so given any token.Pos, you can readily lookup the *token.File. And token.Pos is available through the go/types public APIs, as well as at any ast.Node within a file (including *ast.File itself).

By comparison, nothing points back up to the *ast.File, and that's not information users currently need to explicitly track for any other parts of the API. So users may need to restructure their code more to ensure its availability. (E.g., discussing with @griesemer right now, we're leaning towards leaving types2 keyed by *syntax.PosBase instead of changing it to *syntax.File, because the former aligns more naturally with how the unified IR writer is currently structured.)

[findleyr]

@mdempsky *token.File is problematic for a couple reasons:

1. It pins the *token.File in memory, which is not done anywhere else.
2. *token.File is not canonical. A separate *token.File may be created that describes the same file, via the SetLines method.
3. Line directives make things confusing.

This is why we originally switched to token.Pos, which avoids problems 1-3. But *ast.File is more symmetrical with the rest of the types.Info API, and in practice one usually has the *ast.File around when inspecting a node.

CC @adonovan, with whom I was discussing this earlier, and who agreed at the time that *ast.File was more natural. Perhaps we should all talk together :)

[adonovan]

I'm not worried about increased liveness, because in practice one nearly always wants the token, ast, and types information for a complete package until all of them become garbage together.

@mdempsky makes a good point that it's easier to find the token.File from an arbitrary node. I was concerned earlier that line directives make things confusing, but they affect only the apparent filename and line, not the token.File instance, associated with the ast.File. So I'm ok with using token.File.

[findleyr]

I agree it may be more convenient, but it still feels off to me, since I think of go/types as deriving from the abstract syntax (which holds *ast.File.GoVersion), and not the concrete position. Furthermore, it feels incorrect to use *token.File as a key when we have been trying to avoid treating *token.File as canonical in gopls (though of course, within the context of a given FileSet the File is unambiguous).

The one exception to my mental model is the fact that types.Scope.LookupParent uses a token.Pos. I recall @griesemer having second thoughts about this API, and I wonder if those thoughts can help inform this decision.

I am also not entirely convinced by the convenience argument. I think that one often already has the *ast.File available, in which case looking up via *token.File is an unnecessary indirection. If one doesn't have the *ast.File available, then one could easily build a helper to look it up, just as one would do in order to collect other contextual information (such as astutil.PathEnclosingInterval).

No problem if I'm overruled.

[timothy-king]

After prototyping using goversions for x/tools/go/ssa, I found that *ast.File was pretty direct. I needed to record a bit of extra info for types.Initializers, but it was not a big deal. My intuition is that *token.File would have been more work.

I agree that it seems odd to get information from *ast.File.GoVersion and then use *token.File which is unaware of GoVersions and may not be a go file.

[adonovan]

Russ suggested that we keep the representation opaque and provide an accessor function, allowing the representation to evolve as needed:

func (*Info) FileVersion(token.Pos) string

That's not consistent with the other fields of Info which are public, but in hindsight I do wish we had encapsulated them as well, so perhaps it's not too late to start now.

It does leave the question of how the user initializes the Info to indicate that they want file version info, but perhaps this particular mapping is so tiny that it's fine to gather it unconditionally.

[griesemer]

After in-person discussions with @adonovan and @findleyr, we concluded that the API change should be essentially as proposed a few days ago (comment):

// FileVersions maps a file to the file's Go version string.
// If the file doesn't specify a version the reported string
// is Config.GoVersion.
FileVersions map[*ast.File]string

The version string can be compared against with functionality as suggested in #62039 (comment).

Discussion:
FileVersions will have an entry for each file, consistent with all the other maps, which have an entry for each object they track. This will make it unnecessary for clients to consult Config.GoVersion because all entries are present.
Using an *ast.File as key may not be the most convenient for clients, but it is usually not difficult to track the relevant file in a side table/map where needed. Using an *ast.File (rather than a token.Pos) is consistent with the rest of the go/type.Info API.
More importantly, except for some Scope functionality (where it was likely a mistake), go/types does not rely on position information for type checking. This makes it possible to type-check non-parser constructed ASTs that may have incomplete or missing position information. We do not want to sacrifice this property.
By tracking the version information per file (which is where it may be specified) we also don't need to worry about constant additions to the meaning of the versions map, as would be the case if we adopted ast.Node as key (comment).

[cuonglm]

Note that the existing implementation in types2 always set info.FileVersions, even there's no //go:build line:

go/src/cmd/compile/internal/types2/check.go

Line 289 in aa05674

check.recordFileVersion(fbase, check.version) // record package version (possibly zero version)

[rsc]

Have all remaining concerns about this proposal been addressed?

A new map will be added to types.Info, populated when non-nil like all the other fields in Info:

// FileVersions maps a file to the file's Go version string.
// If the file doesn't specify a version the reported string
// is Config.GoVersion.
// Version strings begin with “go”, like “go1.21”,
// and are suitable for use with the [go/version] package.
FileVersions map[*ast.File]string

[gopherbot]

Change https://go.dev/cl/539016 mentions this issue: go/analysis/passes/loopclosure: disable checker after go1.22.

[rsc]

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

A new map will be added to types.Info, populated when non-nil like all the other fields in Info:

// FileVersions maps a file to the file's Go version string.
// If the file doesn't specify a version the reported string
// is Config.GoVersion.
// Version strings begin with “go”, like “go1.21”,
// and are suitable for use with the [go/version] package.
FileVersions map[*ast.File]string

[gopherbot]

Change https://go.dev/cl/540056 mentions this issue: go/types: export Info.FileVersions

[gopherbot]

Change https://go.dev/cl/541296 mentions this issue: cmd/compile: update types2.Info.FileVersions API to match go/types

[rsc]

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

A new map will be added to types.Info, populated when non-nil like all the other fields in Info:

// FileVersions maps a file to the file's Go version string.
// If the file doesn't specify a version the reported string
// is Config.GoVersion.
// Version strings begin with “go”, like “go1.21”,
// and are suitable for use with the [go/version] package.
FileVersions map[*ast.File]string

[gopherbot]

Change https://go.dev/cl/541736 mentions this issue: go/types, types2: add FileVersions map to test Info

[cherrymui]

A number of CLs are already landed, including the new API. Is there anything else needed to be done for this? Or we can close this issue? Thanks.

[gopherbot]

Change https://go.dev/cl/546836 mentions this issue: doc: add release note for go/types.Info.FileVersions