Merge changes I101fa6b0,Ic1a9bb1e into rvc-dev

* changes:
  Report visibility errors in both check and gather phases
  Reduce duplication in visibility property management

android/defaults.go

@@ -176,18 +176,18 @@ func InitDefaultsModule(module DefaultsModule) {
 	defaultsVisibility := module.defaultsVisibility()
 	module.AddProperties(&base.nameProperties, defaultsVisibility)
 
-	// The defaults_visibility property controls the visibility of a defaults module.
-	base.primaryVisibilityProperty =
-		newVisibilityProperty("defaults_visibility", &defaultsVisibility.Defaults_visibility)
-
 	// Unlike non-defaults modules the visibility property is not stored in m.base().commonProperties.
-	// Instead it is stored in a separate instance of commonProperties created above so use that.
+	// Instead it is stored in a separate instance of commonProperties created above so clear the
+	// existing list of properties.
+	clearVisibilityProperties(module)
+
+	// The defaults_visibility property controls the visibility of a defaults module so it must be
+	// set as the primary property, which also adds it to the list.
+	setPrimaryVisibilityProperty(module, "defaults_visibility", &defaultsVisibility.Defaults_visibility)
+
 	// The visibility property needs to be checked (but not parsed) by the visibility module during
-	// its checking phase and parsing phase.
-	base.visibilityPropertyInfo = []visibilityProperty{
-		base.primaryVisibilityProperty,
-		newVisibilityProperty("visibility", &commonProperties.Visibility),
-	}
+	// its checking phase and parsing phase so add it to the list as a normal property.
+	AddVisibilityProperty(module, "visibility", &commonProperties.Visibility)
 
 	base.module = module
 }

android/module.go

@@ -607,10 +607,8 @@ func InitAndroidModule(m Module) {
 	base.customizableProperties = m.GetProperties()
 
 	// The default_visibility property needs to be checked and parsed by the visibility module during
-	// its checking and parsing phases.
-	base.primaryVisibilityProperty =
-		newVisibilityProperty("visibility", &base.commonProperties.Visibility)
-	base.visibilityPropertyInfo = []visibilityProperty{base.primaryVisibilityProperty}
+	// its checking and parsing phases so make it the primary visibility property.
+	setPrimaryVisibilityProperty(m, "visibility", &base.commonProperties.Visibility)
 }
 
 func InitAndroidArchModule(m Module, hod HostOrDeviceSupported, defaultMultilib Multilib) {

android/package.go

@@ -101,10 +101,8 @@ func PackageFactory() Module {
 	module.AddProperties(&module.properties)
 
 	// The default_visibility property needs to be checked and parsed by the visibility module during
-	// its checking and parsing phases.
-	module.primaryVisibilityProperty =
-		newVisibilityProperty("default_visibility", &module.properties.Default_visibility)
-	module.visibilityPropertyInfo = []visibilityProperty{module.primaryVisibilityProperty}
+	// its checking and parsing phases so make it the primary visibility property.
+	setPrimaryVisibilityProperty(module, "default_visibility", &module.properties.Default_visibility)
 
 	return module
 }

android/visibility.go

@@ -246,14 +246,8 @@ func checkRules(ctx BaseModuleContext, currentPkg, property string, visibility [
 	}
 
 	for _, v := range visibility {
-		ok, pkg, name := splitRule(v, currentPkg)
+		ok, pkg, name := splitRule(ctx, v, currentPkg, property)
 		if !ok {
-			// Visibility rule is invalid so ignore it. Keep going rather than aborting straight away to
-			// ensure all the rules on this module are checked.
-			ctx.PropertyErrorf(property,
-				"invalid visibility pattern %q must match"+
-					" //<package>:<module>, //<package> or :<module>",
-				v)
 			continue
 		}
 
@@ -301,21 +295,24 @@ func visibilityRuleGatherer(ctx BottomUpMutatorContext) {
 
 	// Parse the visibility rules that control access to the module and store them by id
 	// for use when enforcing the rules.
-	if visibility := m.visibility(); visibility != nil {
-		rule := parseRules(ctx, currentPkg, m.visibility())
-		if rule != nil {
-			moduleToVisibilityRuleMap(ctx.Config()).Store(qualifiedModuleId, rule)
+	primaryProperty := m.base().primaryVisibilityProperty
+	if primaryProperty != nil {
+		if visibility := primaryProperty.getStrings(); visibility != nil {
+			rule := parseRules(ctx, currentPkg, primaryProperty.getName(), visibility)
+			if rule != nil {
+				moduleToVisibilityRuleMap(ctx.Config()).Store(qualifiedModuleId, rule)
+			}
 		}
 	}
 }
 
-func parseRules(ctx BaseModuleContext, currentPkg string, visibility []string) compositeRule {
+func parseRules(ctx BaseModuleContext, currentPkg, property string, visibility []string) compositeRule {
 	rules := make(compositeRule, 0, len(visibility))
 	hasPrivateRule := false
 	hasPublicRule := false
 	hasNonPrivateRule := false
 	for _, v := range visibility {
-		ok, pkg, name := splitRule(v, currentPkg)
+		ok, pkg, name := splitRule(ctx, v, currentPkg, property)
 		if !ok {
 			continue
 		}
@@ -376,10 +373,16 @@ func isAllowedFromOutsideVendor(pkg string, name string) bool {
 	return !isAncestor("vendor", pkg)
 }
 
-func splitRule(ruleExpression string, currentPkg string) (bool, string, string) {
+func splitRule(ctx BaseModuleContext, ruleExpression string, currentPkg, property string) (bool, string, string) {
 	// Make sure that the rule is of the correct format.
 	matches := visibilityRuleRegexp.FindStringSubmatch(ruleExpression)
 	if ruleExpression == "" || matches == nil {
+		// Visibility rule is invalid so ignore it. Keep going rather than aborting straight away to
+		// ensure all the rules on this module are checked.
+		ctx.PropertyErrorf(property,
+			"invalid visibility pattern %q must match"+
+				" //<package>:<module>, //<package> or :<module>",
+			ruleExpression)
 		return false, "", ""
 	}
 
@@ -480,3 +483,28 @@ func EffectiveVisibilityRules(ctx BaseModuleContext, module Module) []string {
 
 	return rule.Strings()
 }
+
+// Clear the default visibility properties so they can be replaced.
+func clearVisibilityProperties(module Module) {
+	module.base().visibilityPropertyInfo = nil
+}
+
+// Add a property that contains visibility rules so that they are checked for
+// correctness.
+func AddVisibilityProperty(module Module, name string, stringsProperty *[]string) {
+	addVisibilityProperty(module, name, stringsProperty)
+}
+
+func addVisibilityProperty(module Module, name string, stringsProperty *[]string) visibilityProperty {
+	base := module.base()
+	property := newVisibilityProperty(name, stringsProperty)
+	base.visibilityPropertyInfo = append(base.visibilityPropertyInfo, property)
+	return property
+}
+
+// Set the primary visibility property.
+//
+// Also adds the property to the list of properties to be validated.
+func setPrimaryVisibilityProperty(module Module, name string, stringsProperty *[]string) {
+	module.base().primaryVisibilityProperty = addVisibilityProperty(module, name, stringsProperty)
+}