Simplify visibility rules that include //visibility:public

While it is invalid to mix //visibility:public with other rules in the
visibility property in a .bp file tt was possible, by overriding
defaults, to have //visibility:public mixed in with other rules in the
effective visibility rules. That caused problems when those effective
rules were used in an sdk snapshot.

This change replaces any set of rules that include //visibility:public
with just the //visibility:public rule. That simplifies those rules,
making them cheaper to process and ensures that the effective rules are
valid in the visibility property.

Adding test support required some refactoring of the
effectiveVisibilityRules(BaseModuleContext, ...) and underlying methods
to take a Config instead of BaseModuleContext as the tests do not have
access to BaseModuleContext.

Bug: 142935992
Test: m nothing - new tests failed without change, work with it
      Add dex2oat to art-module-host-exports, build it and check the
      generated Android.bp file in the snapshot to ensure the
	  visibility property for the dex2oat prebuilt does not mix
	  //visibility:public with other rules.
Change-Id: I08e7f0dcb40838d426fe88fedf69eae27b77473c

android/visibility.go

@@ -186,8 +186,8 @@ func (r privateRule) String() string {
 var visibilityRuleMap = NewOnceKey("visibilityRuleMap")
 
 // The map from qualifiedModuleName to visibilityRule.
-func moduleToVisibilityRuleMap(ctx BaseModuleContext) *sync.Map {
+func moduleToVisibilityRuleMap(config Config) *sync.Map {
-	return ctx.Config().Once(visibilityRuleMap, func() interface{} {
+	return config.Once(visibilityRuleMap, func() interface{} {
 		return &sync.Map{}
 	}).(*sync.Map)
 }
@@ -304,7 +304,7 @@ func visibilityRuleGatherer(ctx BottomUpMutatorContext) {
 	if visibility := m.visibility(); visibility != nil {
 		rule := parseRules(ctx, currentPkg, m.visibility())
 		if rule != nil {
-			moduleToVisibilityRuleMap(ctx).Store(qualifiedModuleId, rule)
+			moduleToVisibilityRuleMap(ctx.Config()).Store(qualifiedModuleId, rule)
 		}
 	}
 }
@@ -312,6 +312,7 @@ func visibilityRuleGatherer(ctx BottomUpMutatorContext) {
 func parseRules(ctx BaseModuleContext, currentPkg 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)
@@ -328,6 +329,7 @@ func parseRules(ctx BaseModuleContext, currentPkg string, visibility []string) c
 				isPrivateRule = true
 			case "public":
 				r = publicRule{}
+				hasPublicRule = true
 			}
 		} else {
 			switch name {
@@ -355,6 +357,11 @@ func parseRules(ctx BaseModuleContext, currentPkg string, visibility []string) c
 		return compositeRule{privateRule{}}
 	}
 
+	if hasPublicRule {
+		// Public overrides all other rules so just return it.
+		return compositeRule{publicRule{}}
+	}
+
 	return rules
 }
 
@@ -415,21 +422,21 @@ func visibilityRuleEnforcer(ctx TopDownMutatorContext) {
 			return
 		}
 
-		rule := effectiveVisibilityRules(ctx, depQualified)
+		rule := effectiveVisibilityRules(ctx.Config(), depQualified)
 		if rule != nil && !rule.matches(qualified) {
 			ctx.ModuleErrorf("depends on %s which is not visible to this module", depQualified)
 		}
 	})
 }
 
-func effectiveVisibilityRules(ctx BaseModuleContext, qualified qualifiedModuleName) compositeRule {
+func effectiveVisibilityRules(config Config, qualified qualifiedModuleName) compositeRule {
-	moduleToVisibilityRule := moduleToVisibilityRuleMap(ctx)
+	moduleToVisibilityRule := moduleToVisibilityRuleMap(config)
 	value, ok := moduleToVisibilityRule.Load(qualified)
 	var rule compositeRule
 	if ok {
 		rule = value.(compositeRule)
 	} else {
-		rule = packageDefaultVisibility(ctx, qualified)
+		rule = packageDefaultVisibility(config, qualified)
 	}
 	return rule
 }
@@ -441,8 +448,8 @@ func createQualifiedModuleName(ctx BaseModuleContext) qualifiedModuleName {
 	return qualified
 }
 
-func packageDefaultVisibility(ctx BaseModuleContext, moduleId qualifiedModuleName) compositeRule {
+func packageDefaultVisibility(config Config, moduleId qualifiedModuleName) compositeRule {
-	moduleToVisibilityRule := moduleToVisibilityRuleMap(ctx)
+	moduleToVisibilityRule := moduleToVisibilityRuleMap(config)
 	packageQualifiedId := moduleId.getContainingPackageId()
 	for {
 		value, ok := moduleToVisibilityRule.Load(packageQualifiedId)
@@ -469,7 +476,7 @@ func EffectiveVisibilityRules(ctx BaseModuleContext, module Module) []string {
 	dir := ctx.OtherModuleDir(module)
 	qualified := qualifiedModuleName{dir, moduleName}
 
-	rule := effectiveVisibilityRules(ctx, qualified)
+	rule := effectiveVisibilityRules(ctx.Config(), qualified)
 
 	return rule.Strings()
 }

android/visibility_test.go

@@ -1,15 +1,17 @@
 package android
 
 import (
+	"reflect"
 	"testing"
 
 	"github.com/google/blueprint"
 )
 
 var visibilityTests = []struct {
-	name           string
+	name                string
-	fs             map[string][]byte
+	fs                  map[string][]byte
-	expectedErrors []string
+	expectedErrors      []string
+	effectiveVisibility map[qualifiedModuleName][]string
 }{
 	{
 		name: "invalid visibility: empty list",
@@ -493,6 +495,9 @@ var visibilityTests = []struct {
 					deps: ["libexample"],
 				}`),
 		},
+		effectiveVisibility: map[qualifiedModuleName][]string{
+			qualifiedModuleName{pkg: "top", name: "libexample"}: {"//visibility:public"},
+		},
 	},
 	{
 		name: "//visibility:public mixed with other from different defaults 1",
@@ -903,13 +908,27 @@ var visibilityTests = []struct {
 func TestVisibility(t *testing.T) {
 	for _, test := range visibilityTests {
 		t.Run(test.name, func(t *testing.T) {
-			_, errs := testVisibility(buildDir, test.fs)
+			ctx, errs := testVisibility(buildDir, test.fs)
 
 			CheckErrorsAgainstExpectations(t, errs, test.expectedErrors)
+
+			if test.effectiveVisibility != nil {
+				checkEffectiveVisibility(t, ctx, test.effectiveVisibility)
+			}
 		})
 	}
 }
 
+func checkEffectiveVisibility(t *testing.T, ctx *TestContext, effectiveVisibility map[qualifiedModuleName][]string) {
+	for moduleName, expectedRules := range effectiveVisibility {
+		rule := effectiveVisibilityRules(ctx.config, moduleName)
+		stringRules := rule.Strings()
+		if !reflect.DeepEqual(expectedRules, stringRules) {
+			t.Errorf("effective rules mismatch: expected %q, found %q", expectedRules, stringRules)
+		}
+	}
+}
+
 func testVisibility(buildDir string, fs map[string][]byte) (*TestContext, []error) {
 
 	// Create a new config per test as visibility information is stored in the config.

sdk/sdk_test.go

@@ -111,8 +111,14 @@ func TestSnapshotVisibility(t *testing.T) {
 			sdk_version: "none",
 		}
 
+		java_defaults {
+			name: "java-defaults",
+			visibility: ["//other/bar"], 
+		}
+
 		java_library {
 			name: "mypublicjavalib",
+			defaults: ["java-defaults"],
       visibility: ["//visibility:public"],
 			srcs: ["Test.java"],
 			system_modules: "none",