compiler/checker: fix corner cases in field selector

This change implements field finding by name instead of using the reflect method of the struct type. This resolve some corner cases in the find. For example the 'fieldByName' method checks if the field is exported before checking if there is an ambiguous selector error to return. Checking if the field can be accessed by the code in the current package is done on the main struct instead of the struct where the field is located. This change fixes these issues.
open2b · Aug 24, 2021 · e6be418 · e6be418
1 parent 22a575d
commit e6be418
Show file tree

Hide file tree

Showing 3 changed files with 192 additions and 86 deletions.
diff --git a/internal/compiler/checker_expressions.go b/internal/compiler/checker_expressions.go
@@ -812,15 +812,7 @@ func (tc *typechecker) typeof(expr ast.Expression, typeExpected bool) *typeInfo
 			return method
 		}
 		// Field selector.
-		field, newName, err := tc.fieldByName(t, expr.Ident)
-		if err != nil {
-			if newName != "" && structFieldExists(t.Type, newName) {
-				panic(tc.errorf(expr, "ambiguous selector %s", expr))
-			}
-			panic(tc.errorf(expr, "%v undefined (%s)", expr, err))
-		}
-		expr.Ident = newName
-		return field
+		return tc.checkFieldSelector(t, expr)
 
 	case *ast.TypeAssertion:
 		t := tc.checkExpr(expr.Expr)
@@ -2535,6 +2527,94 @@ func (tc *typechecker) checkExplicitField(field *ast.Field, i int, blank *int) r
 	return f
 }
 
+// checkFieldSelector checks a field selector.
+func (tc *typechecker) checkFieldSelector(t *typeInfo, expr *ast.Selector) *typeInfo {
+
+	name := expr.Ident
+
+	typ := t.Type
+	if typ.Kind() == reflect.Ptr {
+		typ = t.Type.Elem()
+	}
+	if typ.Kind() != reflect.Struct {
+		panic(tc.errorf(expr, "%v undefined (type %s has no field or method %s)", expr, t.Type, name))
+	}
+
+	typ, _, encodedName := tc.findStructField(typ, expr)
+	if typ == nil {
+		panic(tc.errorf(expr, "%v undefined (type %s has no field or method %s)", expr, t.Type, name))
+	}
+	if encodedName == "" {
+		panic(tc.errorf(expr, "%v undefined (cannot refer to unexported field or method %s)", expr, name))
+	}
+
+	// Create the type info of the field.
+	ti := &typeInfo{Type: typ}
+	if t.Type.Kind() == reflect.Struct && t.Addressable() {
+		// Struct fields are addressable only if the struct is addressable.
+		ti.Properties = propertyAddressable
+	} else if t.Type.Kind() == reflect.Ptr {
+		// Pointer to struct fields are always addressable.
+		ti.Properties = propertyAddressable
+	}
+
+	expr.Ident = encodedName
+
+	return ti
+}
+
+// findStructField returns the type, depth and encoded name of the field in s
+// with name expr.Ident. If the field does not exist, typ is nil. If the
+// field exists but is not accessible from the current package code,
+// encodingName is empty.
+func (tc *typechecker) findStructField(s reflect.Type, expr *ast.Selector) (typ reflect.Type, depth int, encodedName string) {
+	n := s.NumField()
+	for i := 0; i < n; i++ {
+		f := s.Field(i)
+		if name := decodeFieldName(f.Name); name == expr.Ident {
+			typ = f.Type
+			if f.PkgPath == "" {
+				if tc.structDeclPkg[s] == tc.path {
+					encodedName = f.Name
+				} else if !strings.HasPrefix(f.Name, "𝗽") {
+					encodedName = f.Name
+				}
+			}
+			return
+		}
+	}
+	for i := 0; i < n; i++ {
+		f := s.Field(i)
+		if !f.Anonymous {
+			continue
+		}
+		t := f.Type
+		if t.Kind() == reflect.Ptr {
+			t = t.Elem()
+		}
+		if t.Kind() != reflect.Struct {
+			continue
+		}
+		t, d, n := tc.findStructField(t, expr)
+		if t == nil {
+			continue
+		}
+		d++
+		if d == depth {
+			if encodedName == "" {
+				return nil, 0, ""
+			}
+			panic(tc.errorf(expr, "ambiguous selector %s", expr))
+		}
+		if depth == 0 || d < depth {
+			typ = t
+			depth = d
+			encodedName = n
+		}
+	}
+	return
+}
+
 // encodeFieldName encodes a field name to be used in a reflect.StructField.
 // blank is a pointer to the blank identifier index.
 //

diff --git a/internal/compiler/checker_util.go b/internal/compiler/checker_util.go
@@ -10,10 +10,7 @@ import (
 	"errors"
 	"fmt"
 	"reflect"
-	"strconv"
 	"strings"
-	"unicode"
-	"unicode/utf8"
 
 	"github.com/open2b/scriggo/ast"
 	"github.com/open2b/scriggo/internal/compiler/types"
@@ -283,52 +280,6 @@ func deferGoBuiltin(name string) *typeInfo {
 	}
 }
 
-// fieldByName returns the struct field with the given name if such field
-// exists and can be accessed, else returns an error.
-//
-// If name is non-exported and the type is declared in Scriggo then name is
-// transformed and the new name is returned. For further information about this
-// check the documentation of the type checking of an *ast.StructType.
-func (tc *typechecker) fieldByName(t *typeInfo, name string) (*typeInfo, string, error) {
-
-	typ := t.Type
-	if typ.Kind() == reflect.Ptr {
-		typ = t.Type.Elem()
-	}
-	if typ.Kind() != reflect.Struct {
-		return nil, "", fmt.Errorf("type %s has no field or method %s", t.Type, name)
-	}
-
-	var innerName string
-	if r, _ := utf8.DecodeRuneInString(name); unicode.Is(unicode.Lu, r) {
-		// Exported field.
-		innerName = name
-	} else {
-		// Non-exported field.
-		if tc.structDeclPkg[typ] != tc.path {
-			return nil, "", fmt.Errorf("cannot refer to unexported field or method %s", name)
-		}
-		innerName = "𝗽" + strconv.Itoa(tc.compilation.UniqueIndex(tc.path)) + name
-	}
-
-	field, ok := typ.FieldByName(innerName)
-	if !ok {
-		return nil, innerName, fmt.Errorf("type %s has no field or method %s", t.Type, name)
-	}
-
-	// Create the type info of the field.
-	ti := &typeInfo{Type: field.Type}
-	if t.Type.Kind() == reflect.Struct && t.Addressable() {
-		// Struct fields are addressable only if the struct is addressable.
-		ti.Properties = propertyAddressable
-	} else if t.Type.Kind() == reflect.Ptr {
-		// Pointer to struct fields are always addressable.
-		ti.Properties = propertyAddressable
-	}
-
-	return ti, innerName, nil
-}
-
 // checkDuplicateParams checks if a function type contains duplicate
 // parameter names.
 func (tc *typechecker) checkDuplicateParams(fn *ast.FuncType) {
@@ -750,31 +701,3 @@ func (tc *typechecker) errTypeAssertion(typ reflect.Type, iface reflect.Type) er
 	}
 	panic("unexpected")
 }
-
-// structFieldExists reports whether the struct s has a field named name. It
-// considers the fields in s and in any embedded structs. name is the encoded
-// name of the field. If returns true also for multiple fields at the same
-// depth.
-func structFieldExists(s reflect.Type, name string) bool {
-	if s.Kind() == reflect.Ptr {
-		s = s.Elem()
-	}
-	n := s.NumField()
-	for i := 0; i < n; i++ {
-		f := s.Field(i)
-		if f.Name == name {
-			return true
-		}
-		if !f.Anonymous {
-			continue
-		}
-		t := f.Type
-		if t.Kind() == reflect.Ptr {
-			t = t.Elem()
-		}
-		if t.Kind() == reflect.Struct && structFieldExists(t, name) {
-			return true
-		}
-	}
-	return false
-}
diff --git a/test/program_test.go b/test/program_test.go
@@ -234,3 +234,106 @@ func TestCompositeStructLiterals(t *testing.T) {
 		}
 	}
 }
+
+var structFieldSelectorTests = []struct {
+	fsys fs.FS
+	pass bool
+	err  string
+}{{
+	fsys: fstest.Files{
+		"go.mod":  "module a.b\ngo 1.16",
+		"main.go": `package main; import "a.b/p"; func main() { _ = p.S{}.F }`,
+		"p/p.go":  `package p; type S struct{ F int }`,
+	},
+	pass: true,
+}, {
+	fsys: fstest.Files{
+		"go.mod":  "module a.b\ngo 1.16",
+		"main.go": `package main; import "a.b/p"; func main() { _ = p.S{}.f }`,
+		"p/p.go":  `package p; type S struct{ f int }`,
+	},
+	err: "main:1:54: p.S{}.f undefined (cannot refer to unexported field or method f)",
+}, {
+	fsys: fstest.Files{
+		"go.mod":  "module a.b\ngo 1.16",
+		"main.go": `package main; import "a.b/p"; func main() { _ = p.S{}.G }`,
+		"p/p.go":  `package p; type S struct{ F int }`,
+	},
+	err: "main:1:54: p.S{}.G undefined (type S has no field or method G)",
+}, {
+	fsys: fstest.Files{
+		"go.mod":  "module a.b\ngo 1.16",
+		"main.go": `package main; import "a.b/p"; func main() { _ = p.S{}.g }`,
+		"p/p.go":  `package p; type S struct{ F int }`,
+	},
+	err: "main:1:54: p.S{}.g undefined (type S has no field or method g)",
+}, {
+	fsys: fstest.Files{
+		"go.mod":  "module a.b\ngo 1.16",
+		"main.go": `package main; import "a.b/p"; func main() { _ = p.S{}.F }`,
+		"p/p.go":  `package p; type ( T struct { F int }; V struct { V int }; S struct{ T; V } )`,
+	},
+	pass: true,
+}, {
+	fsys: fstest.Files{
+		"go.mod":  "module a.b\ngo 1.16",
+		"main.go": `package main; import "a.b/p"; func main() { _ = p.S{}.F }`,
+		"p/p.go":  `package p; type ( T struct { F int }; V struct { F int }; S struct{ T; V } )`,
+	},
+	err: "main:1:54: ambiguous selector p.S{}.F",
+}, {
+	fsys: fstest.Files{
+		"go.mod":  "module a.b\ngo 1.16",
+		"main.go": `package main; import "a.b/p"; func main() { _ = p.S{}.F }`,
+		"p/p.go":  `package p; type ( T struct { F int }; V struct { F int }; S struct{ T; V } )`,
+	},
+	err: "main:1:54: ambiguous selector p.S{}.F",
+}, {
+	fsys: fstest.Files{
+		"go.mod":  "module a.b\ngo 1.16",
+		"main.go": `package main; import "a.b/p"; func main() { _ = p.S{}.f }`,
+		"p/p.go":  `package p; type ( T struct { f int }; V struct { f int }; S struct{ T; V } )`,
+	},
+	err: "main:1:54: p.S{}.f undefined (type S has no field or method f)", // TODO(marco): S should be p.S
+}, {
+	fsys: fstest.Files{
+		"go.mod":  "module a.b\ngo 1.16",
+		"main.go": `package main; import "a.b/p"; func main() { _ = p.S{}.F }`,
+		"p/p.go":  `package p; type ( t struct { F int }; S struct{ t } )`,
+	},
+	pass: true,
+}, {
+	fsys: fstest.Files{
+		"go.mod":  "module a.b\ngo 1.16",
+		"main.go": `package main; import "a.b/p"; func main() { _ = p.S{}.t.F }`,
+		"p/p.go":  `package p; type ( t struct { F int }; S struct{ t } )`,
+	},
+	err: "main:1:54: p.S{}.t undefined (cannot refer to unexported field or method t)",
+}, {
+	fsys: fstest.Files{
+		"go.mod":  "module a.b\ngo 1.16",
+		"main.go": `package main; import "a.b/p"; func main() { var _ string = p.S{}.F }`,
+		"p/p.go":  `package p; type ( T struct { F int }; A = T; V struct { T; F string; A }; S struct{ V } )`,
+	},
+	pass: true,
+}}
+
+// TestStructFieldSelector tests struct field selectors when the struct is
+// defined in another package.
+func TestStructFieldSelector(t *testing.T) {
+	for _, cas := range structFieldSelectorTests {
+		_, err := scriggo.Build(cas.fsys, nil)
+		if cas.pass {
+			if err != nil {
+				t.Fatalf("unexpected error: %q", err)
+			}
+		} else {
+			if err == nil {
+				t.Fatalf("expected error %q, got no error", cas.err)
+			}
+			if cas.err != err.Error() {
+				t.Fatalf("expected error %q, got error %q", cas.err, err)
+			}
+		}
+	}
+}