rust: modify linting properties

Move the linting properties to an enum with 4 possible options:
"default", "android", "vendor" or "none". The previous logic for
default, based on the module's location, is kept. It is now possible to
force the upgrade to a certain lint level for some modules (e.g.
external/[...]/android). Update the unit tests and documentation.

Bug: 163400111
Test: m
Change-Id: I8e464b04401158ed2d3c518a9b72f145a9835c99

rust/clippy.go

@@ -19,8 +19,14 @@ import (
 )
 
 type ClippyProperties struct {
-	// whether to run clippy.
+	// name of the lint set that should be used to validate this module.
-	Clippy *bool
+	//
+	// Possible values are "default" (for using a sensible set of lints
+	// depending on the module's location), "android" (for the strictest
+	// lint set that applies to all Android platform code), "vendor" (for a
+	// relaxed set) and "none" (to disable the execution of clippy).  The
+	// default value is "default". See also the `lints` property.
+	Clippy_lints *string
 }
 
 type clippy struct {
@@ -32,10 +38,10 @@ func (c *clippy) props() []interface{} {
 }
 
 func (c *clippy) flags(ctx ModuleContext, flags Flags, deps PathDeps) (Flags, PathDeps) {
-	if c.Properties.Clippy != nil && !*c.Properties.Clippy {
+	enabled, lints, err := config.ClippyLintsForDir(ctx.ModuleDir(), c.Properties.Clippy_lints)
-		return flags, deps
+	if err != nil {
+		ctx.PropertyErrorf("clippy_lints", err.Error())
 	}
-	enabled, lints := config.ClippyLintsForDir(ctx.ModuleDir())
 	flags.Clippy = enabled
 	flags.ClippyFlags = append(flags.ClippyFlags, lints)
 	return flags, deps

rust/clippy_test.go

@@ -16,31 +16,77 @@ package rust
 
 import (
 	"testing"
+
+	"android/soong/android"
 )
 
 func TestClippy(t *testing.T) {
-	ctx := testRust(t, `
+
+	bp := `
+		// foo uses the default value of clippy_lints
 		rust_library {
 			name: "libfoo",
 			srcs: ["foo.rs"],
 			crate_name: "foo",
 		}
+		// bar forces the use of the "android" lint set
+		rust_library {
+			name: "libbar",
+			srcs: ["foo.rs"],
+			crate_name: "bar",
+			clippy_lints: "android",
+		}
+		// foobar explicitly disable clippy
 		rust_library {
 			name: "libfoobar",
 			srcs: ["foo.rs"],
 			crate_name: "foobar",
-			clippy: false,
+			clippy_lints: "none",
-		}`)
+		}`
 
-	ctx.ModuleForTests("libfoo", "android_arm64_armv8-a_dylib").Output("libfoo.dylib.so")
+	bp = bp + GatherRequiredDepsForTest()
-	fooClippy := ctx.ModuleForTests("libfoo", "android_arm64_armv8-a_dylib").MaybeRule("clippy")
+
-	if fooClippy.Rule.String() != "android/soong/rust.clippy" {
+	fs := map[string][]byte{
-		t.Errorf("Clippy output (default) for libfoo was not generated: %+v", fooClippy)
+		// Reuse the same blueprint file for subdirectories.
+		"external/Android.bp": []byte(bp),
+		"hardware/Android.bp": []byte(bp),
 	}
 
-	ctx.ModuleForTests("libfoobar", "android_arm64_armv8-a_dylib").Output("libfoobar.dylib.so")
+	var clippyLintTests = []struct {
-	foobarClippy := ctx.ModuleForTests("libfoobar", "android_arm64_armv8-a_dylib").MaybeRule("clippy")
+		modulePath string
-	if foobarClippy.Rule != nil {
+		fooFlags   string
-		t.Errorf("Clippy output for libfoobar is not empty")
+	}{
+		{"", "${config.ClippyDefaultLints}"},
+		{"external/", ""},
+		{"hardware/", "${config.ClippyVendorLints}"},
+	}
+
+	for _, tc := range clippyLintTests {
+		t.Run("path="+tc.modulePath, func(t *testing.T) {
+
+			config := android.TestArchConfig(buildDir, nil, bp, fs)
+			ctx := CreateTestContext()
+			ctx.Register(config)
+			_, errs := ctx.ParseFileList(".", []string{tc.modulePath + "Android.bp"})
+			android.FailIfErrored(t, errs)
+			_, errs = ctx.PrepareBuildActions(config)
+			android.FailIfErrored(t, errs)
+
+			r := ctx.ModuleForTests("libfoo", "android_arm64_armv8-a_dylib").MaybeRule("clippy")
+			if r.Args["clippyFlags"] != tc.fooFlags {
+				t.Errorf("Incorrect flags for libfoo: %q, want %q", r.Args["clippyFlags"], tc.fooFlags)
+			}
+
+			r = ctx.ModuleForTests("libbar", "android_arm64_armv8-a_dylib").MaybeRule("clippy")
+			if r.Args["clippyFlags"] != "${config.ClippyDefaultLints}" {
+				t.Errorf("Incorrect flags for libbar: %q, want %q", r.Args["clippyFlags"], "${config.ClippyDefaultLints}")
+			}
+
+			r = ctx.ModuleForTests("libfoobar", "android_arm64_armv8-a_dylib").MaybeRule("clippy")
+			if r.Rule != nil {
+				t.Errorf("libfoobar is setup to use clippy when explicitly disabled: clippyFlags=%q", r.Args["clippyFlags"])
+			}
+
+		})
 	}
 }

rust/compiler.go

@@ -32,8 +32,8 @@ func (compiler *baseCompiler) setNoStdlibs() {
 	compiler.Properties.No_stdlibs = proptools.BoolPtr(true)
 }
 
-func (compiler *baseCompiler) setNoLint() {
+func (compiler *baseCompiler) disableLints() {
-	compiler.Properties.No_lint = proptools.BoolPtr(true)
+	compiler.Properties.Lints = proptools.StringPtr("none")
 }
 
 func NewBaseCompiler(dir, dir64 string, location installLocation) *baseCompiler {
@@ -58,8 +58,14 @@ type BaseCompilerProperties struct {
 	// path to the source file that is the main entry point of the program (e.g. main.rs or lib.rs)
 	Srcs []string `android:"path,arch_variant"`
 
-	// whether to suppress the standard lint flags - default to false
+	// name of the lint set that should be used to validate this module.
-	No_lint *bool
+	//
+	// Possible values are "default" (for using a sensible set of lints
+	// depending on the module's location), "android" (for the strictest
+	// lint set that applies to all Android platform code), "vendor" (for
+	// a relaxed set) and "none" (for ignoring all lint warnings and
+	// errors). The default value is "default".
+	Lints *string
 
 	// flags to pass to rustc
 	Flags []string `android:"path,arch_variant"`
@@ -159,11 +165,11 @@ func (compiler *baseCompiler) featuresToFlags(features []string) []string {
 
 func (compiler *baseCompiler) compilerFlags(ctx ModuleContext, flags Flags) Flags {
 
-	if Bool(compiler.Properties.No_lint) {
+	lintFlags, err := config.RustcLintsForDir(ctx.ModuleDir(), compiler.Properties.Lints)
-		flags.RustFlags = append(flags.RustFlags, config.AllowAllLints)
+	if err != nil {
-	} else {
+		ctx.PropertyErrorf("lints", err.Error())
-		flags.RustFlags = append(flags.RustFlags, config.RustcLintsForDir(ctx.ModuleDir()))
 	}
+	flags.RustFlags = append(flags.RustFlags, lintFlags)
 	flags.RustFlags = append(flags.RustFlags, compiler.Properties.Flags...)
 	flags.RustFlags = append(flags.RustFlags, compiler.featuresToFlags(compiler.Properties.Features)...)
 	flags.RustFlags = append(flags.RustFlags, "--edition="+compiler.edition())

rust/compiler_test.go

@@ -17,6 +17,8 @@ package rust
 import (
 	"strings"
 	"testing"
+
+	"android/soong/android"
 )
 
 // Test that feature flags are being correctly generated.
@@ -104,3 +106,74 @@ func TestInstallDir(t *testing.T) {
 		t.Fatalf("unexpected install path for binary: %#v", install_path_bin)
 	}
 }
+
+func TestLints(t *testing.T) {
+
+	bp := `
+		// foo uses the default value of lints
+		rust_library {
+			name: "libfoo",
+			srcs: ["foo.rs"],
+			crate_name: "foo",
+		}
+		// bar forces the use of the "android" lint set
+		rust_library {
+			name: "libbar",
+			srcs: ["foo.rs"],
+			crate_name: "bar",
+			lints: "android",
+		}
+		// foobar explicitly disable all lints
+		rust_library {
+			name: "libfoobar",
+			srcs: ["foo.rs"],
+			crate_name: "foobar",
+			lints: "none",
+		}`
+
+	bp = bp + GatherRequiredDepsForTest()
+
+	fs := map[string][]byte{
+		// Reuse the same blueprint file for subdirectories.
+		"external/Android.bp": []byte(bp),
+		"hardware/Android.bp": []byte(bp),
+	}
+
+	var lintTests = []struct {
+		modulePath string
+		fooFlags   string
+	}{
+		{"", "${config.RustDefaultLints}"},
+		{"external/", "${config.RustAllowAllLints}"},
+		{"hardware/", "${config.RustVendorLints}"},
+	}
+
+	for _, tc := range lintTests {
+		t.Run("path="+tc.modulePath, func(t *testing.T) {
+
+			config := android.TestArchConfig(buildDir, nil, bp, fs)
+			ctx := CreateTestContext()
+			ctx.Register(config)
+			_, errs := ctx.ParseFileList(".", []string{tc.modulePath + "Android.bp"})
+			android.FailIfErrored(t, errs)
+			_, errs = ctx.PrepareBuildActions(config)
+			android.FailIfErrored(t, errs)
+
+			r := ctx.ModuleForTests("libfoo", "android_arm64_armv8-a_dylib").MaybeRule("rustc")
+			if !strings.Contains(r.Args["rustcFlags"], tc.fooFlags) {
+				t.Errorf("Incorrect flags for libfoo: %q, want %q", r.Args["rustcFlags"], tc.fooFlags)
+			}
+
+			r = ctx.ModuleForTests("libbar", "android_arm64_armv8-a_dylib").MaybeRule("rustc")
+			if !strings.Contains(r.Args["rustcFlags"], "${config.RustDefaultLints}") {
+				t.Errorf("Incorrect flags for libbar: %q, want %q", r.Args["rustcFlags"], "${config.RustDefaultLints}")
+			}
+
+			r = ctx.ModuleForTests("libfoobar", "android_arm64_armv8-a_dylib").MaybeRule("rustc")
+			if !strings.Contains(r.Args["rustcFlags"], "${config.RustAllowAllLints}") {
+				t.Errorf("Incorrect flags for libfoobar: %q, want %q", r.Args["rustcFlags"], "${config.RustAllowAllLints}")
+			}
+
+		})
+	}
+}

rust/config/lints.go

@@ -15,6 +15,7 @@
 package config
 
 import (
+	"fmt"
 	"strings"
 
 	"android/soong/android"
@@ -24,18 +25,21 @@ import (
 // The Android build system tries to avoid reporting warnings during the build.
 // Therefore, by default, we upgrade warnings to denials. For some of these
 // lints, an allow exception is setup, using the variables below.
-// There are different default lints depending on the repository location (see
+//
-// DefaultLocalClippyChecks).
 // The lints are split into two categories. The first one contains the built-in
 // lints (https://doc.rust-lang.org/rustc/lints/index.html). The second is
 // specific to Clippy lints (https://rust-lang.github.io/rust-clippy/master/).
 //
-// When developing a module, it is possible to use the `no_lint` property to
+// For both categories, there are 3 levels of linting possible:
-// disable the built-in lints configuration. It is also possible to set
+// - "android", for the strictest lints that applies to all Android platform code.
-// `clippy` to false to disable the clippy verification. Expect some
+// - "vendor", for relaxed rules.
-// questioning during review if you enable one of these options. For external/
+// - "none", to disable the linting.
-// code, if you need to use them, it is likely a bug. Otherwise, it may be
+// There is a fourth option ("default") which automatically selects the linting level
-// useful to add an exception (that is, move a lint from deny to allow).
+// based on the module's location. See defaultLintSetForPath.
+//
+// When developing a module, you may set `lints = "none"` and `clippy_lints =
+// "none"` to disable all the linting. Expect some questioning during code review
+// if you enable one of these options.
 var (
 	// Default Rust lints that applies to Google-authored modules.
 	defaultRustcLints = []string{
@@ -102,13 +106,6 @@ func init() {
 	pctx.StaticVariable("RustAllowAllLints", strings.Join(allowAllLints, " "))
 }
 
-type PathBasedClippyConfig struct {
-	PathPrefix    string
-	RustcConfig   string
-	ClippyEnabled bool
-	ClippyConfig  string
-}
-
 const noLint = ""
 const rustcDefault = "${config.RustDefaultLints}"
 const rustcVendor = "${config.RustVendorLints}"
@@ -116,36 +113,78 @@ const rustcAllowAll = "${config.RustAllowAllLints}"
 const clippyDefault = "${config.ClippyDefaultLints}"
 const clippyVendor = "${config.ClippyVendorLints}"
 
-// This is a map of local path prefixes to a set of parameters for the linting:
+// lintConfig defines a set of lints and clippy configuration.
-// - a string for the lints to apply to rustc.
+type lintConfig struct {
-// - a boolean to indicate if clippy should be executed.
+	rustcConfig   string // for the lints to apply to rustc.
-// - a string for the lints to apply to clippy.
+	clippyEnabled bool   // to indicate if clippy should be executed.
-// The first entry matching will be used.
+	clippyConfig  string // for the lints to apply to clippy.
-var DefaultLocalClippyChecks = []PathBasedClippyConfig{
+}
-	{"external/", rustcAllowAll, false, noLint},
+
-	{"hardware/", rustcVendor, true, clippyVendor},
+const (
-	{"prebuilts/", rustcAllowAll, false, noLint},
+	androidLints = "android"
-	{"vendor/google", rustcDefault, true, clippyDefault},
+	vendorLints  = "vendor"
-	{"vendor/", rustcVendor, true, clippyVendor},
+	noneLints    = "none"
+)
+
+// lintSets defines the categories of linting for Android and their mapping to lintConfigs.
+var lintSets = map[string]lintConfig{
+	androidLints: {rustcDefault, true, clippyDefault},
+	vendorLints:  {rustcVendor, true, clippyVendor},
+	noneLints:    {rustcAllowAll, false, noLint},
+}
+
+type pathLintSet struct {
+	prefix string
+	set    string
+}
+
+// This is a map of local path prefixes to a lint set.  The first entry
+// matching will be used. If no entry matches, androidLints ("android") will be
+// used.
+var defaultLintSetForPath = []pathLintSet{
+	{"external", noneLints},
+	{"hardware", vendorLints},
+	{"prebuilts", noneLints},
+	{"vendor/google", androidLints},
+	{"vendor", vendorLints},
 }
-var AllowAllLints = rustcAllowAll
 
 // ClippyLintsForDir returns a boolean if Clippy should be executed and if so, the lints to be used.
-func ClippyLintsForDir(dir string) (bool, string) {
+func ClippyLintsForDir(dir string, clippyLintsProperty *string) (bool, string, error) {
-	for _, pathCheck := range DefaultLocalClippyChecks {
+	if clippyLintsProperty != nil {
-		if strings.HasPrefix(dir, pathCheck.PathPrefix) {
+		set, ok := lintSets[*clippyLintsProperty]
-			return pathCheck.ClippyEnabled, pathCheck.ClippyConfig
+		if ok {
+			return set.clippyEnabled, set.clippyConfig, nil
+		}
+		if *clippyLintsProperty != "default" {
+			return false, "", fmt.Errorf("unknown value for `clippy_lints`: %v, valid options are: default, android, vendor or none", *clippyLintsProperty)
 		}
 	}
-	return true, clippyDefault
+	for _, p := range defaultLintSetForPath {
+		if strings.HasPrefix(dir, p.prefix) {
+			setConfig := lintSets[p.set]
+			return setConfig.clippyEnabled, setConfig.clippyConfig, nil
+		}
+	}
+	return true, clippyDefault, nil
 }
 
 // RustcLintsForDir returns the standard lints to be used for a repository.
-func RustcLintsForDir(dir string) string {
+func RustcLintsForDir(dir string, lintProperty *string) (string, error) {
-	for _, pathCheck := range DefaultLocalClippyChecks {
+	if lintProperty != nil {
-		if strings.HasPrefix(dir, pathCheck.PathPrefix) {
+		set, ok := lintSets[*lintProperty]
-			return pathCheck.RustcConfig
+		if ok {
+			return set.rustcConfig, nil
+		}
+		if *lintProperty != "default" {
+			return "", fmt.Errorf("unknown value for `lints`: %v, valid options are: default, android, vendor or none", *lintProperty)
+		}
+
+	}
+	for _, p := range defaultLintSetForPath {
+		if strings.HasPrefix(dir, p.prefix) {
+			return lintSets[p.set].rustcConfig, nil
 		}
 	}
-	return rustcDefault
+	return rustcDefault, nil
 }

rust/rust.go

@@ -1046,9 +1046,9 @@ func (mod *Module) Name() string {
 	return name
 }
 
-func (mod *Module) setClippy(clippy bool) {
+func (mod *Module) disableClippy() {
 	if mod.clippy != nil {
-		mod.clippy.Properties.Clippy = proptools.BoolPtr(clippy)
+		mod.clippy.Properties.Clippy_lints = proptools.StringPtr("none")
 	}
 }
 

rust/source_provider.go

@@ -73,8 +73,8 @@ func NewSourceProviderModule(hod android.HostOrDeviceSupported, sourceProvider S
 	module.compiler = library
 
 	if !enableLints {
-		library.setNoLint()
+		library.disableLints()
-		module.setClippy(false)
+		module.disableClippy()
 	}
 
 	return module