From 66dd5c09e2d77b19030c13f4028ad7f649514d29 Mon Sep 17 00:00:00 2001
From: Jiyong Park <jiyong@google.com>
Date: Wed, 24 Feb 2021 01:22:57 +0900
Subject: [PATCH] android:path attribute is respected for fields in a slice of
 struct

This change fixes a bug that android:path attribute is ignored when
annotated field is in a slice of struct.

blueprint now traverses fields whose type is slice of struct and
provides the index list in the case. Soong is modified so that it checks
whether the field being referenced is a slice of struct or not. If that
is the case, it gathers field values from each of the elements. If not,
it follows the original path.

Bug: 181018147
Test: m nothing
Change-Id: I220efb6feaa525a00939654459b2998e98e7ad56
---
 android/path_properties.go      | 87 ++++++++++++++++++++-------------
 android/path_properties_test.go | 37 +++++++++++++-
 2 files changed, 89 insertions(+), 35 deletions(-)

diff --git a/android/path_properties.go b/android/path_properties.go
index 4bb706aa8..853e5a9a7 100644
--- a/android/path_properties.go
+++ b/android/path_properties.go
@@ -76,52 +76,73 @@ func pathPropertiesForPropertyStruct(ps interface{}) []string {
 	var ret []string
 
 	for _, i := range pathPropertyIndexes {
-		// Turn an index into a field.
-		sv := fieldByIndex(v, i)
-		if !sv.IsValid() {
-			// Skip properties inside a nil pointer.
-			continue
-		}
-
-		// If the field is a non-nil pointer step into it.
-		if sv.Kind() == reflect.Ptr {
-			if sv.IsNil() {
+		var values []reflect.Value
+		fieldsByIndex(v, i, &values)
+		for _, sv := range values {
+			if !sv.IsValid() {
+				// Skip properties inside a nil pointer.
 				continue
 			}
-			sv = sv.Elem()
-		}
 
-		// Collect paths from all strings and slices of strings.
-		switch sv.Kind() {
-		case reflect.String:
-			ret = append(ret, sv.String())
-		case reflect.Slice:
-			ret = append(ret, sv.Interface().([]string)...)
-		default:
-			panic(fmt.Errorf(`field %s in type %s has tag android:"path" but is not a string or slice of strings, it is a %s`,
-				v.Type().FieldByIndex(i).Name, v.Type(), sv.Type()))
+			// If the field is a non-nil pointer step into it.
+			if sv.Kind() == reflect.Ptr {
+				if sv.IsNil() {
+					continue
+				}
+				sv = sv.Elem()
+			}
+
+			// Collect paths from all strings and slices of strings.
+			switch sv.Kind() {
+			case reflect.String:
+				ret = append(ret, sv.String())
+			case reflect.Slice:
+				ret = append(ret, sv.Interface().([]string)...)
+			default:
+				panic(fmt.Errorf(`field %s in type %s has tag android:"path" but is not a string or slice of strings, it is a %s`,
+					v.Type().FieldByIndex(i).Name, v.Type(), sv.Type()))
+			}
 		}
 	}
 
 	return ret
 }
 
-// fieldByIndex is like reflect.Value.FieldByIndex, but returns an invalid reflect.Value when
-// traversing a nil pointer to a struct.
-func fieldByIndex(v reflect.Value, index []int) reflect.Value {
+// fieldsByIndex is similar to reflect.Value.FieldByIndex, but is more robust: it doesn't track
+// nil pointers and it returns multiple values when there's slice of struct.
+func fieldsByIndex(v reflect.Value, index []int, values *[]reflect.Value) {
+	// leaf case
 	if len(index) == 1 {
-		return v.Field(index[0])
-	}
-	for _, x := range index {
-		if v.Kind() == reflect.Ptr {
-			if v.IsNil() {
-				return reflect.Value{}
+		if isSliceOfStruct(v) {
+			for i := 0; i < v.Len(); i++ {
+				*values = append(*values, v.Index(i).Field(index[0]))
 			}
-			v = v.Elem()
+		} else {
+			*values = append(*values, v.Field(index[0]))
 		}
-		v = v.Field(x)
+		return
 	}
-	return v
+
+	// recursion
+	if v.Kind() == reflect.Ptr {
+		// don't track nil pointer
+		if v.IsNil() {
+			return
+		}
+		v = v.Elem()
+	} else if isSliceOfStruct(v) {
+		// do the recursion for all elements
+		for i := 0; i < v.Len(); i++ {
+			fieldsByIndex(v.Index(i).Field(index[0]), index[1:], values)
+		}
+		return
+	}
+	fieldsByIndex(v.Field(index[0]), index[1:], values)
+	return
+}
+
+func isSliceOfStruct(v reflect.Value) bool {
+	return v.Kind() == reflect.Slice && v.Type().Elem().Kind() == reflect.Struct
 }
 
 var pathPropertyIndexesCache OncePer
diff --git a/android/path_properties_test.go b/android/path_properties_test.go
index f964d9f1e..2aab74835 100644
--- a/android/path_properties_test.go
+++ b/android/path_properties_test.go
@@ -33,12 +33,21 @@ type pathDepsMutatorTestModule struct {
 		Foo string `android:"path"`
 	}
 
+	// nested slices of struct
+	props3 struct {
+		X []struct {
+			Y []struct {
+				Z []string `android:"path"`
+			}
+		}
+	}
+
 	sourceDeps []string
 }
 
 func pathDepsMutatorTestModuleFactory() Module {
 	module := &pathDepsMutatorTestModule{}
-	module.AddProperties(&module.props, &module.props2)
+	module.AddProperties(&module.props, &module.props2, &module.props3)
 	InitAndroidArchModule(module, DeviceSupported, MultilibBoth)
 	return module
 }
@@ -73,8 +82,20 @@ func TestPathDepsMutator(t *testing.T) {
 				bar: [":b"],
 				baz: ":c{.bar}",
 				qux: ":d",
+				x: [
+					{
+						y: [
+							{
+								z: [":x", ":y"],
+							},
+							{
+								z: [":z"],
+							},
+						],
+					},
+				],
 			}`,
-			deps: []string{"a", "b", "c"},
+			deps: []string{"a", "b", "c", "x", "y", "z"},
 		},
 		{
 			name: "arch variant",
@@ -113,6 +134,18 @@ func TestPathDepsMutator(t *testing.T) {
 				filegroup {
 					name: "d",
 				}
+
+				filegroup {
+					name: "x",
+				}
+
+				filegroup {
+					name: "y",
+				}
+
+				filegroup {
+					name: "z",
+				}
 			`
 
 			config := TestArchConfig(buildDir, nil, bp, nil)