go/types: should the 'newer Go version' error be reported to Config.Error?

[timothy-king]

Since go1.21, (*go/types.Checker).Files() has returned an error when a package has a GoVersion (specified in types.Config.GoVersion) that is newer than the go/types library it is compiled against. Error in question is:

	if check.version.after(version{1, goversion.Version}) {
		return fmt.Errorf("package requires newer Go version %v", check.version)
	}

x/tools/go/packages was assuming the only error not reported to types.Config.Error was types.errBadCgo. Example downstream issue: #65590.

This issue is to decide whether this error should be reported to the types.Config.Error field, and if so how?

@griesemer @adonovan

[gopherbot]

Change https://go.dev/cl/574255 mentions this issue: go/packages: report type errors unconditionally

[timothy-king]

One option that came up in code review is to report the error as a types.Error on the first package symbol and to proceed as if the GoVersion=="". This would not fail-fast and best effort types would work better for IDEs like gopls.

OTOH it is not clear whether it is acceptable to do a best effort typing when we know the package is a future GoVersion.

A minor complicating wrinkle is when all files are pinned to versions less than the version of go/types. Should there be an error on this package at all?

[adonovan]

This issue is to decide whether this error should be reported to the types.Config.Error field, and if so how?

Yes, all errors should be reported through Config.Error, and the error returned by Checker.Files should merely be the first of them. We should re-establish the invariant that a non-zero number of calls to Error is equivalent to the package being ill-typed. The two early error returns in Checker.Files both violate this invariant. The FakeImportC one was rarely exercised, but the version check (downgraded from a panic by https://go.dev/cl/507975) is much more important. Both these errors should be reported (e.g. against the first package declaration if no other location is appropriate), but should not abort type-checking of the package. We should document clearly in the code that early returns are not allowed.

Clients of go/types make widespread assumptions that the type-checker does the best it can with the syntax, and that well-formed declarations are annotated with types even if the package contains some type errors. For example, it is common to evaluate info.Defs[decl].(*types.Func) where decl is an *ast.FuncDecl, without needing a check.

I can send you a CL for the go/types change if you like.

[gopherbot]

Change https://go.dev/cl/574495 mentions this issue: go/types: don't fail fast on Go version errors

[findleyr]

Not happening for 1.23. Moving to 1.24.