diff --git a/rust/clippy.go b/rust/clippy.go index e1f567d99..6f0ed7ff1 100644 --- a/rust/clippy.go +++ b/rust/clippy.go @@ -19,8 +19,14 @@ import ( ) type ClippyProperties struct { - // whether to run clippy. - Clippy *bool + // name of the lint set that should be used to validate this module. + // + // 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 { - return flags, deps + enabled, lints, err := config.ClippyLintsForDir(ctx.ModuleDir(), c.Properties.Clippy_lints) + 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 diff --git a/rust/clippy_test.go b/rust/clippy_test.go index 314417362..7815aab9d 100644 --- a/rust/clippy_test.go +++ b/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") - fooClippy := ctx.ModuleForTests("libfoo", "android_arm64_armv8-a_dylib").MaybeRule("clippy") - if fooClippy.Rule.String() != "android/soong/rust.clippy" { - t.Errorf("Clippy output (default) for libfoo was not generated: %+v", fooClippy) + bp = bp + GatherRequiredDepsForTest() + + fs := map[string][]byte{ + // 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") - foobarClippy := ctx.ModuleForTests("libfoobar", "android_arm64_armv8-a_dylib").MaybeRule("clippy") - if foobarClippy.Rule != nil { - t.Errorf("Clippy output for libfoobar is not empty") + var clippyLintTests = []struct { + modulePath string + fooFlags string + }{ + {"", "${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"]) + } + + }) } } diff --git a/rust/compiler.go b/rust/compiler.go index c39a4a181..ef7fb8cfc 100644 --- a/rust/compiler.go +++ b/rust/compiler.go @@ -32,8 +32,8 @@ func (compiler *baseCompiler) setNoStdlibs() { compiler.Properties.No_stdlibs = proptools.BoolPtr(true) } -func (compiler *baseCompiler) setNoLint() { - compiler.Properties.No_lint = proptools.BoolPtr(true) +func (compiler *baseCompiler) disableLints() { + 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 - No_lint *bool + // name of the lint set that should be used to validate this module. + // + // 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) { - flags.RustFlags = append(flags.RustFlags, config.AllowAllLints) - } else { - flags.RustFlags = append(flags.RustFlags, config.RustcLintsForDir(ctx.ModuleDir())) + lintFlags, err := config.RustcLintsForDir(ctx.ModuleDir(), compiler.Properties.Lints) + if err != nil { + ctx.PropertyErrorf("lints", err.Error()) } + 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()) diff --git a/rust/compiler_test.go b/rust/compiler_test.go index b85319643..8b9fccc61 100644 --- a/rust/compiler_test.go +++ b/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}") + } + + }) + } +} diff --git a/rust/config/lints.go b/rust/config/lints.go index e24ffac60..06bb668d1 100644 --- a/rust/config/lints.go +++ b/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 -// disable the built-in lints configuration. It is also possible to set -// `clippy` to false to disable the clippy verification. Expect some -// questioning during review if you enable one of these options. For external/ -// code, if you need to use them, it is likely a bug. Otherwise, it may be -// useful to add an exception (that is, move a lint from deny to allow). +// For both categories, there are 3 levels of linting possible: +// - "android", for the strictest lints that applies to all Android platform code. +// - "vendor", for relaxed rules. +// - "none", to disable the linting. +// There is a fourth option ("default") which automatically selects the linting level +// 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: -// - a string for the lints to apply to rustc. -// - a boolean to indicate if clippy should be executed. -// - a string for the lints to apply to clippy. -// The first entry matching will be used. -var DefaultLocalClippyChecks = []PathBasedClippyConfig{ - {"external/", rustcAllowAll, false, noLint}, - {"hardware/", rustcVendor, true, clippyVendor}, - {"prebuilts/", rustcAllowAll, false, noLint}, - {"vendor/google", rustcDefault, true, clippyDefault}, - {"vendor/", rustcVendor, true, clippyVendor}, +// lintConfig defines a set of lints and clippy configuration. +type lintConfig struct { + rustcConfig string // for the lints to apply to rustc. + clippyEnabled bool // to indicate if clippy should be executed. + clippyConfig string // for the lints to apply to clippy. +} + +const ( + androidLints = "android" + vendorLints = "vendor" + 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) { - for _, pathCheck := range DefaultLocalClippyChecks { - if strings.HasPrefix(dir, pathCheck.PathPrefix) { - return pathCheck.ClippyEnabled, pathCheck.ClippyConfig +func ClippyLintsForDir(dir string, clippyLintsProperty *string) (bool, string, error) { + if clippyLintsProperty != nil { + set, ok := lintSets[*clippyLintsProperty] + 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 { - for _, pathCheck := range DefaultLocalClippyChecks { - if strings.HasPrefix(dir, pathCheck.PathPrefix) { - return pathCheck.RustcConfig +func RustcLintsForDir(dir string, lintProperty *string) (string, error) { + if lintProperty != nil { + set, ok := lintSets[*lintProperty] + 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 } diff --git a/rust/rust.go b/rust/rust.go index 54b22853c..b69786976 100644 --- a/rust/rust.go +++ b/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") } } diff --git a/rust/source_provider.go b/rust/source_provider.go index 76679c222..e168718e4 100644 --- a/rust/source_provider.go +++ b/rust/source_provider.go @@ -73,8 +73,8 @@ func NewSourceProviderModule(hod android.HostOrDeviceSupported, sourceProvider S module.compiler = library if !enableLints { - library.setNoLint() - module.setClippy(false) + library.disableLints() + module.disableClippy() } return module