Merge "Add comments and clarify errors in neverallow"

android/neverallow.go

@@ -15,6 +15,7 @@
 package android
 
 import (
+	"fmt"
 	"path/filepath"
 	"reflect"
 	"regexp"
@@ -372,6 +373,20 @@ type ruleProperty struct {
 	matcher ValueMatcher
 }
 
+func (r *ruleProperty) String() string {
+	return fmt.Sprintf("%q matches: %s", strings.Join(r.fields, "."), r.matcher)
+}
+
+type ruleProperties []ruleProperty
+
+func (r ruleProperties) String() string {
+	var s []string
+	for _, r := range r {
+		s = append(s, r.String())
+	}
+	return strings.Join(s, " ")
+}
+
 // A NeverAllow rule.
 type Rule interface {
 	In(path ...string) Rule
@@ -413,8 +428,8 @@ type rule struct {
 	moduleTypes       []string
 	unlessModuleTypes []string
 
-	props       []ruleProperty
+	props       ruleProperties
-	unlessProps []ruleProperty
+	unlessProps ruleProperties
 
 	onlyBootclasspathJar bool
 }
@@ -424,16 +439,19 @@ func NeverAllow() Rule {
 	return &rule{directDeps: make(map[string]bool)}
 }
 
+// In adds path(s) where this rule applies.
 func (r *rule) In(path ...string) Rule {
 	r.paths = append(r.paths, cleanPaths(path)...)
 	return r
 }
 
+// NotIn adds path(s) to that this rule does not apply to.
 func (r *rule) NotIn(path ...string) Rule {
 	r.unlessPaths = append(r.unlessPaths, cleanPaths(path)...)
 	return r
 }
 
+// InDirectDeps adds dep(s) that are not allowed with this rule.
 func (r *rule) InDirectDeps(deps ...string) Rule {
 	for _, d := range deps {
 		r.directDeps[d] = true
@@ -441,25 +459,30 @@ func (r *rule) InDirectDeps(deps ...string) Rule {
 	return r
 }
 
+// WithOsClass adds osClass(es) that this rule applies to.
 func (r *rule) WithOsClass(osClasses ...OsClass) Rule {
 	r.osClasses = append(r.osClasses, osClasses...)
 	return r
 }
 
+// ModuleType adds type(s) that this rule applies to.
 func (r *rule) ModuleType(types ...string) Rule {
 	r.moduleTypes = append(r.moduleTypes, types...)
 	return r
 }
 
+// NotModuleType adds type(s) that this rule does not apply to..
 func (r *rule) NotModuleType(types ...string) Rule {
 	r.unlessModuleTypes = append(r.unlessModuleTypes, types...)
 	return r
 }
 
+// With specifies property/value combinations that are restricted for this rule.
 func (r *rule) With(properties, value string) Rule {
 	return r.WithMatcher(properties, selectMatcher(value))
 }
 
+// WithMatcher specifies property/matcher combinations that are restricted for this rule.
 func (r *rule) WithMatcher(properties string, matcher ValueMatcher) Rule {
 	r.props = append(r.props, ruleProperty{
 		fields:  fieldNamesForProperties(properties),
@@ -468,10 +491,12 @@ func (r *rule) WithMatcher(properties string, matcher ValueMatcher) Rule {
 	return r
 }
 
+// Without specifies property/value combinations that this rule does not apply to.
 func (r *rule) Without(properties, value string) Rule {
 	return r.WithoutMatcher(properties, selectMatcher(value))
 }
 
+// Without specifies property/matcher combinations that this rule does not apply to.
 func (r *rule) WithoutMatcher(properties string, matcher ValueMatcher) Rule {
 	r.unlessProps = append(r.unlessProps, ruleProperty{
 		fields:  fieldNamesForProperties(properties),
@@ -487,49 +512,54 @@ func selectMatcher(expected string) ValueMatcher {
 	return &equalMatcher{expected: expected}
 }
 
+// Because specifies a reason for this rule.
 func (r *rule) Because(reason string) Rule {
 	r.reason = reason
 	return r
 }
 
+// BootclasspathJar whether this rule only applies to Jars in the Bootclasspath
 func (r *rule) BootclasspathJar() Rule {
 	r.onlyBootclasspathJar = true
 	return r
 }
 
 func (r *rule) String() string {
-	s := "neverallow"
+	s := []string{"neverallow requirements. Not allowed:"}
-	for _, v := range r.paths {
+	if len(r.paths) > 0 {
-		s += " dir:" + v + "*"
+		s = append(s, fmt.Sprintf("in dirs: %q", r.paths))
 	}
-	for _, v := range r.unlessPaths {
+	if len(r.moduleTypes) > 0 {
-		s += " -dir:" + v + "*"
+		s = append(s, fmt.Sprintf("module types: %q", r.moduleTypes))
 	}
-	for _, v := range r.moduleTypes {
+	if len(r.props) > 0 {
-		s += " type:" + v
+		s = append(s, fmt.Sprintf("properties matching: %s", r.props))
 	}
-	for _, v := range r.unlessModuleTypes {
+	if len(r.directDeps) > 0 {
-		s += " -type:" + v
+		s = append(s, fmt.Sprintf("dep(s): %q", SortedStringKeys(r.directDeps)))
 	}
-	for _, v := range r.props {
+	if len(r.osClasses) > 0 {
-		s += " " + strings.Join(v.fields, ".") + v.matcher.String()
+		s = append(s, fmt.Sprintf("os class(es): %q", r.osClasses))
-	}
-	for _, v := range r.unlessProps {
-		s += " -" + strings.Join(v.fields, ".") + v.matcher.String()
-	}
-	for k := range r.directDeps {
-		s += " deps:" + k
-	}
-	for _, v := range r.osClasses {
-		s += " os:" + v.String()
 	}
 	if r.onlyBootclasspathJar {
-		s += " inBcp"
+		s = append(s, "in bootclasspath jar")
+	}
+	if len(r.unlessPaths) > 0 {
+		s = append(s, fmt.Sprintf("EXCEPT in dirs: %q", r.unlessPaths))
+	}
+	if len(r.unlessModuleTypes) > 0 {
+		s = append(s, fmt.Sprintf("EXCEPT module types: %q", r.unlessModuleTypes))
+	}
+	if len(r.unlessProps) > 0 {
+		s = append(s, fmt.Sprintf("EXCEPT properties matching: %q", r.unlessProps))
 	}
 	if len(r.reason) != 0 {
-		s += " which is restricted because " + r.reason
+		s = append(s, " which is restricted because "+r.reason)
 	}
-	return s
+	if len(s) == 1 {
+		s[0] = "neverallow requirements (empty)"
+	}
+	return strings.Join(s, "\n\t")
 }
 
 func (r *rule) appliesToPath(dir string) bool {

android/neverallow_test.go

@@ -15,6 +15,7 @@
 package android
 
 import (
+	"regexp"
 	"testing"
 
 	"github.com/google/blueprint"
@@ -55,7 +56,37 @@ var neverallowTests = []struct {
 				}`),
 		},
 		expectedErrors: []string{
-			`module "libother": violates neverallow deps:not_allowed_in_direct_deps`,
+			regexp.QuoteMeta("module \"libother\": violates neverallow requirements. Not allowed:\n\tdep(s): [\"not_allowed_in_direct_deps\"]"),
+		},
+	},
+	{
+		name: "multiple constraints",
+		rules: []Rule{
+			NeverAllow().
+				InDirectDeps("not_allowed_in_direct_deps").
+				In("other").
+				ModuleType("cc_library").
+				NotIn("top").
+				NotModuleType("cc_binary"),
+		},
+		fs: map[string][]byte{
+			"top/Android.bp": []byte(`
+				cc_library {
+					name: "not_allowed_in_direct_deps",
+				}`),
+			"other/Android.bp": []byte(`
+				cc_library {
+					name: "libother",
+					static_libs: ["not_allowed_in_direct_deps"],
+				}`),
+		},
+		expectedErrors: []string{
+			regexp.QuoteMeta(`module "libother": violates neverallow requirements. Not allowed:
+	in dirs: ["other/"]
+	module types: ["cc_library"]
+	dep(s): ["not_allowed_in_direct_deps"]
+	EXCEPT in dirs: ["top/"]
+	EXCEPT module types: ["cc_binary"]`),
 		},
 	},
 

apex/apex_test.go

@@ -7457,7 +7457,7 @@ func TestApexPermittedPackagesRules(t *testing.T) {
 		},
 		{
 			name:          "Bootclasspath apex jar not satisfying allowed module packages on Q.",
-			expectedError: `module "bcp_lib2" .* which is restricted because jars that are part of the myapex module may only allow these packages: foo.bar with min_sdk < T. Please jarjar or move code around.`,
+			expectedError: `(?s)module "bcp_lib2" .* which is restricted because jars that are part of the myapex module may only allow these packages: foo.bar with min_sdk < T. Please jarjar or move code around.`,
 			bp: `
 				java_library {
 					name: "bcp_lib1",
@@ -7494,7 +7494,7 @@ func TestApexPermittedPackagesRules(t *testing.T) {
 		},
 		{
 			name:          "Bootclasspath apex jar not satisfying allowed module packages on R.",
-			expectedError: `module "bcp_lib2" .* which is restricted because jars that are part of the myapex module may only allow these packages: foo.bar with min_sdk < T. Please jarjar or move code around.`,
+			expectedError: `(?s)module "bcp_lib2" .* which is restricted because jars that are part of the myapex module may only allow these packages: foo.bar with min_sdk < T. Please jarjar or move code around.`,
 			bp: `
 				java_library {
 					name: "bcp_lib1",