]> Cypherpunks repositories - gostls13.git/commitdiff
go/types, types2: simplify Checker.resolveBaseTypeName (cleanup)
authorRobert Griesemer <gri@golang.org>
Wed, 20 Nov 2024 22:57:43 +0000 (14:57 -0800)
committerGopher Robot <gobot@golang.org>
Thu, 21 Nov 2024 05:01:49 +0000 (05:01 +0000)
Method receivers that denote cgo-generated types are not permitted
per issues #60725 and #57926. There's no need to collect such methods
in the first place. Simplify Checker.resolveBaseTypeName so that it
doesn't find a base type name in these cases.

Also, simplify the test case for issue #59944 and update it to use
current cgo-generated output.

For #60725.
For #57926.
For #59944.

Change-Id: I70594daebc3d4d594c5b06be138f66f8927b0e58
Reviewed-on: https://go-review.googlesource.com/c/go/+/630395
Reviewed-by: Robert Findley <rfindley@google.com>
Reviewed-by: Robert Griesemer <gri@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Auto-Submit: Robert Griesemer <gri@google.com>

src/cmd/compile/internal/types2/issues_test.go
src/cmd/compile/internal/types2/resolver.go
src/go/types/issues_test.go
src/go/types/resolver.go

index aea1cbb16368bbacbaa0632ca0377f071530b316..60b28c4bf859725ac0d6dda823c0bc4a1e7d685c 100644 (file)
@@ -838,25 +838,19 @@ func (S) M5(struct {S;t}) {}
 func TestIssue59944(t *testing.T) {
        testenv.MustHaveCGO(t)
 
-       // The typechecker should resolve methods declared on aliases of cgo types.
+       // Methods declared on aliases of cgo types are not permitted.
        const src = `// -gotypesalias=1
 
 package p
 
 /*
-struct layout {
-       int field;
-};
+struct layout {};
 */
 import "C"
 
 type Layout = C.struct_layout
 
-func (l *Layout /* ERROR "cannot define new methods on non-local type Layout" */ ) Binding() {}
-
-func _() {
-       _ = (*Layout).Binding
-}
+func (*Layout /* ERROR "cannot define new methods on non-local type Layout" */) Binding() {}
 `
 
        // code generated by cmd/cgo for the above source.
@@ -879,10 +873,12 @@ func _Cgo_ptr(ptr unsafe.Pointer) unsafe.Pointer { return ptr }
 var _Cgo_always_false bool
 //go:linkname _Cgo_use runtime.cgoUse
 func _Cgo_use(interface{})
-type _Ctype_int int32
-
+//go:linkname _Cgo_keepalive runtime.cgoKeepAlive
+//go:noescape
+func _Cgo_keepalive(interface{})
+//go:linkname _Cgo_no_callback runtime.cgoNoCallback
+func _Cgo_no_callback(bool)
 type _Ctype_struct_layout struct {
-       field _Ctype_int
 }
 
 type _Ctype_void [0]byte
@@ -891,9 +887,11 @@ type _Ctype_void [0]byte
 func _cgo_runtime_cgocall(unsafe.Pointer, uintptr) int32
 
 //go:linkname _cgoCheckPointer runtime.cgoCheckPointer
+//go:noescape
 func _cgoCheckPointer(interface{}, interface{})
 
 //go:linkname _cgoCheckResult runtime.cgoCheckResult
+//go:noescape
 func _cgoCheckResult(interface{})
 `
        testFiles(t, []string{"p.go", "_cgo_gotypes.go"}, [][]byte{[]byte(src), []byte(cgoTypes)}, 0, false, func(cfg *Config) {
index 3aad3c4ada642e5fafa79037df70581b3624cad3..694c035ab5b36b4bd3c9e33673d2970ae8f46042 100644 (file)
@@ -498,41 +498,11 @@ func (check *Checker) collectObjects() {
                return
        }
 
-       // lookupScope returns the file scope which contains the given name,
-       // or nil if the name is not found in any scope. The search does not
-       // step inside blocks (function bodies).
-       // This function is only used in conjuction with import "C", and even
-       // then only rarely. It doesn't have to be particularly fast.
-       lookupScope := func(name *syntax.Name) *Scope {
-               for i, file := range check.files {
-                       found := false
-                       syntax.Inspect(file, func(n syntax.Node) bool {
-                               if found {
-                                       return false // we're done
-                               }
-                               switch n := n.(type) {
-                               case *syntax.Name:
-                                       if n == name {
-                                               found = true
-                                               return false
-                                       }
-                               case *syntax.BlockStmt:
-                                       return false // don't descend into function bodies
-                               }
-                               return true
-                       })
-                       if found {
-                               return fileScopes[i]
-                       }
-               }
-               return nil
-       }
-
        check.methods = make(map[*TypeName][]*Func)
        for i := range methods {
                m := &methods[i]
                // Determine the receiver base type and associate m with it.
-               ptr, base := check.resolveBaseTypeName(m.ptr, m.recv, lookupScope)
+               ptr, base := check.resolveBaseTypeName(m.ptr, m.recv)
                if base != nil {
                        m.obj.hasPtrRecv_ = ptr
                        check.methods[base] = append(check.methods[base], m.obj)
@@ -585,107 +555,75 @@ func (check *Checker) unpackRecv(rtyp syntax.Expr, unpackParams bool) (ptr bool,
        return
 }
 
-// resolveBaseTypeName returns the non-alias base type name for recvName, and whether
+// resolveBaseTypeName returns the non-alias base type name for the given name, and whether
 // there was a pointer indirection to get to it. The base type name must be declared
-// in package scope, and there can be at most one pointer indirection. If no such type
-// name exists, the returned base is nil.
-func (check *Checker) resolveBaseTypeName(recvHasPtr bool, recvName *syntax.Name, lookupScope func(*syntax.Name) *Scope) (ptr bool, base *TypeName) {
-       // Algorithm: Starting from a type expression, which may be a name,
+// in package scope, and there can be at most one pointer indirection. Traversals
+// through generic alias types are not permitted. If no such type name exists, the
+// returned base is nil.
+func (check *Checker) resolveBaseTypeName(ptr bool, name *syntax.Name) (ptr_ bool, base *TypeName) {
+       // Algorithm: Starting from name, which is expected to denote a type,
        // we follow that type through non-generic alias declarations until
-       // we reach a non-alias type name. A single pointer indirection and
-       // references to cgo types are permitted.
-       ptr = recvHasPtr
-       var typ syntax.Expr = recvName
+       // we reach a non-alias type name.
        var seen map[*TypeName]bool
-       for {
-               // The syntax parser strips unnecessary parentheses; calling Unparen is not needed.
-               // typ = syntax.Unparen(typ)
-
-               // check if we have a pointer type
-               // if pexpr, _ := typ.(*ast.StarExpr); pexpr != nil {
-               if pexpr, _ := typ.(*syntax.Operation); pexpr != nil && pexpr.Op == syntax.Mul && pexpr.Y == nil {
-                       // if we've already seen a pointer, we're done
-                       if ptr {
-                               return false, nil
-                       }
-                       ptr = true
-                       typ = syntax.Unparen(pexpr.X) // continue with pointer base type
-               }
-
-               // typ must be a name, or a C.name cgo selector.
-               var name string
-               switch typ := typ.(type) {
-               case *syntax.Name:
-                       name = typ.Value
-               case *syntax.SelectorExpr:
-                       // C.struct_foo is a valid type name for packages using cgo.
-                       // See go.dev/issue/59944.
-                       // TODO(gri) why is it possible to associate methods with C types?
-                       //
-                       // Detect this case, and adjust name so that the correct TypeName is
-                       // resolved below.
-                       if ident, _ := typ.X.(*syntax.Name); ident != nil && ident.Value == "C" {
-                               // Check whether "C" actually resolves to an import of "C", by looking
-                               // in the appropriate file scope.
-                               obj := lookupScope(ident).Lookup(ident.Value) // the fileScope must always be found
-                               // If Config.go115UsesCgo is set, the typechecker will resolve Cgo
-                               // selectors to their cgo name. We must do the same here.
-                               if pname, _ := obj.(*PkgName); pname != nil {
-                                       if pname.imported.cgo { // only set if Config.go115UsesCgo is set
-                                               name = "_Ctype_" + typ.Sel.Value
-                                       }
-                               }
-                       }
-                       if name == "" {
-                               return false, nil
-                       }
-               // An instantiated type may appear on the RHS of an alias declaration.
-               // Defining new methods with receivers that are generic aliases (or
-               // which refer to generic aliases) is not permitted, so we're done.
-               // Treat like the default case.
-               // case *syntax.IndexExpr:
-               default:
-                       return false, nil
-               }
-
+       for name != nil {
                // name must denote an object found in the current package scope
                // (note that dot-imported objects are not in the package scope!)
-               obj := check.pkg.scope.Lookup(name)
+               obj := check.pkg.scope.Lookup(name.Value)
                if obj == nil {
-                       return false, nil
+                       break
                }
 
                // the object must be a type name...
                tname, _ := obj.(*TypeName)
                if tname == nil {
-                       return false, nil
+                       break
                }
 
                // ... which we have not seen before
                if seen[tname] {
-                       return false, nil
+                       break
                }
 
-               // we're done if tdecl defined tname as a new type
-               // (rather than an alias)
+               // we're done if tdecl describes a defined type (not an alias)
                tdecl := check.objMap[tname].tdecl // must exist for objects in package scope
                if !tdecl.Alias {
                        return ptr, tname
                }
 
-               // we're done if tdecl defined a generic alias
+               // an alias must not be generic
                // (importantly, we must not collect such methods - was https://go.dev/issue/70417)
                if tdecl.TParamList != nil {
-                       return false, nil
+                       break
                }
 
-               // otherwise, continue resolving
-               typ = tdecl.Type
+               // otherwise, remember this type name and continue resolving
                if seen == nil {
                        seen = make(map[*TypeName]bool)
                }
                seen[tname] = true
+
+               // The syntax parser strips unnecessary parentheses; call Unparen for consistency with go/types.
+               typ := syntax.Unparen(tdecl.Type)
+
+               // dereference a pointer type
+               if pexpr, _ := typ.(*syntax.Operation); pexpr != nil && pexpr.Op == syntax.Mul && pexpr.Y == nil {
+                       // if we've already seen a pointer, we're done
+                       if ptr {
+                               break
+                       }
+                       ptr = true
+                       typ = syntax.Unparen(pexpr.X) // continue with pointer base type
+               }
+
+               // After dereferencing, typ must be a locally defined type name.
+               // Referring to other packages (qualified identifiers) or going
+               // through instantiated types (index expressions) is not permitted,
+               // so we can ignore those.
+               name, _ = typ.(*syntax.Name)
        }
+
+       // no base type found
+       return false, nil
 }
 
 // packageObjects typechecks all package objects, but not function bodies.
index 1c4c450b40380fdf9329efdfff6490cf9b235de1..3eb34cf2d0dee520350d72b2911ec9a7e9a76f21 100644 (file)
@@ -847,25 +847,19 @@ func (S) M5(struct {S;t}) {}
 func TestIssue59944(t *testing.T) {
        testenv.MustHaveCGO(t)
 
-       // The typechecker should resolve methods declared on aliases of cgo types.
+       // Methods declared on aliases of cgo types are not permitted.
        const src = `// -gotypesalias=1
 
 package p
 
 /*
-struct layout {
-       int field;
-};
+struct layout {};
 */
 import "C"
 
 type Layout = C.struct_layout
 
-func (l *Layout /* ERROR "cannot define new methods on non-local type Layout" */ ) Binding() {}
-
-func _() {
-       _ = (*Layout).Binding
-}
+func (*Layout /* ERROR "cannot define new methods on non-local type Layout" */) Binding() {}
 `
 
        // code generated by cmd/cgo for the above source.
@@ -888,10 +882,12 @@ func _Cgo_ptr(ptr unsafe.Pointer) unsafe.Pointer { return ptr }
 var _Cgo_always_false bool
 //go:linkname _Cgo_use runtime.cgoUse
 func _Cgo_use(interface{})
-type _Ctype_int int32
-
+//go:linkname _Cgo_keepalive runtime.cgoKeepAlive
+//go:noescape
+func _Cgo_keepalive(interface{})
+//go:linkname _Cgo_no_callback runtime.cgoNoCallback
+func _Cgo_no_callback(bool)
 type _Ctype_struct_layout struct {
-       field _Ctype_int
 }
 
 type _Ctype_void [0]byte
@@ -900,9 +896,11 @@ type _Ctype_void [0]byte
 func _cgo_runtime_cgocall(unsafe.Pointer, uintptr) int32
 
 //go:linkname _cgoCheckPointer runtime.cgoCheckPointer
+//go:noescape
 func _cgoCheckPointer(interface{}, interface{})
 
 //go:linkname _cgoCheckResult runtime.cgoCheckResult
+//go:noescape
 func _cgoCheckResult(interface{})
 `
        testFiles(t, []string{"p.go", "_cgo_gotypes.go"}, [][]byte{[]byte(src), []byte(cgoTypes)}, false, func(cfg *Config) {
index 1520422ebad6c2d368d0f8f27a0251768b9c4487..ef9ffb5013e0445aa6a14a5c7376dd4c59a049b9 100644 (file)
@@ -489,41 +489,11 @@ func (check *Checker) collectObjects() {
                return
        }
 
-       // lookupScope returns the file scope which contains the given name,
-       // or nil if the name is not found in any scope. The search does not
-       // step inside blocks (function bodies).
-       // This function is only used in conjuction with import "C", and even
-       // then only rarely. It doesn't have to be particularly fast.
-       lookupScope := func(name *ast.Ident) *Scope {
-               for i, file := range check.files {
-                       found := false
-                       ast.Inspect(file, func(n ast.Node) bool {
-                               if found {
-                                       return false // we're done
-                               }
-                               switch n := n.(type) {
-                               case *ast.Ident:
-                                       if n == name {
-                                               found = true
-                                               return false
-                                       }
-                               case *ast.BlockStmt:
-                                       return false // don't descend into function bodies
-                               }
-                               return true
-                       })
-                       if found {
-                               return fileScopes[i]
-                       }
-               }
-               return nil
-       }
-
        check.methods = make(map[*TypeName][]*Func)
        for i := range methods {
                m := &methods[i]
                // Determine the receiver base type and associate m with it.
-               ptr, base := check.resolveBaseTypeName(m.ptr, m.recv, lookupScope)
+               ptr, base := check.resolveBaseTypeName(m.ptr, m.recv)
                if base != nil {
                        m.obj.hasPtrRecv_ = ptr
                        check.methods[base] = append(check.methods[base], m.obj)
@@ -577,106 +547,78 @@ func (check *Checker) unpackRecv(rtyp ast.Expr, unpackParams bool) (ptr bool, ba
        return
 }
 
-// resolveBaseTypeName returns the non-alias base type name for recvName, and whether
+// resolveBaseTypeName returns the non-alias base type name for the given name, and whether
 // there was a pointer indirection to get to it. The base type name must be declared
-// in package scope, and there can be at most one pointer indirection. If no such type
-// name exists, the returned base is nil.
-func (check *Checker) resolveBaseTypeName(recvHasPtr bool, recvName *ast.Ident, lookupScope func(*ast.Ident) *Scope) (ptr bool, base *TypeName) {
-       // Algorithm: Starting from a type expression, which may be a name,
+// in package scope, and there can be at most one pointer indirection. Traversals
+// through generic alias types are not permitted. If no such type name exists, the
+// returned base is nil.
+func (check *Checker) resolveBaseTypeName(ptr bool, name *ast.Ident) (ptr_ bool, base *TypeName) {
+       // Algorithm: Starting from name, which is expected to denote a type,
        // we follow that type through non-generic alias declarations until
-       // we reach a non-alias type name. A single pointer indirection and
-       // references to cgo types are permitted.
-       ptr = recvHasPtr
-       var typ ast.Expr = recvName
+       // we reach a non-alias type name.
        var seen map[*TypeName]bool
-       for {
-               // The go/parser keeps parentheses; strip them.
-               typ = ast.Unparen(typ)
-
-               // check if we have a pointer type
-               if pexpr, _ := typ.(*ast.StarExpr); pexpr != nil {
-                       // if we've already seen a pointer, we're done
-                       if ptr {
-                               return false, nil
-                       }
-                       ptr = true
-                       typ = ast.Unparen(pexpr.X) // continue with pointer base type
-               }
-
-               // typ must be a name, or a C.name cgo selector.
-               var name string
-               switch typ := typ.(type) {
-               case *ast.Ident:
-                       name = typ.Name
-               case *ast.SelectorExpr:
-                       // C.struct_foo is a valid type name for packages using cgo.
-                       // See go.dev/issue/59944.
-                       // TODO(gri) why is it possible to associate methods with C types?
-                       //
-                       // Detect this case, and adjust name so that the correct TypeName is
-                       // resolved below.
-                       if ident, _ := typ.X.(*ast.Ident); ident != nil && ident.Name == "C" {
-                               // Check whether "C" actually resolves to an import of "C", by looking
-                               // in the appropriate file scope.
-                               obj := lookupScope(ident).Lookup(ident.Name) // the fileScope must always be found
-                               // If Config.go115UsesCgo is set, the typechecker will resolve Cgo
-                               // selectors to their cgo name. We must do the same here.
-                               if pname, _ := obj.(*PkgName); pname != nil {
-                                       if pname.imported.cgo { // only set if Config.go115UsesCgo is set
-                                               name = "_Ctype_" + typ.Sel.Name
-                                       }
-                               }
-                       }
-                       if name == "" {
-                               return false, nil
-                       }
-               // An instantiated type may appear on the RHS of an alias declaration.
-               // Defining new methods with receivers that are generic aliases (or
-               // which refer to generic aliases) is not permitted, so we're done.
-               // Treat like the default case.
-               // case *ast.IndexExpr, *ast.IndexListExpr:
-               default:
-                       return false, nil
-               }
-
+       for name != nil {
                // name must denote an object found in the current package scope
                // (note that dot-imported objects are not in the package scope!)
-               obj := check.pkg.scope.Lookup(name)
+               obj := check.pkg.scope.Lookup(name.Name)
                if obj == nil {
-                       return false, nil
+                       break
                }
 
                // the object must be a type name...
                tname, _ := obj.(*TypeName)
                if tname == nil {
-                       return false, nil
+                       break
                }
 
                // ... which we have not seen before
                if seen[tname] {
-                       return false, nil
+                       break
                }
 
-               // we're done if tdecl defined tname as a new type
-               // (rather than an alias)
+               // we're done if tdecl describes a defined type (not an alias)
                tdecl := check.objMap[tname].tdecl // must exist for objects in package scope
                if !tdecl.Assign.IsValid() {
                        return ptr, tname
                }
 
-               // we're done if tdecl defined a generic alias
+               // an alias must not be generic
                // (importantly, we must not collect such methods - was https://go.dev/issue/70417)
                if tdecl.TypeParams != nil {
-                       return false, nil
+                       break
                }
 
-               // otherwise, continue resolving
-               typ = tdecl.Type
+               // otherwise, remember this type name and continue resolving
                if seen == nil {
                        seen = make(map[*TypeName]bool)
                }
                seen[tname] = true
+
+               // The go/parser keeps parentheses; strip them, if any.
+               typ := ast.Unparen(tdecl.Type)
+
+               // dereference a pointer type
+               if pexpr, _ := typ.(*ast.StarExpr); pexpr != nil {
+                       // if we've already seen a pointer, we're done
+                       if ptr {
+                               break
+                       }
+                       ptr = true
+                       typ = ast.Unparen(pexpr.X) // continue with pointer base type
+               }
+
+               // After dereferencing, typ must be a locally defined type name.
+               // Referring to other packages (qualified identifiers) or going
+               // through instantiated types (index expressions) is not permitted,
+               // so we can ignore those.
+               name, _ = typ.(*ast.Ident)
+               if name == nil {
+                       break
+               }
        }
+
+       // no base type found
+       return false, nil
 }
 
 // packageObjects typechecks all package objects, but not function bodies.