From f04033be81f2119f5e07e88912019464912d7641 Mon Sep 17 00:00:00 2001 From: Paul Duffin Date: Wed, 22 Sep 2021 11:51:09 +0100 Subject: [PATCH 1/2] Dedup SdkMemberType/TraitRegistry Bug: 195754365 Test: m nothing Change-Id: I3baa2535fd21a47bea2229f13cf5eb166396fe79 --- android/sdk.go | 165 +++++++++++++++++++++++-------------------------- 1 file changed, 78 insertions(+), 87 deletions(-) diff --git a/android/sdk.go b/android/sdk.go index 21e0366ba..1518a8731 100644 --- a/android/sdk.go +++ b/android/sdk.go @@ -377,6 +377,63 @@ func (b BpPrintableBase) bpPrintable() { var _ BpPrintable = BpPrintableBase{} +// sdkRegisterable defines the interface that must be implemented by objects that can be registered +// in an sdkRegistry. +type sdkRegisterable interface { + // SdkPropertyName returns the name of the corresponding property on an sdk module. + SdkPropertyName() string +} + +// sdkRegistry provides support for registering and retrieving objects that define properties for +// use by sdk and module_exports module types. +type sdkRegistry struct { + // The list of registered objects sorted by property name. + list []sdkRegisterable +} + +// copyAndAppend creates a new sdkRegistry that includes all the traits registered in +// this registry plus the supplied trait. +func (r *sdkRegistry) copyAndAppend(registerable sdkRegisterable) *sdkRegistry { + oldList := r.list + + // Copy the slice just in case this is being read while being modified, e.g. when testing. + list := make([]sdkRegisterable, 0, len(oldList)+1) + list = append(list, oldList...) + list = append(list, registerable) + + // Sort the registered objects by their property name to ensure that registry order has no effect + // on behavior. + sort.Slice(list, func(i1, i2 int) bool { + t1 := list[i1] + t2 := list[i2] + + return t1.SdkPropertyName() < t2.SdkPropertyName() + }) + + // Create a new registry so the pointer uniquely identifies the set of registered types. + return &sdkRegistry{ + list: list, + } +} + +// registeredObjects returns the list of registered instances. +func (r *sdkRegistry) registeredObjects() []sdkRegisterable { + return r.list +} + +// uniqueOnceKey returns a key that uniquely identifies this instance and can be used with +// OncePer.Once +func (r *sdkRegistry) uniqueOnceKey() OnceKey { + // Use the pointer to the registry as the unique key. The pointer is used because it is guaranteed + // to uniquely identify the contained list. The list itself cannot be used as slices are not + // comparable. Using the pointer does mean that two separate registries with identical lists would + // have different keys and so cause whatever information is cached to be created multiple times. + // However, that is not an issue in practice as it should not occur outside tests. Constructing a + // string representation of the list to use instead would avoid that but is an unnecessary + // complication that provides no significant benefit. + return NewCustomOnceKey(r) +} + // SdkMemberTrait represents a trait that members of an sdk module can contribute to the sdk // snapshot. // @@ -417,6 +474,8 @@ type SdkMemberTrait interface { SdkPropertyName() string } +var _ sdkRegisterable = (SdkMemberTrait)(nil) + // SdkMemberTraitBase is the base struct that must be embedded within any type that implements // SdkMemberTrait. type SdkMemberTraitBase struct { @@ -496,56 +555,19 @@ func (s sdkMemberTraitSet) String() string { return fmt.Sprintf("[%s]", strings.Join(list, ",")) } -// SdkMemberTraitsRegistry is a registry of SdkMemberTrait instances. -type SdkMemberTraitsRegistry struct { - // The list of traits sorted by property name. - list []SdkMemberTrait -} - -// copyAndAppend creates a new SdkMemberTraitsRegistry that includes all the traits registered in -// this registry plus the supplied trait. -func (r *SdkMemberTraitsRegistry) copyAndAppend(trait SdkMemberTrait) *SdkMemberTraitsRegistry { - oldList := r.list - - // Copy the slice just in case this is being read while being modified, e.g. when testing. - list := make([]SdkMemberTrait, 0, len(oldList)+1) - list = append(list, oldList...) - list = append(list, trait) - - // Sort the member types by their property name to ensure that registry order has no effect - // on behavior. - sort.Slice(list, func(i1, i2 int) bool { - t1 := list[i1] - t2 := list[i2] - - return t1.SdkPropertyName() < t2.SdkPropertyName() - }) - - // Create a new registry so the pointer uniquely identifies the set of registered types. - return &SdkMemberTraitsRegistry{ - list: list, - } -} - -// RegisteredTraits returns the list of registered SdkMemberTrait instances. -func (r *SdkMemberTraitsRegistry) RegisteredTraits() []SdkMemberTrait { - return r.list -} - -// UniqueOnceKey returns a key to use with Config.Once that uniquely identifies this instance. -func (r *SdkMemberTraitsRegistry) UniqueOnceKey() OnceKey { - // Use the pointer to the registry as the unique key. - return NewCustomOnceKey(r) -} - -var registeredSdkMemberTraits = &SdkMemberTraitsRegistry{} +var registeredSdkMemberTraits = &sdkRegistry{} // RegisteredSdkMemberTraits returns a OnceKey and a sorted list of registered traits. // // The key uniquely identifies the array of traits and can be used with OncePer.Once() to cache // information derived from the array of traits. func RegisteredSdkMemberTraits() (OnceKey, []SdkMemberTrait) { - return registeredSdkMemberTraits.UniqueOnceKey(), registeredSdkMemberTraits.RegisteredTraits() + registerables := registeredSdkMemberTraits.registeredObjects() + traits := make([]SdkMemberTrait, len(registerables)) + for i, registerable := range registerables { + traits[i] = registerable.(SdkMemberTrait) + } + return registeredSdkMemberTraits.uniqueOnceKey(), traits } // RegisterSdkMemberTrait registers an SdkMemberTrait object to allow them to be used in the @@ -724,6 +746,8 @@ type SdkMemberType interface { SupportedTraits() SdkMemberTraitSet } +var _ sdkRegisterable = (SdkMemberType)(nil) + // SdkDependencyContext provides access to information needed by the SdkMemberType.AddDependencies() // implementations. type SdkDependencyContext interface { @@ -783,50 +807,12 @@ func (b *SdkMemberTypeBase) SupportedTraits() SdkMemberTraitSet { return NewSdkMemberTraitSet(b.Traits) } -// SdkMemberTypesRegistry encapsulates the information about registered SdkMemberTypes. -type SdkMemberTypesRegistry struct { - // The list of types sorted by property name. - list []SdkMemberType -} - -func (r *SdkMemberTypesRegistry) copyAndAppend(memberType SdkMemberType) *SdkMemberTypesRegistry { - oldList := r.list - - // Copy the slice just in case this is being read while being modified, e.g. when testing. - list := make([]SdkMemberType, 0, len(oldList)+1) - list = append(list, oldList...) - list = append(list, memberType) - - // Sort the member types by their property name to ensure that registry order has no effect - // on behavior. - sort.Slice(list, func(i1, i2 int) bool { - t1 := list[i1] - t2 := list[i2] - - return t1.SdkPropertyName() < t2.SdkPropertyName() - }) - - // Create a new registry so the pointer uniquely identifies the set of registered types. - return &SdkMemberTypesRegistry{ - list: list, - } -} - -func (r *SdkMemberTypesRegistry) RegisteredTypes() []SdkMemberType { - return r.list -} - -func (r *SdkMemberTypesRegistry) UniqueOnceKey() OnceKey { - // Use the pointer to the registry as the unique key. - return NewCustomOnceKey(r) -} - // registeredModuleExportsMemberTypes is the set of registered SdkMemberTypes for module_exports // modules. -var registeredModuleExportsMemberTypes = &SdkMemberTypesRegistry{} +var registeredModuleExportsMemberTypes = &sdkRegistry{} -// registeredSdkMemberTypes is the set of registered SdkMemberTypes for sdk modules. -var registeredSdkMemberTypes = &SdkMemberTypesRegistry{} +// registeredSdkMemberTypes is the set of registered registeredSdkMemberTypes for sdk modules. +var registeredSdkMemberTypes = &sdkRegistry{} // RegisteredSdkMemberTypes returns a OnceKey and a sorted list of registered types. // @@ -838,14 +824,19 @@ var registeredSdkMemberTypes = &SdkMemberTypesRegistry{} // The key uniquely identifies the array of types and can be used with OncePer.Once() to cache // information derived from the array of types. func RegisteredSdkMemberTypes(moduleExports bool) (OnceKey, []SdkMemberType) { - var registry *SdkMemberTypesRegistry + var registry *sdkRegistry if moduleExports { registry = registeredModuleExportsMemberTypes } else { registry = registeredSdkMemberTypes } - return registry.UniqueOnceKey(), registry.RegisteredTypes() + registerables := registry.registeredObjects() + types := make([]SdkMemberType, len(registerables)) + for i, registerable := range registerables { + types[i] = registerable.(SdkMemberType) + } + return registry.uniqueOnceKey(), types } // RegisterSdkMemberType registers an SdkMemberType object to allow them to be used in the From 581f2e5f79c757d6f351fa57e5e4896002211a10 Mon Sep 17 00:00:00 2001 From: Paul Duffin Date: Wed, 22 Sep 2021 13:25:23 +0100 Subject: [PATCH 2/2] Detect duplicates in sdkRegistry Bug: 195754365 Test: m nothing Change-Id: I67c5022b7cc61891fd6b90365f8271d97d7bcd98 --- android/Android.bp | 1 + android/sdk.go | 19 ++++++++++++++++ android/sdk_test.go | 53 +++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 73 insertions(+) create mode 100644 android/sdk_test.go diff --git a/android/Android.bp b/android/Android.bp index f3a385025..6450a06ca 100644 --- a/android/Android.bp +++ b/android/Android.bp @@ -110,6 +110,7 @@ bootstrap_go_package { "paths_test.go", "prebuilt_test.go", "rule_builder_test.go", + "sdk_test.go", "singleton_module_test.go", "soong_config_modules_test.go", "util_test.go", diff --git a/android/sdk.go b/android/sdk.go index 1518a8731..1d63d7a94 100644 --- a/android/sdk.go +++ b/android/sdk.go @@ -396,6 +396,25 @@ type sdkRegistry struct { func (r *sdkRegistry) copyAndAppend(registerable sdkRegisterable) *sdkRegistry { oldList := r.list + // Make sure that list does not already contain the property. Uses a simple linear search instead + // of a binary search even though the list is sorted. That is because the number of items in the + // list is small and so not worth the overhead of a binary search. + found := false + newPropertyName := registerable.SdkPropertyName() + for _, r := range oldList { + if r.SdkPropertyName() == newPropertyName { + found = true + break + } + } + if found { + names := []string{} + for _, r := range oldList { + names = append(names, r.SdkPropertyName()) + } + panic(fmt.Errorf("duplicate properties found, %q already exists in %q", newPropertyName, names)) + } + // Copy the slice just in case this is being read while being modified, e.g. when testing. list := make([]sdkRegisterable, 0, len(oldList)+1) list = append(list, oldList...) diff --git a/android/sdk_test.go b/android/sdk_test.go new file mode 100644 index 000000000..51aeb314c --- /dev/null +++ b/android/sdk_test.go @@ -0,0 +1,53 @@ +// Copyright (C) 2021 The Android Open Source Project +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package android + +import "testing" + +type testSdkRegisterable struct { + name string +} + +func (t *testSdkRegisterable) SdkPropertyName() string { + return t.name +} + +var _ sdkRegisterable = &testSdkRegisterable{} + +func TestSdkRegistry(t *testing.T) { + alpha := &testSdkRegisterable{"alpha"} + beta := &testSdkRegisterable{"beta"} + betaDup := &testSdkRegisterable{"beta"} + + // Make sure that an empty registry is empty. + emptyRegistry := &sdkRegistry{} + AssertDeepEquals(t, "emptyRegistry should be empty", ([]sdkRegisterable)(nil), emptyRegistry.registeredObjects()) + + // Add beta to the empty registry to create another registry, check that it contains beta and make + // sure that it does not affect the creating registry. + registry1 := emptyRegistry.copyAndAppend(beta) + AssertDeepEquals(t, "emptyRegistry should still be empty", ([]sdkRegisterable)(nil), emptyRegistry.registeredObjects()) + AssertDeepEquals(t, "registry1 should contain beta", []sdkRegisterable{beta}, registry1.registeredObjects()) + + // Add alpha to the registry containing beta to create another registry, check that it contains + // alpha,beta (in order) and make sure that it does not affect the creating registry. + registry2 := registry1.copyAndAppend(alpha) + AssertDeepEquals(t, "registry1 should still contain beta", []sdkRegisterable{beta}, registry1.registeredObjects()) + AssertDeepEquals(t, "registry2 should contain alpha,beta", []sdkRegisterable{alpha, beta}, registry2.registeredObjects()) + + AssertPanicMessageContains(t, "duplicate beta should be detected", `"beta" already exists in ["alpha" "beta"]`, func() { + registry2.copyAndAppend(betaDup) + }) +}