Add hook to be called after defaults have been applied

Previously, the only way for a module type to create modules (other
than defining its own mutator) was to register a LoadHook. However,
that is called before defaults are applied so any properties used in
that hook cannot take advantage of defaults, e.g. java_sdk_library
cannot use defaults to set its sdk_version property and have that
affect its child modules.

This change adds a new SetDefaultableHook() to DefaultableModule to
register a hook that is called after any defaults have been applied.

Also adds some tests to ensure that errors in the visibility property
introduced in a DefaultableHook are reported in the gather phase of
visibility processing.

A follow up change will switch java_sdk_library to use that instead
of the AddLoadHook() mechanism.

Bug: 155295806
Test: m checkapi
Change-Id: I13df3115f9e225f7324b6725eaeb16a78cc2538a

android/defaults.go

@@ -35,6 +35,9 @@ type DefaultableModuleBase struct {
 	defaultsProperties            defaultsProperties
 	defaultableProperties         []interface{}
 	defaultableVariableProperties interface{}
+
+	// The optional hook to call after any defaults have been applied.
+	hook DefaultableHook
 }
 
 func (d *DefaultableModuleBase) defaults() *defaultsProperties {
@@ -46,6 +49,16 @@ func (d *DefaultableModuleBase) setProperties(props []interface{}, variablePrope
 	d.defaultableVariableProperties = variableProperties
 }
 
+func (d *DefaultableModuleBase) SetDefaultableHook(hook DefaultableHook) {
+	d.hook = hook
+}
+
+func (d *DefaultableModuleBase) callHookIfAvailable(ctx DefaultableHookContext) {
+	if d.hook != nil {
+		d.hook(ctx)
+	}
+}
+
 // Interface that must be supported by any module to which defaults can be applied.
 type Defaultable interface {
 	// Get a pointer to the struct containing the Defaults property.
@@ -57,6 +70,15 @@ type Defaultable interface {
 	// Apply defaults from the supplied Defaults to the property structures supplied to
 	// setProperties(...).
 	applyDefaults(TopDownMutatorContext, []Defaults)
+
+	// Set the hook to be called after any defaults have been applied.
+	//
+	// Should be used in preference to a AddLoadHook when the behavior of the load
+	// hook is dependent on properties supplied in the Android.bp file.
+	SetDefaultableHook(hook DefaultableHook)
+
+	// Call the hook if specified.
+	callHookIfAvailable(context DefaultableHookContext)
 }
 
 type DefaultableModule interface {
@@ -75,6 +97,15 @@ func InitDefaultableModule(module DefaultableModule) {
 	module.AddProperties(module.defaults())
 }
 
+// A restricted subset of context methods, similar to LoadHookContext.
+type DefaultableHookContext interface {
+	EarlyModuleContext
+
+	CreateModule(ModuleFactory, ...interface{}) Module
+}
+
+type DefaultableHook func(ctx DefaultableHookContext)
+
 // The Defaults_visibility property.
 type DefaultsVisibilityProperties struct {
 
@@ -268,25 +299,29 @@ func defaultsDepsMutator(ctx BottomUpMutatorContext) {
 }
 
 func defaultsMutator(ctx TopDownMutatorContext) {
-	if defaultable, ok := ctx.Module().(Defaultable); ok && len(defaultable.defaults().Defaults) > 0 {
-		var defaultsList []Defaults
-		seen := make(map[Defaults]bool)
+	if defaultable, ok := ctx.Module().(Defaultable); ok {
+		if len(defaultable.defaults().Defaults) > 0 {
+			var defaultsList []Defaults
+			seen := make(map[Defaults]bool)
 
-		ctx.WalkDeps(func(module, parent Module) bool {
-			if ctx.OtherModuleDependencyTag(module) == DefaultsDepTag {
-				if defaults, ok := module.(Defaults); ok {
-					if !seen[defaults] {
-						seen[defaults] = true
-						defaultsList = append(defaultsList, defaults)
-						return len(defaults.defaults().Defaults) > 0
+			ctx.WalkDeps(func(module, parent Module) bool {
+				if ctx.OtherModuleDependencyTag(module) == DefaultsDepTag {
+					if defaults, ok := module.(Defaults); ok {
+						if !seen[defaults] {
+							seen[defaults] = true
+							defaultsList = append(defaultsList, defaults)
+							return len(defaults.defaults().Defaults) > 0
+						}
+					} else {
+						ctx.PropertyErrorf("defaults", "module %s is not an defaults module",
+							ctx.OtherModuleName(module))
 					}
-				} else {
-					ctx.PropertyErrorf("defaults", "module %s is not an defaults module",
-						ctx.OtherModuleName(module))
 				}
-			}
-			return false
-		})
-		defaultable.applyDefaults(ctx, defaultsList)
+				return false
+			})
+			defaultable.applyDefaults(ctx, defaultsList)
+		}
+
+		defaultable.callHookIfAvailable(ctx)
 	}
 }

android/hooks.go

@@ -39,6 +39,12 @@ type LoadHookContext interface {
 	moduleFactories() map[string]blueprint.ModuleFactory
 }
 
+// Add a hook that will be called once the module has been loaded, i.e. its
+// properties have been initialized from the Android.bp file.
+//
+// Consider using SetDefaultableHook to register a hook for any module that implements
+// DefaultableModule as the hook is called after any defaults have been applied to the
+// module which could reduce duplication and make it easier to use.
 func AddLoadHook(m blueprint.Module, hook func(LoadHookContext)) {
 	blueprint.AddLoadHook(m, func(ctx blueprint.LoadHookContext) {
 		actx := &loadHookContext{

android/mutator.go

@@ -110,10 +110,17 @@ var preArch = []RegisterMutatorFunc{
 	RegisterVisibilityRuleChecker,
 
 	// Apply properties from defaults modules to the referencing modules.
+	//
+	// Any mutators that are added before this will not see any modules created by
+	// a DefaultableHook.
 	RegisterDefaultsPreArchMutators,
 
 	// Create an association between prebuilt modules and their corresponding source
 	// modules (if any).
+	//
+	// Must be run after defaults mutators to ensure that any modules created by
+	// a DefaultableHook can be either a prebuilt or a source module with a matching
+	// prebuilt.
 	RegisterPrebuiltsPreArchMutators,
 
 	// Gather the visibility rules for all modules for us during visibility enforcement.

android/visibility_test.go

@@ -903,6 +903,69 @@ var visibilityTests = []struct {
 				}`),
 		},
 	},
+	{
+		name: "ensure visibility properties are checked for correctness",
+		fs: map[string][]byte{
+			"top/Blueprints": []byte(`
+				mock_parent {
+					name: "parent",
+					visibility: ["//top/nested"],
+					child: {
+						name: "libchild",
+						visibility: ["top/other"],
+					},
+				}`),
+		},
+		expectedErrors: []string{
+			`module "parent": child.visibility: invalid visibility pattern "top/other"`,
+		},
+	},
+	{
+		name: "invalid visibility added to child detected during gather phase",
+		fs: map[string][]byte{
+			"top/Blueprints": []byte(`
+				mock_parent {
+					name: "parent",
+					visibility: ["//top/nested"],
+					child: {
+						name: "libchild",
+						invalid_visibility: ["top/other"],
+					},
+				}`),
+		},
+		expectedErrors: []string{
+			// That this error is reported against the child not the parent shows it was
+			// not being detected in the parent which is correct as invalid_visibility is
+			// purposely not added to the list of visibility properties to check, and was
+			// in fact detected in the child in the gather phase. Contrast this error message
+			// with the preceding one.
+			`module "libchild" \(created by module "parent"\): visibility: invalid visibility pattern "top/other"`,
+		},
+	},
+	{
+		name: "automatic visibility inheritance enabled",
+		fs: map[string][]byte{
+			"top/Blueprints": []byte(`
+				mock_parent {
+					name: "parent",
+					visibility: ["//top/nested"],
+					child: {
+						name: "libchild",
+						visibility: ["//top/other"],
+					},
+				}`),
+			"top/nested/Blueprints": []byte(`
+				mock_library {
+					name: "libnested",
+					deps: ["libchild"],
+				}`),
+			"top/other/Blueprints": []byte(`
+				mock_library {
+					name: "libother",
+					deps: ["libchild"],
+				}`),
+		},
+	},
 }
 
 func TestVisibility(t *testing.T) {
@@ -936,6 +999,7 @@ func testVisibility(buildDir string, fs map[string][]byte) (*TestContext, []erro
 
 	ctx := NewTestArchContext()
 	ctx.RegisterModuleType("mock_library", newMockLibraryModule)
+	ctx.RegisterModuleType("mock_parent", newMockParentFactory)
 	ctx.RegisterModuleType("mock_defaults", defaultsFactory)
 
 	// Order of the following method calls is significant.
@@ -996,3 +1060,42 @@ func defaultsFactory() Module {
 	InitDefaultsModule(m)
 	return m
 }
+
+type mockParentProperties struct {
+	Child struct {
+		Name *string
+
+		// Visibility to pass to the child module.
+		Visibility []string
+
+		// Purposely not validated visibility to pass to the child.
+		Invalid_visibility []string
+	}
+}
+
+type mockParent struct {
+	ModuleBase
+	DefaultableModuleBase
+	properties mockParentProperties
+}
+
+func (p *mockParent) GenerateAndroidBuildActions(ModuleContext) {
+}
+
+func newMockParentFactory() Module {
+	m := &mockParent{}
+	m.AddProperties(&m.properties)
+	InitAndroidArchModule(m, HostAndDeviceSupported, MultilibCommon)
+	InitDefaultableModule(m)
+	AddVisibilityProperty(m, "child.visibility", &m.properties.Child.Visibility)
+
+	m.SetDefaultableHook(func(ctx DefaultableHookContext) {
+		visibility := m.properties.Child.Visibility
+		visibility = append(visibility, m.properties.Child.Invalid_visibility...)
+		ctx.CreateModule(newMockLibraryModule, &struct {
+			Name       *string
+			Visibility []string
+		}{m.properties.Child.Name, visibility})
+	})
+	return m
+}