From c5bce7445e1792f134413ad312fd1f2211c0a55d Mon Sep 17 00:00:00 2001 From: Robert Griesemer Date: Mon, 7 Feb 2022 18:37:02 -0800 Subject: [PATCH] go/types, types2: AssertableTo is undefined for generalized interfaces Document that AssertableTo is undefined (at least for 1.18) if the first argument is a generalized interface; i.e., an interface that may only be used as a constraint in Go code. Still, implement it as we might expect it to be defined in the future, to prevent problems down the road due to Hyrum's Law. While at it, also removed the internal flag forceStrict and its one use in Checker.assertableTo; forceStrict was never enabled and if it would have been enabled, the behavior would not have been correct. Change-Id: Ie4dc9345c88d04c9640f881132154a002db22643 Reviewed-on: https://go-review.googlesource.com/c/go/+/383917 Trust: Robert Griesemer Run-TryBot: Robert Griesemer Reviewed-by: Robert Findley --- src/cmd/compile/internal/types2/api.go | 10 ++++- src/cmd/compile/internal/types2/api_test.go | 42 +++++++++++++-------- src/cmd/compile/internal/types2/check.go | 13 ------- src/cmd/compile/internal/types2/lookup.go | 19 ++++++++-- src/go/types/api.go | 10 ++++- src/go/types/api_test.go | 42 +++++++++++++-------- src/go/types/check.go | 13 ------- src/go/types/lookup.go | 19 ++++++++-- 8 files changed, 102 insertions(+), 66 deletions(-) diff --git a/src/cmd/compile/internal/types2/api.go b/src/cmd/compile/internal/types2/api.go index ee4f275bc07..6230c58401a 100644 --- a/src/cmd/compile/internal/types2/api.go +++ b/src/cmd/compile/internal/types2/api.go @@ -421,9 +421,15 @@ func (conf *Config) Check(path string, files []*syntax.File, info *Info) (*Packa } // AssertableTo reports whether a value of type V can be asserted to have type T. +// The behavior of AssertableTo is undefined if V is a generalized interface; i.e., +// an interface that may only be used as a type constraint in Go code. func AssertableTo(V *Interface, T Type) bool { - m, _ := (*Checker)(nil).assertableTo(V, T) - return m == nil + // Checker.newAssertableTo suppresses errors for invalid types, so we need special + // handling here. + if T.Underlying() == Typ[Invalid] { + return false + } + return (*Checker)(nil).newAssertableTo(V, T) == nil } // AssignableTo reports whether a value of type V is assignable to a variable of type T. diff --git a/src/cmd/compile/internal/types2/api_test.go b/src/cmd/compile/internal/types2/api_test.go index 094374f7f1b..46b184f53c9 100644 --- a/src/cmd/compile/internal/types2/api_test.go +++ b/src/cmd/compile/internal/types2/api_test.go @@ -2313,27 +2313,27 @@ type Bad Bad // invalid type conf := Config{Error: func(error) {}} pkg, _ := conf.Check(f.PkgName.Value, []*syntax.File{f}, nil) - scope := pkg.Scope() + lookup := func(tname string) Type { return pkg.Scope().Lookup(tname).Type() } var ( - EmptyIface = scope.Lookup("EmptyIface").Type().Underlying().(*Interface) - I = scope.Lookup("I").Type().(*Named) + EmptyIface = lookup("EmptyIface").Underlying().(*Interface) + I = lookup("I").(*Named) II = I.Underlying().(*Interface) - C = scope.Lookup("C").Type().(*Named) + C = lookup("C").(*Named) CI = C.Underlying().(*Interface) - Integer = scope.Lookup("Integer").Type().Underlying().(*Interface) - EmptyTypeSet = scope.Lookup("EmptyTypeSet").Type().Underlying().(*Interface) - N1 = scope.Lookup("N1").Type() + Integer = lookup("Integer").Underlying().(*Interface) + EmptyTypeSet = lookup("EmptyTypeSet").Underlying().(*Interface) + N1 = lookup("N1") N1p = NewPointer(N1) - N2 = scope.Lookup("N2").Type() + N2 = lookup("N2") N2p = NewPointer(N2) - N3 = scope.Lookup("N3").Type() - N4 = scope.Lookup("N4").Type() - Bad = scope.Lookup("Bad").Type() + N3 = lookup("N3") + N4 = lookup("N4") + Bad = lookup("Bad") ) tests := []struct { - t Type - i *Interface + V Type + T *Interface want bool }{ {I, II, true}, @@ -2364,8 +2364,20 @@ type Bad Bad // invalid type } for _, test := range tests { - if got := Implements(test.t, test.i); got != test.want { - t.Errorf("Implements(%s, %s) = %t, want %t", test.t, test.i, got, test.want) + if got := Implements(test.V, test.T); got != test.want { + t.Errorf("Implements(%s, %s) = %t, want %t", test.V, test.T, got, test.want) + } + + // The type assertion x.(T) is valid if T is an interface or if T implements the type of x. + // The assertion is never valid if T is a bad type. + V := test.T + T := test.V + want := false + if _, ok := T.Underlying().(*Interface); (ok || Implements(T, V)) && T != Bad { + want = true + } + if got := AssertableTo(V, T); got != want { + t.Errorf("AssertableTo(%s, %s) = %t, want %t", V, T, got, want) } } } diff --git a/src/cmd/compile/internal/types2/check.go b/src/cmd/compile/internal/types2/check.go index bfed16993b5..535de0256cb 100644 --- a/src/cmd/compile/internal/types2/check.go +++ b/src/cmd/compile/internal/types2/check.go @@ -18,19 +18,6 @@ var nopos syntax.Pos // debugging/development support const debug = false // leave on during development -// If forceStrict is set, the type-checker enforces additional -// rules not specified by the Go 1 spec, but which will -// catch guaranteed run-time errors if the respective -// code is executed. In other words, programs passing in -// strict mode are Go 1 compliant, but not all Go 1 programs -// will pass in strict mode. The additional rules are: -// -// - A type assertion x.(T) where T is an interface type -// is invalid if any (statically known) method that exists -// for both x and T have different signatures. -// -const forceStrict = false - // exprInfo stores information about an untyped expression. type exprInfo struct { isLhs bool // expression is lhs operand of a shift with delayed type-check diff --git a/src/cmd/compile/internal/types2/lookup.go b/src/cmd/compile/internal/types2/lookup.go index b8ddd94cd78..9987da48546 100644 --- a/src/cmd/compile/internal/types2/lookup.go +++ b/src/cmd/compile/internal/types2/lookup.go @@ -425,18 +425,31 @@ func (check *Checker) funcString(f *Func) string { // method required by V and whether it is missing or just has the wrong type. // The receiver may be nil if assertableTo is invoked through an exported API call // (such as AssertableTo), i.e., when all methods have been type-checked. -// If the global constant forceStrict is set, assertions that are known to fail -// are not permitted. +// TODO(gri) replace calls to this function with calls to newAssertableTo. func (check *Checker) assertableTo(V *Interface, T Type) (method, wrongType *Func) { // no static check is required if T is an interface // spec: "If T is an interface type, x.(T) asserts that the // dynamic type of x implements the interface T." - if IsInterface(T) && !forceStrict { + if IsInterface(T) { return } + // TODO(gri) fix this for generalized interfaces return check.missingMethod(T, V, false) } +// newAssertableTo reports whether a value of type V can be asserted to have type T. +// It also implements behavior for interfaces that currently are only permitted +// in constraint position (we have not yet defined that behavior in the spec). +func (check *Checker) newAssertableTo(V *Interface, T Type) error { + // no static check is required if T is an interface + // spec: "If T is an interface type, x.(T) asserts that the + // dynamic type of x implements the interface T." + if IsInterface(T) { + return nil + } + return check.implements(T, V) +} + // deref dereferences typ if it is a *Pointer and returns its base and true. // Otherwise it returns (typ, false). func deref(typ Type) (Type, bool) { diff --git a/src/go/types/api.go b/src/go/types/api.go index 2776e05232c..828461477b9 100644 --- a/src/go/types/api.go +++ b/src/go/types/api.go @@ -417,9 +417,15 @@ func (conf *Config) Check(path string, fset *token.FileSet, files []*ast.File, i } // AssertableTo reports whether a value of type V can be asserted to have type T. +// The behavior of AssertableTo is undefined if V is a generalized interface; i.e., +// an interface that may only be used as a type constraint in Go code. func AssertableTo(V *Interface, T Type) bool { - m, _ := (*Checker)(nil).assertableTo(V, T) - return m == nil + // Checker.newAssertableTo suppresses errors for invalid types, so we need special + // handling here. + if T.Underlying() == Typ[Invalid] { + return false + } + return (*Checker)(nil).newAssertableTo(V, T) == nil } // AssignableTo reports whether a value of type V is assignable to a variable of type T. diff --git a/src/go/types/api_test.go b/src/go/types/api_test.go index a18ee16c7b5..85452dffe63 100644 --- a/src/go/types/api_test.go +++ b/src/go/types/api_test.go @@ -2306,27 +2306,27 @@ type Bad Bad // invalid type conf := Config{Error: func(error) {}} pkg, _ := conf.Check(f.Name.Name, fset, []*ast.File{f}, nil) - scope := pkg.Scope() + lookup := func(tname string) Type { return pkg.Scope().Lookup(tname).Type() } var ( - EmptyIface = scope.Lookup("EmptyIface").Type().Underlying().(*Interface) - I = scope.Lookup("I").Type().(*Named) + EmptyIface = lookup("EmptyIface").Underlying().(*Interface) + I = lookup("I").(*Named) II = I.Underlying().(*Interface) - C = scope.Lookup("C").Type().(*Named) + C = lookup("C").(*Named) CI = C.Underlying().(*Interface) - Integer = scope.Lookup("Integer").Type().Underlying().(*Interface) - EmptyTypeSet = scope.Lookup("EmptyTypeSet").Type().Underlying().(*Interface) - N1 = scope.Lookup("N1").Type() + Integer = lookup("Integer").Underlying().(*Interface) + EmptyTypeSet = lookup("EmptyTypeSet").Underlying().(*Interface) + N1 = lookup("N1") N1p = NewPointer(N1) - N2 = scope.Lookup("N2").Type() + N2 = lookup("N2") N2p = NewPointer(N2) - N3 = scope.Lookup("N3").Type() - N4 = scope.Lookup("N4").Type() - Bad = scope.Lookup("Bad").Type() + N3 = lookup("N3") + N4 = lookup("N4") + Bad = lookup("Bad") ) tests := []struct { - t Type - i *Interface + V Type + T *Interface want bool }{ {I, II, true}, @@ -2357,8 +2357,20 @@ type Bad Bad // invalid type } for _, test := range tests { - if got := Implements(test.t, test.i); got != test.want { - t.Errorf("Implements(%s, %s) = %t, want %t", test.t, test.i, got, test.want) + if got := Implements(test.V, test.T); got != test.want { + t.Errorf("Implements(%s, %s) = %t, want %t", test.V, test.T, got, test.want) + } + + // The type assertion x.(T) is valid if T is an interface or if T implements the type of x. + // The assertion is never valid if T is a bad type. + V := test.T + T := test.V + want := false + if _, ok := T.Underlying().(*Interface); (ok || Implements(T, V)) && T != Bad { + want = true + } + if got := AssertableTo(V, T); got != want { + t.Errorf("AssertableTo(%s, %s) = %t, want %t", V, T, got, want) } } } diff --git a/src/go/types/check.go b/src/go/types/check.go index a0c3700254c..6e1da04b9fc 100644 --- a/src/go/types/check.go +++ b/src/go/types/check.go @@ -24,19 +24,6 @@ const ( compilerErrorMessages = false // match compiler error messages ) -// If forceStrict is set, the type-checker enforces additional -// rules not specified by the Go 1 spec, but which will -// catch guaranteed run-time errors if the respective -// code is executed. In other words, programs passing in -// strict mode are Go 1 compliant, but not all Go 1 programs -// will pass in strict mode. The additional rules are: -// -// - A type assertion x.(T) where T is an interface type -// is invalid if any (statically known) method that exists -// for both x and T have different signatures. -// -const forceStrict = false - // exprInfo stores information about an untyped expression. type exprInfo struct { isLhs bool // expression is lhs operand of a shift with delayed type-check diff --git a/src/go/types/lookup.go b/src/go/types/lookup.go index f2f38be2660..9f1cd7667ca 100644 --- a/src/go/types/lookup.go +++ b/src/go/types/lookup.go @@ -424,18 +424,31 @@ func (check *Checker) funcString(f *Func) string { // method required by V and whether it is missing or just has the wrong type. // The receiver may be nil if assertableTo is invoked through an exported API call // (such as AssertableTo), i.e., when all methods have been type-checked. -// If the global constant forceStrict is set, assertions that are known to fail -// are not permitted. +// TODO(gri) replace calls to this function with calls to newAssertableTo. func (check *Checker) assertableTo(V *Interface, T Type) (method, wrongType *Func) { // no static check is required if T is an interface // spec: "If T is an interface type, x.(T) asserts that the // dynamic type of x implements the interface T." - if IsInterface(T) && !forceStrict { + if IsInterface(T) { return } + // TODO(gri) fix this for generalized interfaces return check.missingMethod(T, V, false) } +// newAssertableTo reports whether a value of type V can be asserted to have type T. +// It also implements behavior for interfaces that currently are only permitted +// in constraint position (we have not yet defined that behavior in the spec). +func (check *Checker) newAssertableTo(V *Interface, T Type) error { + // no static check is required if T is an interface + // spec: "If T is an interface type, x.(T) asserts that the + // dynamic type of x implements the interface T." + if IsInterface(T) { + return nil + } + return check.implements(T, V) +} + // deref dereferences typ if it is a *Pointer and returns its base and true. // Otherwise it returns (typ, false). func deref(typ Type) (Type, bool) {