]> Cypherpunks repositories - gostls13.git/commitdiff
go/types: don't fail fast on Go version errors
authorAlan Donovan <adonovan@google.com>
Tue, 26 Mar 2024 16:45:36 +0000 (12:45 -0400)
committerAlan Donovan <adonovan@google.com>
Wed, 3 Apr 2024 22:50:48 +0000 (22:50 +0000)
Many tools (especially in the IDE) rely on type information
being computed even for packages that have some type errors.
Previously, there were two early (error) exits in checkFiles
that violated this invariant, one related to FakeImportC
and one related to a too-new Config.GoVersion.
(The FakeImportC one is rarely encountered in practice,
but the GoVersion one, which was recently downgraded from
a panic by CL 507975, was a source of crashes
due to incomplete type information.)

This change moves both of those errors out of checkFiles
so that they report localized errors and don't obstruct
type checking. A test exercises the errors, and that
type annotations are produced.

Also, we restructure and document checkFiles to make clear
that it is never supposed to stop early.

Updates #66525

Change-Id: I9c6210e30bbf619f32a21157f17864b09cfb5cf2
Reviewed-on: https://go-review.googlesource.com/c/go/+/574495
Reviewed-by: Tim King <taking@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>

src/cmd/compile/internal/types2/api_test.go
src/cmd/compile/internal/types2/check.go
src/cmd/compile/internal/types2/resolver.go
src/go/types/api_test.go
src/go/types/check.go
src/go/types/resolver.go
src/internal/types/errors/code_string.go
src/internal/types/errors/codes.go

index bab120ff9335d13a68e83ab4ef4304757147ccdf..008a5302abdca4a21998bdf5aae86110579dfe90 100644 (file)
@@ -2937,3 +2937,51 @@ func TestFileVersions(t *testing.T) {
                }
        }
 }
+
+// TestTooNew ensures that "too new" errors are emitted when the file
+// or module is tagged with a newer version of Go than this go/types.
+func TestTooNew(t *testing.T) {
+       for _, test := range []struct {
+               goVersion   string // package's Go version (as if derived from go.mod file)
+               fileVersion string // file's Go version (becomes a build tag)
+               wantErr     string // expected substring of concatenation of all errors
+       }{
+               {"go1.98", "", "package requires newer Go version go1.98"},
+               {"", "go1.99", "p:2:9: file requires newer Go version go1.99"},
+               {"go1.98", "go1.99", "package requires newer Go version go1.98"}, // (two
+               {"go1.98", "go1.99", "file requires newer Go version go1.99"},    // errors)
+       } {
+               var src string
+               if test.fileVersion != "" {
+                       src = "//go:build " + test.fileVersion + "\n"
+               }
+               src += "package p; func f()"
+
+               var errs []error
+               conf := Config{
+                       GoVersion: test.goVersion,
+                       Error:     func(err error) { errs = append(errs, err) },
+               }
+               info := &Info{Defs: make(map[*syntax.Name]Object)}
+               typecheck(src, &conf, info)
+               got := fmt.Sprint(errs)
+               if !strings.Contains(got, test.wantErr) {
+                       t.Errorf("%q: unexpected error: got %q, want substring %q",
+                               src, got, test.wantErr)
+               }
+
+               // Assert that declarations were type checked nonetheless.
+               var gotObjs []string
+               for id, obj := range info.Defs {
+                       if obj != nil {
+                               objStr := strings.ReplaceAll(fmt.Sprintf("%s:%T", id.Value, obj), "types2", "types")
+                               gotObjs = append(gotObjs, objStr)
+                       }
+               }
+               wantObjs := "f:*types.Func"
+               if !strings.Contains(fmt.Sprint(gotObjs), wantObjs) {
+                       t.Errorf("%q: got %s, want substring %q",
+                               src, gotObjs, wantObjs)
+               }
+       }
+}
index 2c6d77d6fdaeba3509c1d3801c3b266a800b3f73..b59e471e1566a5bf8262a56bc389dfbf1b030d33 100644 (file)
@@ -8,7 +8,6 @@ package types2
 
 import (
        "cmd/compile/internal/syntax"
-       "errors"
        "fmt"
        "go/constant"
        "internal/godebug"
@@ -311,6 +310,10 @@ func (check *Checker) initFiles(files []*syntax.File) {
        check.versions = versions
 
        pkgVersionOk := check.version.isValid()
+       if pkgVersionOk && len(files) > 0 && check.version.cmp(go_current) > 0 {
+               check.errorf(files[0], TooNew, "package requires newer Go version %v (application built with %v)",
+                       check.version, go_current)
+       }
        downgradeOk := check.version.cmp(go1_21) >= 0
 
        // determine Go version for each file
@@ -319,11 +322,12 @@ func (check *Checker) initFiles(files []*syntax.File) {
                // (This version string may contain dot-release numbers as in go1.20.1,
                // unlike file versions which are Go language versions only, if valid.)
                v := check.conf.GoVersion
-               // use the file version, if applicable
-               // (file versions are either the empty string or of the form go1.dd)
-               if pkgVersionOk {
-                       fileVersion := asGoVersion(file.GoVersion)
-                       if fileVersion.isValid() {
+
+               fileVersion := asGoVersion(file.GoVersion)
+               if fileVersion.isValid() {
+                       // use the file version, if applicable
+                       // (file versions are either the empty string or of the form go1.dd)
+                       if pkgVersionOk {
                                cmp := fileVersion.cmp(check.version)
                                // Go 1.21 introduced the feature of setting the go.mod
                                // go line to an early version of Go and allowing //go:build lines
@@ -346,6 +350,15 @@ func (check *Checker) initFiles(files []*syntax.File) {
                                        v = file.GoVersion
                                }
                        }
+
+                       // Report a specific error for each tagged file that's too new.
+                       // (Normally the build system will have filtered files by version,
+                       // but clients can present arbitrary files to the type checker.)
+                       if fileVersion.cmp(go_current) > 0 {
+                               // Use position of 'package [p]' for types/types2 consistency.
+                               // (Ideally we would use the //build tag itself.)
+                               check.errorf(file.PkgName, TooNew, "file requires newer Go version %v", fileVersion)
+                       }
                }
                versions[base(file.Pos())] = v // base(file.Pos()) may be nil for tests
        }
@@ -366,11 +379,7 @@ func (check *Checker) handleBailout(err *error) {
 }
 
 // Files checks the provided files as part of the checker's package.
-func (check *Checker) Files(files []*syntax.File) error { return check.checkFiles(files) }
-
-var errBadCgo = errors.New("cannot use FakeImportC and go115UsesCgo together")
-
-func (check *Checker) checkFiles(files []*syntax.File) (err error) {
+func (check *Checker) Files(files []*syntax.File) (err error) {
        if check.pkg == Unsafe {
                // Defensive handling for Unsafe, which cannot be type checked, and must
                // not be mutated. See https://go.dev/issue/61212 for an example of where
@@ -378,16 +387,20 @@ func (check *Checker) checkFiles(files []*syntax.File) (err error) {
                return nil
        }
 
-       // Note: NewChecker doesn't return an error, so we need to check the version here.
-       if check.version.cmp(go_current) > 0 {
-               return fmt.Errorf("package requires newer Go version %v", check.version)
-       }
-       if check.conf.FakeImportC && check.conf.go115UsesCgo {
-               return errBadCgo
-       }
+       // Avoid early returns here! Nearly all errors can be
+       // localized to a piece of syntax and needn't prevent
+       // type-checking of the rest of the package.
 
        defer check.handleBailout(&err)
+       check.checkFiles(files)
+       return
+}
 
+// checkFiles type-checks the specified files. Errors are reported as
+// a side effect, not by returning early, to ensure that well-formed
+// syntax is properly type annotated even in a package containing
+// errors.
+func (check *Checker) checkFiles(files []*syntax.File) {
        print := func(msg string) {
                if check.conf.Trace {
                        fmt.Println()
@@ -440,8 +453,6 @@ func (check *Checker) checkFiles(files []*syntax.File) (err error) {
        check.ctxt = nil
 
        // TODO(gri) There's more memory we should release at this point.
-
-       return
 }
 
 // processDelayed processes all delayed actions pushed after top.
index f57234806e00f91ca10b0cbad6df3f550a2af0c9..af932a80fe07dd4d1ddce2185bebe86b5d9b41a8 100644 (file)
@@ -134,6 +134,9 @@ func (check *Checker) importPackage(pos syntax.Pos, path, dir string) *Package {
 
        // no package yet => import it
        if path == "C" && (check.conf.FakeImportC || check.conf.go115UsesCgo) {
+               if check.conf.FakeImportC && check.conf.go115UsesCgo {
+                       check.error(pos, BadImportPath, "cannot use FakeImportC and go115UsesCgo together")
+               }
                imp = NewPackage("C", "C")
                imp.fake = true // package scope is not populated
                imp.cgo = check.conf.go115UsesCgo
index 52f000980418110716fea6890f16beb2d4e8b48a..ed13ebb952db37e14060d72111d43f24edfd6056 100644 (file)
@@ -2946,3 +2946,51 @@ func TestFileVersions(t *testing.T) {
                }
        }
 }
+
+// TestTooNew ensures that "too new" errors are emitted when the file
+// or module is tagged with a newer version of Go than this go/types.
+func TestTooNew(t *testing.T) {
+       for _, test := range []struct {
+               goVersion   string // package's Go version (as if derived from go.mod file)
+               fileVersion string // file's Go version (becomes a build tag)
+               wantErr     string // expected substring of concatenation of all errors
+       }{
+               {"go1.98", "", "package requires newer Go version go1.98"},
+               {"", "go1.99", "p:2:9: file requires newer Go version go1.99"},
+               {"go1.98", "go1.99", "package requires newer Go version go1.98"}, // (two
+               {"go1.98", "go1.99", "file requires newer Go version go1.99"},    // errors)
+       } {
+               var src string
+               if test.fileVersion != "" {
+                       src = "//go:build " + test.fileVersion + "\n"
+               }
+               src += "package p; func f()"
+
+               var errs []error
+               conf := Config{
+                       GoVersion: test.goVersion,
+                       Error:     func(err error) { errs = append(errs, err) },
+               }
+               info := &Info{Defs: make(map[*ast.Ident]Object)}
+               typecheck(src, &conf, info)
+               got := fmt.Sprint(errs)
+               if !strings.Contains(got, test.wantErr) {
+                       t.Errorf("%q: unexpected error: got %q, want substring %q",
+                               src, got, test.wantErr)
+               }
+
+               // Assert that declarations were type checked nonetheless.
+               var gotObjs []string
+               for id, obj := range info.Defs {
+                       if obj != nil {
+                               objStr := strings.ReplaceAll(fmt.Sprintf("%s:%T", id.Name, obj), "types2", "types")
+                               gotObjs = append(gotObjs, objStr)
+                       }
+               }
+               wantObjs := "f:*types.Func"
+               if !strings.Contains(fmt.Sprint(gotObjs), wantObjs) {
+                       t.Errorf("%q: got %s, want substring %q",
+                               src, gotObjs, wantObjs)
+               }
+       }
+}
index 763be7714f339a54369d717a640ca2448a505358..d201b3ef9f0d5297e133f42898117f8292c554e7 100644 (file)
@@ -7,7 +7,6 @@
 package types
 
 import (
-       "errors"
        "fmt"
        "go/ast"
        "go/constant"
@@ -316,6 +315,10 @@ func (check *Checker) initFiles(files []*ast.File) {
        check.versions = versions
 
        pkgVersionOk := check.version.isValid()
+       if pkgVersionOk && len(files) > 0 && check.version.cmp(go_current) > 0 {
+               check.errorf(files[0], TooNew, "package requires newer Go version %v (application built with %v)",
+                       check.version, go_current)
+       }
        downgradeOk := check.version.cmp(go1_21) >= 0
 
        // determine Go version for each file
@@ -324,11 +327,12 @@ func (check *Checker) initFiles(files []*ast.File) {
                // (This version string may contain dot-release numbers as in go1.20.1,
                // unlike file versions which are Go language versions only, if valid.)
                v := check.conf.GoVersion
-               // use the file version, if applicable
-               // (file versions are either the empty string or of the form go1.dd)
-               if pkgVersionOk {
-                       fileVersion := asGoVersion(file.GoVersion)
-                       if fileVersion.isValid() {
+
+               fileVersion := asGoVersion(file.GoVersion)
+               if fileVersion.isValid() {
+                       // use the file version, if applicable
+                       // (file versions are either the empty string or of the form go1.dd)
+                       if pkgVersionOk {
                                cmp := fileVersion.cmp(check.version)
                                // Go 1.21 introduced the feature of setting the go.mod
                                // go line to an early version of Go and allowing //go:build lines
@@ -351,6 +355,15 @@ func (check *Checker) initFiles(files []*ast.File) {
                                        v = file.GoVersion
                                }
                        }
+
+                       // Report a specific error for each tagged file that's too new.
+                       // (Normally the build system will have filtered files by version,
+                       // but clients can present arbitrary files to the type checker.)
+                       if fileVersion.cmp(go_current) > 0 {
+                               // Use position of 'package [p]' for types/types2 consistency.
+                               // (Ideally we would use the //build tag itself.)
+                               check.errorf(file.Name, TooNew, "file requires newer Go version %v (application built with %v)", fileVersion, go_current)
+                       }
                }
                versions[file] = v
        }
@@ -371,11 +384,7 @@ func (check *Checker) handleBailout(err *error) {
 }
 
 // Files checks the provided files as part of the checker's package.
-func (check *Checker) Files(files []*ast.File) error { return check.checkFiles(files) }
-
-var errBadCgo = errors.New("cannot use FakeImportC and go115UsesCgo together")
-
-func (check *Checker) checkFiles(files []*ast.File) (err error) {
+func (check *Checker) Files(files []*ast.File) (err error) {
        if check.pkg == Unsafe {
                // Defensive handling for Unsafe, which cannot be type checked, and must
                // not be mutated. See https://go.dev/issue/61212 for an example of where
@@ -383,16 +392,20 @@ func (check *Checker) checkFiles(files []*ast.File) (err error) {
                return nil
        }
 
-       // Note: NewChecker doesn't return an error, so we need to check the version here.
-       if check.version.cmp(go_current) > 0 {
-               return fmt.Errorf("package requires newer Go version %v", check.version)
-       }
-       if check.conf.FakeImportC && check.conf.go115UsesCgo {
-               return errBadCgo
-       }
+       // Avoid early returns here! Nearly all errors can be
+       // localized to a piece of syntax and needn't prevent
+       // type-checking of the rest of the package.
 
        defer check.handleBailout(&err)
+       check.checkFiles(files)
+       return
+}
 
+// checkFiles type-checks the specified files. Errors are reported as
+// a side effect, not by returning early, to ensure that well-formed
+// syntax is properly type annotated even in a package containing
+// errors.
+func (check *Checker) checkFiles(files []*ast.File) {
        print := func(msg string) {
                if check.conf._Trace {
                        fmt.Println()
@@ -445,8 +458,6 @@ func (check *Checker) checkFiles(files []*ast.File) (err error) {
        check.ctxt = nil
 
        // TODO(rFindley) There's more memory we should release at this point.
-
-       return
 }
 
 // processDelayed processes all delayed actions pushed after top.
index 1f6847d103bad39152427e883d2ce2768c5e5557..69cc6ba15426a0deaeaeb5b67aa5d9cbda0bb00f 100644 (file)
@@ -146,6 +146,9 @@ func (check *Checker) importPackage(at positioner, path, dir string) *Package {
 
        // no package yet => import it
        if path == "C" && (check.conf.FakeImportC || check.conf.go115UsesCgo) {
+               if check.conf.FakeImportC && check.conf.go115UsesCgo {
+                       check.error(at, BadImportPath, "cannot use FakeImportC and go115UsesCgo together")
+               }
                imp = NewPackage("C", "C")
                imp.fake = true // package scope is not populated
                imp.cgo = check.conf.go115UsesCgo
index 719fc73a5a7672d109fe83bf0b4bfac520a30a16..9ae675ef849d87c25202d8fca499b8d4890e69d8 100644 (file)
@@ -155,6 +155,7 @@ func _() {
        _ = x[InvalidClear-148]
        _ = x[TypeTooLarge-149]
        _ = x[InvalidMinMaxOperand-150]
+       _ = x[TooNew-151]
 }
 
 const (
@@ -163,7 +164,7 @@ const (
        _Code_name_2 = "InvalidPtrEmbedBadRecvInvalidRecvDuplicateFieldAndMethodDuplicateMethodInvalidBlankInvalidIotaMissingInitBodyInvalidInitSigInvalidInitDeclInvalidMainDeclTooManyValuesNotAnExprTruncatedFloatNumericOverflowUndefinedOpMismatchedTypesDivByZeroNonNumericIncDecUnaddressableOperandInvalidIndirectionNonIndexableOperandInvalidIndexSwappedSliceIndicesNonSliceableOperandInvalidSliceExprInvalidShiftCountInvalidShiftOperandInvalidReceiveInvalidSendDuplicateLitKeyMissingLitKeyInvalidLitIndexOversizeArrayLitMixedStructLitInvalidStructLitMissingLitFieldDuplicateLitFieldUnexportedLitFieldInvalidLitFieldUntypedLitInvalidLitAmbiguousSelectorUndeclaredImportedNameUnexportedNameUndeclaredNameMissingFieldOrMethodBadDotDotDotSyntaxNonVariadicDotDotDotMisplacedDotDotDot"
        _Code_name_3 = "InvalidDotDotDotUncalledBuiltinInvalidAppendInvalidCapInvalidCloseInvalidCopyInvalidComplexInvalidDeleteInvalidImagInvalidLenSwappedMakeArgsInvalidMakeInvalidRealInvalidAssertImpossibleAssertInvalidConversionInvalidUntypedConversionBadOffsetofSyntaxInvalidOffsetofUnusedExprUnusedVarMissingReturnWrongResultCountOutOfScopeResultInvalidCondInvalidPostDecl"
        _Code_name_4 = "InvalidIterVarInvalidRangeExprMisplacedBreakMisplacedContinueMisplacedFallthroughDuplicateCaseDuplicateDefaultBadTypeKeywordInvalidTypeSwitchInvalidExprSwitchInvalidSelectCaseUndeclaredLabelDuplicateLabelMisplacedLabelUnusedLabelJumpOverDeclJumpIntoBlockInvalidMethodExprWrongArgCountInvalidCallUnusedResultsInvalidDeferInvalidGoBadDeclRepeatedDeclInvalidUnsafeAddInvalidUnsafeSliceUnsupportedFeatureNotAGenericTypeWrongTypeArgCountCannotInferTypeArgsInvalidTypeArgInvalidInstanceCycleInvalidUnionMisplacedConstraintIfaceInvalidMethodTypeParamsMisplacedTypeParamInvalidUnsafeSliceDataInvalidUnsafeString"
-       _Code_name_5 = "InvalidClearTypeTooLargeInvalidMinMaxOperand"
+       _Code_name_5 = "InvalidClearTypeTooLargeInvalidMinMaxOperandTooNew"
 )
 
 var (
@@ -171,7 +172,7 @@ var (
        _Code_index_2 = [...]uint16{0, 15, 22, 33, 56, 71, 83, 94, 109, 123, 138, 153, 166, 175, 189, 204, 215, 230, 239, 255, 275, 293, 312, 324, 343, 362, 378, 395, 414, 428, 439, 454, 467, 482, 498, 512, 528, 543, 560, 578, 593, 603, 613, 630, 652, 666, 680, 700, 718, 738, 756}
        _Code_index_3 = [...]uint16{0, 16, 31, 44, 54, 66, 77, 91, 104, 115, 125, 140, 151, 162, 175, 191, 208, 232, 249, 264, 274, 283, 296, 312, 328, 339, 354}
        _Code_index_4 = [...]uint16{0, 14, 30, 44, 61, 81, 94, 110, 124, 141, 158, 175, 190, 204, 218, 229, 241, 254, 271, 284, 295, 308, 320, 329, 336, 348, 364, 382, 400, 415, 432, 451, 465, 485, 497, 521, 544, 562, 584, 603}
-       _Code_index_5 = [...]uint8{0, 12, 24, 44}
+       _Code_index_5 = [...]uint8{0, 12, 24, 44, 50}
 )
 
 func (i Code) String() string {
@@ -190,7 +191,7 @@ func (i Code) String() string {
        case 108 <= i && i <= 146:
                i -= 108
                return _Code_name_4[_Code_index_4[i]:_Code_index_4[i+1]]
-       case 148 <= i && i <= 150:
+       case 148 <= i && i <= 151:
                i -= 148
                return _Code_name_5[_Code_index_5[i]:_Code_index_5[i+1]]
        default:
index cae688ff874bfca78b42c56d4edda915377f7243..c0e6aa6c2daf52e5ff939e65cefa76ca76fa1eae 100644 (file)
@@ -4,7 +4,7 @@
 
 package errors
 
-//go:generate stringer -type Code codes.go
+//go:generate go run golang.org/x/tools/cmd/stringer@latest -type Code codes.go
 
 type Code int
 
@@ -1474,4 +1474,12 @@ const (
        //  var s, t []byte
        //  var _ = max(s, t)
        InvalidMinMaxOperand
+
+       // TooNew indicates that, through build tags or a go.mod file,
+       // a source file requires a version of Go that is newer than
+       // the logic of the type checker. As a consequence, the type
+       // checker may produce spurious errors or fail to report real
+       // errors. The solution is to rebuild the application with a
+       // newer Go release.
+       TooNew
 )