Disallow -warnings-as-errors in tidy_flags am: 2d481842b1 am: f3a8ef4231

Original change: https://android-review.googlesource.com/c/platform/build/soong/+/2080669

Change-Id: I918c00e10ed94af1fefdc292464534be7cfd5faf
Signed-off-by: Automerger Merge Worker <android-build-automerger-merge-worker@system.gserviceaccount.com>

cc/tidy.go

@@ -188,31 +188,14 @@ func (tidy *tidyFeature) flags(ctx ModuleContext, flags Flags) Flags {
 	tidyChecks = tidyChecks + ",-cert-err33-c"
 	flags.TidyFlags = append(flags.TidyFlags, tidyChecks)
 
-	if ctx.Config().IsEnvTrue("WITH_TIDY") {
-		// WITH_TIDY=1 enables clang-tidy globally. There could be many unexpected
-		// warnings from new checks and many local tidy_checks_as_errors and
-		// -warnings-as-errors can break a global build.
-		// So allow all clang-tidy warnings.
-		inserted := false
-		for i, s := range flags.TidyFlags {
+	// Embedding -warnings-as-errors in tidy_flags is error-prone.
+	// It should be replaced with the tidy_checks_as_errors list.
+	for _, s := range flags.TidyFlags {
 		if strings.Contains(s, "-warnings-as-errors=") {
-				// clang-tidy accepts only one -warnings-as-errors
-				// replace the old one
-				re := regexp.MustCompile(`'?-?-warnings-as-errors=[^ ]* *`)
-				newFlag := re.ReplaceAllString(s, "")
-				if newFlag == "" {
-					flags.TidyFlags[i] = "-warnings-as-errors=-*"
-				} else {
-					flags.TidyFlags[i] = newFlag + " -warnings-as-errors=-*"
-				}
-				inserted = true
-				break
+			ctx.PropertyErrorf("tidy_flags", "should not contain -warnings-as-errors, use tidy_checks_as_errors instead")
 		}
 	}
-		if !inserted {
-			flags.TidyFlags = append(flags.TidyFlags, "-warnings-as-errors=-*")
-		}
-	} else if len(tidy.Properties.Tidy_checks_as_errors) > 0 {
+	if len(tidy.Properties.Tidy_checks_as_errors) > 0 {
 		tidyChecksAsErrors := "-warnings-as-errors=" + strings.Join(esc(ctx, "tidy_checks_as_errors", tidy.Properties.Tidy_checks_as_errors), ",")
 		flags.TidyFlags = append(flags.TidyFlags, tidyChecksAsErrors)
 	}

cc/tidy_test.go

@@ -22,6 +22,76 @@ import (
 	"android/soong/android"
 )
 
+func TestTidyFlagsWarningsAsErrors(t *testing.T) {
+	// The "tidy_flags" property should not contain -warnings-as-errors.
+	testCases := []struct {
+		libName, bp string
+		errorMsg    string   // a negative test; must have error message
+		flags       []string // must have substrings in tidyFlags
+		noFlags     []string // must not have substrings in tidyFlags
+	}{
+		{
+			"libfoo1",
+			`cc_library_shared { // no warnings-as-errors, good tidy_flags
+			  name: "libfoo1",
+			  srcs: ["foo.c"],
+              tidy_flags: ["-header-filter=dir1/"],
+		    }`,
+			"",
+			[]string{"-header-filter=dir1/"},
+			[]string{"-warnings-as-errors"},
+		},
+		{
+			"libfoo2",
+			`cc_library_shared { // good use of tidy_checks_as_errors
+			  name: "libfoo2",
+			  srcs: ["foo.c"],
+			  tidy_checks_as_errors: ["xyz-*", "abc"],
+              tidy_flags: ["-header-filter=dir2/"],
+		    }`,
+			"",
+			[]string{"-header-filter=dir2/", "-warnings-as-errors='xyz-*',abc"},
+			[]string{},
+		},
+		{
+			"libfoo3",
+			`cc_library_shared { // bad use of -warnings-as-errors in tidy_flags
+			  name: "libfoo3",
+			  srcs: ["foo.c"],
+              tidy_flags: [
+                "-header-filters=.*",
+			    "-warnings-as-errors=xyz-*",
+              ],
+		    }`,
+			`module "libfoo3" .*: tidy_flags: should not contain -warnings-as-errors,` +
+				` use tidy_checks_as_errors instead`,
+			[]string{},
+			[]string{},
+		},
+	}
+	for _, test := range testCases {
+		if test.errorMsg != "" {
+			testCcError(t, test.errorMsg, test.bp)
+			continue
+		}
+		variant := "android_arm64_armv8-a_shared"
+		ctx := testCc(t, test.bp)
+		t.Run("caseTidyFlags", func(t *testing.T) {
+			flags := ctx.ModuleForTests(test.libName, variant).Rule("clangTidy").Args["tidyFlags"]
+			for _, flag := range test.flags {
+				if !strings.Contains(flags, flag) {
+					t.Errorf("tidyFlags for %s does not contain %s.", test.libName, flag)
+				}
+			}
+			for _, flag := range test.noFlags {
+				if strings.Contains(flags, flag) {
+					t.Errorf("tidyFlags for %s should not contain %s.", test.libName, flag)
+				}
+			}
+		})
+	}
+}
+
 func TestTidyChecks(t *testing.T) {
 	// The "tidy_checks" property defines additional checks appended
 	// to global default. But there are some checks disabled after