cc: @findleyr

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?

@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.

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.

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

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

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.

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

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.

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

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

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.

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

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

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.

@findleyr Agreed. Removed the open question.

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.)

@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 :)

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.

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.

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.

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.

If we do this, then why not move this information into the *Package object:

// VersionAt returns the Go version for the file containing the given position.
// If the respective file doesn't specify a version, the result is the same as
// pkg.GoVersion() (which may be the empty string).
// The position pos must be based on the same *token.FileSet as was provided
// to Config.Check or NewChecker, respectively, when type-checking this package.
func (pkg *Package) VersionAt(pos token.Pos) string

Note that we already have Package.GoVersion which provides the minimum package version. Putting both together makes sense.

I agree that the data collected is minimal (and we have to collect it always, internally, anyway).

Ok, Package.VersionAt feels correct to me. EDIT: I think the argument against storing this on the Package makes sense.

Err, actually this is making me think twice, and noticing a aspect of this change that I hadn't fully considered until now: previously, the semantics of type checking was mostly unaffected by the positions contained in the AST. IIRC the provided FileSet is only used for formatting error messages, and the only semantic significance of positions is for types.Scope.LookupParent. Otherwise, the syntax is treated abstractly.

As we work on refactoring, we're increasingly wanting to do things like splice in new trees and re-type check. Ideas have been bouncing around for how to make type checking faster and more composeable. Are we sure we want to introduce another semantic dependency on token.Pos? Perhaps the type checker itself should stop looking up versions by position, and instead look them up based on the current *ast.File.

Also, if we can't decide, we could instead expose func (pkg *Package) VersionOf(file *ast.File) string... (half serious proposal).

// Currently the following nodes may have entries: *ast.ForStmt, *ast.RangeStmt.
VersionAt map[ast.Node]string

FWIW the x/tools/go/ssa prototype effectively built a map[ast.Node]string map, but the keys were *ast.FuncDecl and the nodes for possible values of go/types.Info.InitOrder[i].Rhs (IIUC these are the ast.GenDecl.Specs[i].Values[j] expressions on the *ast.Files). For the moment, these seem to suffice as 'root's for deciding where goversion may change. While this seems sufficient at the moment, this may not be a guarantee going forward.

I think here's a use case to judge candidate solutions by: how do you determine which language version to use for each initialization assignment in Info.InitOrder? In particular, if a FuncLit appears in the Rhs, what semantics should be used for its ForStmts?

And I'd suggest this as a criteria for judging whether this belongs in types.Info or in types.Package/types.Object/etc directly: do we anticipate needing this information for imported packages/objects ever, or only for the current package?

E.g., I'm thinking Package.GoVersion was probably a mistake, because I don't think we care about the Go version used to compile an imported package. It probably could have just gone in types.Info. Similarly, I'm thinking the per-file versioning info belongs in Info, not Package/Object.

Info.FileVersion(token.Pos) string seems fine to me, but that also means adding a Fset *token.FileSet field, I think. (Probably not a bad idea anyway though; means users only need to plumb a *types.Info, not a *token.FileSet and *types.Info.)

Info.FileVersion(token.Pos) string seems fine to me, but that also means adding a Fset *token.FileSet field, I think.

i.e. back to Russ's suggestion, but older and wiser. ;-) I'm fine with that. I don't think the FileSet is strictly necessary since clients invariably have one handy already, but you make a good point that putting the Fset in the Info would reduce the need for plumbing and the dependency on external side tables. (I wish I'd done something like that in Analysis.Diagnostic.)

Should the type checker populate this part of Info unconditionally? That's fine by me as in the common case it's a map of zero or one entries.

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).

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

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

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

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

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

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

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

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

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.

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