Add a flag to GuardedByChecker to allow matching ERRORs & member sele…

…cts on methods.

Plumb a flag through so that future improvements are possible.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=282908895

core/src/main/java/com/google/errorprone/bugpatterns/threadsafety/AbstractLockMethodChecker.java

@@ -103,7 +103,8 @@ private static Optional<ImmutableSet<GuardedByExpression>> parseLockExpressions(
     ImmutableSet.Builder<GuardedByExpression> builder = ImmutableSet.builder();
     for (String lockExpression : lockExpressions) {
       Optional<GuardedByExpression> guard =
-          GuardedByBinder.bindString(lockExpression, GuardedBySymbolResolver.from(tree, state));
+          GuardedByBinder.bindString(
+              lockExpression, GuardedBySymbolResolver.from(tree, state), GuardedByFlags.allOn());
       if (!guard.isPresent()) {
         return Optional.empty();
       }

core/src/main/java/com/google/errorprone/bugpatterns/threadsafety/GuardedByBinder.java

@@ -48,7 +48,7 @@ public class GuardedByBinder {
    * Optional.empty()} if the AST node doesn't correspond to a 'simple' lock expression.
    */
   public static Optional<GuardedByExpression> bindExpression(
-      JCTree.JCExpression exp, VisitorState visitorState) {
+      JCTree.JCExpression exp, VisitorState visitorState, GuardedByFlags flags) {
     try {
       return Optional.of(
           bind(
@@ -57,14 +57,16 @@ public static Optional<GuardedByExpression> bindExpression(
                   ALREADY_BOUND_RESOLVER,
                   ASTHelpers.getSymbol(visitorState.findEnclosing(ClassTree.class)),
                   visitorState.getTypes(),
-                  visitorState.getNames())));
+                  visitorState.getNames(),
+                  flags)));
     } catch (IllegalGuardedBy expected) {
       return Optional.empty();
     }
   }
 
   /** Creates a {@link GuardedByExpression} from a string, given the resolution context. */
-  static Optional<GuardedByExpression> bindString(String string, GuardedBySymbolResolver resolver) {
+  static Optional<GuardedByExpression> bindString(
+      String string, GuardedBySymbolResolver resolver, GuardedByFlags flags) {
     try {
       return Optional.of(
           bind(
@@ -73,7 +75,8 @@ static Optional<GuardedByExpression> bindString(String string, GuardedBySymbolRe
                   resolver,
                   resolver.enclosingClass(),
                   resolver.visitorState().getTypes(),
-                  resolver.visitorState().getNames())));
+                  resolver.visitorState().getNames(),
+                  flags)));
     } catch (IllegalGuardedBy expected) {
       return Optional.empty();
     }
@@ -84,17 +87,20 @@ private static class BinderContext {
     final ClassSymbol thisClass;
     final Types types;
     final Names names;
+    final GuardedByFlags flags;
 
-    public BinderContext(Resolver resolver, ClassSymbol thisClass, Types types, Names names) {
+    public BinderContext(
+        Resolver resolver, ClassSymbol thisClass, Types types, Names names, GuardedByFlags flags) {
       this.resolver = resolver;
       this.thisClass = thisClass;
       this.types = types;
       this.names = names;
+      this.flags = flags;
     }
 
     public static BinderContext of(
-        Resolver resolver, ClassSymbol thisClass, Types types, Names names) {
-      return new BinderContext(resolver, thisClass, types, names);
+        Resolver resolver, ClassSymbol thisClass, Types types, Names names, GuardedByFlags flags) {
+      return new BinderContext(resolver, thisClass, types, names, flags);
     }
   }
 
@@ -221,9 +227,9 @@ public GuardedByExpression visitMemberSelect(MemberSelectTree node, BinderContex
           checkGuardedBy(base != null, "Bad expression: %s", node.getExpression());
           Symbol sym = context.resolver.resolveSelect(base, node);
           checkGuardedBy(sym != null, "Could not resolve: %s", node);
-          // TODO(cushon): allow MethodSymbol here once clean-up is done
+          // TODO(b/144776758): remove flag once clean-up is done
           checkGuardedBy(
-              sym instanceof Symbol.VarSymbol /* || sym instanceof Symbol.MethodSymbol*/,
+              sym instanceof Symbol.VarSymbol || sym instanceof Symbol.MethodSymbol,
               "Bad member symbol: %s",
               sym.getClass());
           return bindSelect(normalizeBase(context, sym, base), sym);

core/src/main/java/com/google/errorprone/bugpatterns/threadsafety/GuardedByChecker.java

@@ -21,6 +21,7 @@
 
 import com.google.common.base.Joiner;
 import com.google.errorprone.BugPattern;
+import com.google.errorprone.ErrorProneFlags;
 import com.google.errorprone.VisitorState;
 import com.google.errorprone.bugpatterns.BugChecker;
 import com.google.errorprone.bugpatterns.BugChecker.LambdaExpressionTreeMatcher;
@@ -51,6 +52,12 @@ public class GuardedByChecker extends BugChecker
 
   private static final String JUC_READ_WRITE_LOCK = "java.util.concurrent.locks.ReadWriteLock";
 
+  private final GuardedByFlags flags;
+
+  public GuardedByChecker(ErrorProneFlags flags) {
+    this.flags = GuardedByFlags.of(flags.getBoolean("GuardedByChecker:MatchOnErrors").orElse(true));
+  }
+
   @Override
   public Description matchMethod(MethodTree tree, final VisitorState state) {
     // Constructors (and field initializers, instance initalizers, and class initalizers) are free
@@ -69,12 +76,13 @@ public Description matchLambdaExpression(LambdaExpressionTree tree, VisitorState
     return NO_MATCH;
   }
 
-  private void analyze(final VisitorState state) {
+  private void analyze(VisitorState state) {
     HeldLockAnalyzer.analyze(
         state,
         (ExpressionTree tree, GuardedByExpression guard, HeldLockSet live) ->
             report(GuardedByChecker.this.checkGuardedAccess(tree, guard, live, state), state),
-        this::isSuppressed);
+        this::isSuppressed,
+        flags);
   }
 
   @Override
@@ -108,8 +116,8 @@ protected Description checkGuardedAccess(
       return NO_MATCH;
     }
 
-    // TODO(cushon): re-enable once the clean-up is done
-    if (guard.kind() == GuardedByExpression.Kind.ERROR) {
+    // TODO(b/144776758): remove flag once the clean-up is done
+    if (!flags.matchMethodSymbols() && guard.kind() == GuardedByExpression.Kind.ERROR) {
       return NO_MATCH;
     }
 
@@ -212,8 +220,8 @@ private void report(Description description, VisitorState state) {
   }
 
   /** Validates that {@code @GuardedBy} strings can be resolved. */
-  Description validate(Tree tree, VisitorState state) {
-    GuardedByValidationResult result = GuardedByUtils.isGuardedByValid(tree, state);
+  private Description validate(Tree tree, VisitorState state) {
+    GuardedByValidationResult result = GuardedByUtils.isGuardedByValid(tree, state, flags);
     if (result.isValid()) {
       return Description.NO_MATCH;
     }

core/src/main/java/com/google/errorprone/bugpatterns/threadsafety/GuardedByFlags.java

@@ -0,0 +1,34 @@
+/*
+ * Copyright 2019 The Error Prone Authors.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except
+ * in compliance with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
+ * or implied. See the License for the specific language governing permissions and limitations under
+ * the License.
+ */
+
+package com.google.errorprone.bugpatterns.threadsafety;
+
+import com.google.auto.value.AutoValue;
+
+/**
+ * Flags that control the behavior of threadsafety utils to facilitate rolling out new
+ * functionality.
+ */
+@AutoValue
+public abstract class GuardedByFlags {
+  public abstract boolean matchMethodSymbols();
+
+  public static GuardedByFlags of(boolean matchMethodSymbols) {
+    return new AutoValue_GuardedByFlags(matchMethodSymbols);
+  }
+
+  public static GuardedByFlags allOn() {
+    return of(true);
+  }
+}

core/src/main/java/com/google/errorprone/bugpatterns/threadsafety/GuardedByUtils.java

@@ -100,7 +100,8 @@ static GuardedByValidationResult ok() {
     }
   }
 
-  public static GuardedByValidationResult isGuardedByValid(Tree tree, VisitorState state) {
+  public static GuardedByValidationResult isGuardedByValid(
+      Tree tree, VisitorState state, GuardedByFlags flags) {
     ImmutableSet<String> guards = GuardedByUtils.getGuardValues(tree, state);
     if (guards.isEmpty()) {
       return GuardedByValidationResult.ok();
@@ -109,7 +110,7 @@ public static GuardedByValidationResult isGuardedByValid(Tree tree, VisitorState
     List<GuardedByExpression> boundGuards = new ArrayList<>();
     for (String guard : guards) {
       Optional<GuardedByExpression> boundGuard =
-          GuardedByBinder.bindString(guard, GuardedBySymbolResolver.from(tree, state));
+          GuardedByBinder.bindString(guard, GuardedBySymbolResolver.from(tree, state), flags);
       if (!boundGuard.isPresent()) {
         return GuardedByValidationResult.invalid("could not resolve guard");
       }
@@ -134,9 +135,10 @@ public static GuardedByValidationResult isGuardedByValid(Tree tree, VisitorState
     return GuardedByValidationResult.ok();
   }
 
-  public static Symbol bindGuardedByString(Tree tree, String guard, VisitorState visitorState) {
+  public static Symbol bindGuardedByString(
+      Tree tree, String guard, VisitorState visitorState, GuardedByFlags flags) {
     Optional<GuardedByExpression> bound =
-        GuardedByBinder.bindString(guard, GuardedBySymbolResolver.from(tree, visitorState));
+        GuardedByBinder.bindString(guard, GuardedBySymbolResolver.from(tree, visitorState), flags);
     if (!bound.isPresent()) {
       return null;
     }