Multiple improvements to WaitNotInLoop.

Changes:
1) Now detects only the case where the wait is not in a loop. No longer detects
   the case where it is in a loop, but the synchronized block is inside the
   loop. (I will move that to a separate check)
2) Much better explanation in the markdown file, including suggestions for how
   to fix common patterns.
3) Now matches Condition.await() in addition to Object.wait().
4) Now produces a suggested fix for the simple case of changing the enclosing
   if to a while.

RELNOTES:
- Improvements to WaitNotInLoop. Detects a somewhat different set of problems,
  but explains them better, and fixes a subset.
-------------
Created by MOE: http://code.google.com/p/moe-java
MOE_MIGRATED_REVID=91902359

core/src/main/java/com/google/errorprone/bugpatterns/WaitNotInLoop.java

@@ -20,153 +20,66 @@
 import static com.google.errorprone.BugPattern.MaturityLevel.MATURE;
 import static com.google.errorprone.BugPattern.SeverityLevel.WARNING;
 import static com.google.errorprone.matchers.Matchers.allOf;
+import static com.google.errorprone.matchers.Matchers.inLoop;
 import static com.google.errorprone.matchers.Matchers.not;
-import static com.google.errorprone.matchers.method.MethodMatchers.instanceMethod;
+import static com.google.errorprone.matchers.WaitMatchers.waitMethod;
+import static com.google.errorprone.matchers.WaitMatchers.waitMethodWithTimeout;
 
 import com.google.errorprone.BugPattern;
 import com.google.errorprone.VisitorState;
 import com.google.errorprone.bugpatterns.BugChecker.MethodInvocationTreeMatcher;
 import com.google.errorprone.fixes.SuggestedFix;
 import com.google.errorprone.matchers.Description;
 import com.google.errorprone.matchers.Matcher;
-import com.google.errorprone.matchers.Matchers;
 import com.google.errorprone.util.ASTHelpers;
 
-import com.sun.source.tree.DoWhileLoopTree;
-import com.sun.source.tree.EnhancedForLoopTree;
-import com.sun.source.tree.ExpressionTree;
-import com.sun.source.tree.ForLoopTree;
 import com.sun.source.tree.MethodInvocationTree;
-import com.sun.source.tree.StatementTree;
-import com.sun.source.tree.SynchronizedTree;
-import com.sun.source.tree.Tree;
-import com.sun.source.tree.Tree.Kind;
-import com.sun.source.tree.WhileLoopTree;
-import com.sun.source.util.TreePath;
+import com.sun.tools.javac.code.Symbol.MethodSymbol;
 import com.sun.tools.javac.tree.JCTree.JCIf;
 
-import java.util.Arrays;
-import java.util.List;
-
 /**
- * TODO(user): Doesn't handle the case that the enclosing method is intended to be called
- * in a loop.
- *
  * @author eaftan@google.com (Eddie Aftandilian)
  */
+// TODO(user): Doesn't handle the case that the enclosing method is always called in a loop.
 @BugPattern(name = "WaitNotInLoop",
-    summary = "Object.wait() should always be called in a loop",
-    explanation = "Object.wait() can be woken up in multiple ways, none of which guarantee that " +
-        "the condition it was waiting for has become true (spurious wakeups, for example). " +
-        "Thus, Object.wait() should always be called in a loop that checks the condition " +
-        "predicate.  Additionally, the loop should be inside a synchronized block or " +
-        "method to avoid race conditions on the condition predicate.\n\n" +
-        "See Java Concurrency in Practice section 14.2.2, \"Waking up too soon,\" and " +
-        "[http://docs.oracle.com/javase/7/docs/api/java/lang/Object.html#wait() " +
-        "the Javadoc for Object.wait()].",
+    summary = "Because of spurious wakeups, Object.wait() and Condition.await() must always be "
+        + "called in a loop",
     category = JDK, severity = WARNING, maturity = MATURE)
 public class WaitNotInLoop extends BugChecker implements MethodInvocationTreeMatcher {
 
-  // Since some of the fixes have formatting problems, do not supply them unless explicitly enabled.
-  private static final boolean SUPPLY_FIX = false;
-
-  /**
-   * Matches tree nodes that are enclosed in a loop before hitting a synchronized block or
-   * method definition.
-   */
-  private static Matcher<Tree> inLoopBeforeSynchronizedMatcher = new Matcher<Tree>() {
-    @Override
-    public boolean matches(Tree t, VisitorState state) {
-      TreePath path = state.getPath().getParentPath();
-      Tree node = path.getLeaf();
-      while (path != null) {
-        if (node.getKind() == Kind.SYNCHRONIZED || node.getKind() == Kind.METHOD) {
-          return false;
-        }
-        if (node.getKind() == Kind.WHILE_LOOP || node.getKind() == Kind.FOR_LOOP ||
-            node.getKind() == Kind.ENHANCED_FOR_LOOP || node.getKind() == Kind.DO_WHILE_LOOP) {
-          return true;
-        }
-        path = path.getParentPath();
-        node = path.getLeaf();
-      }
-      return false;
-    }
-  };
+  private static final String MESSAGE_TEMPLATE =
+      "Because of spurious wakeups, %s must always be called in a loop";
 
-  /**
-   * Matches if:
-   * 1) The method call is a call to any of Object.wait(), Object.wait(long), or
-   *    Object.wait(long, int), and
-   * 2) There is no enclosing loop before reaching a synchronized block or method declaration.
-   */
-  private static Matcher<MethodInvocationTree> waitMatcher = allOf(
-        Matchers.<ExpressionTree>anyOf(
-            instanceMethod().onDescendantOf("java.lang.Object").withSignature("wait()"),
-            instanceMethod().onDescendantOf("java.lang.Object").withSignature("wait(long)"),
-            instanceMethod().onDescendantOf("java.lang.Object").withSignature("wait(long,int)")),
-        not(inLoopBeforeSynchronizedMatcher));
+  private static final Matcher<MethodInvocationTree> matcher = allOf(waitMethod, not(inLoop()));
 
-  // TODO(user): Better suggested fixes.
   @Override
   public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) {
-    if (!waitMatcher.matches(tree, state)) {
+    if (!matcher.matches(tree, state)) {
       return Description.NO_MATCH;
     }
 
-    if (!SUPPLY_FIX) {
-      return describeMatch(tree);
+    Description.Builder description = buildDescription(tree);
+    MethodSymbol sym = ASTHelpers.getSymbol(tree);
+    if (sym != null) {
+      description.setMessage(String.format(MESSAGE_TEMPLATE, sym));
     }
 
-    // if -> while case
-    JCIf enclosingIf =
-        ASTHelpers.findEnclosingNode(state.getPath().getParentPath(), JCIf.class);
-    if (enclosingIf != null && enclosingIf.getElseStatement() == null) {
-      // Assume first 2 characters of the IfTree are "if", replace with while.
-      int startPos = enclosingIf.getStartPosition();
-      return describeMatch(tree, SuggestedFix.replace(startPos, startPos + 2, "while"));
-    }
-
-    // loop outside synchronized block -> move synchronized outside
-    List<Class<? extends StatementTree>> loopClasses = Arrays.asList(WhileLoopTree.class, ForLoopTree.class,
-        EnhancedForLoopTree.class, DoWhileLoopTree.class);
-    StatementTree enclosingLoop = null;
-    for (Class<? extends StatementTree> loopClass : loopClasses) {
-      enclosingLoop = ASTHelpers.findEnclosingNode(state.getPath().getParentPath(), loopClass);
-      if (enclosingLoop != null) {
-        break;
-      }
-    }
-    if (enclosingLoop != null) {
-      SynchronizedTree enclosingSynchronized = ASTHelpers.findEnclosingNode(
-          state.getPath().getParentPath(), SynchronizedTree.class);
-      if (enclosingSynchronized != null) {
-        String blockStatements = enclosingSynchronized.getBlock().toString();
-        int openBracketIndex = blockStatements.indexOf('{');
-        int closeBracketIndex = blockStatements.lastIndexOf('}');
-        blockStatements = blockStatements.substring(openBracketIndex + 1, closeBracketIndex).trim();
-        return describeMatch(tree, SuggestedFix.builder()
-            .replace(enclosingSynchronized, blockStatements)
-            .prefixWith(enclosingLoop, "synchronized " + enclosingSynchronized.getExpression() + " {\n")
-            .postfixWith(enclosingLoop, "\n}")
-            .build());
+    // If this looks like the "Wait until a condition becomes true" case from the wiki content,
+    // rewrite the enclosing if to a while.  Other fixes are too complicated to construct
+    // mechanically, so we provide detailed instructions in the wiki content.
+    if (!waitMethodWithTimeout.matches(tree, state)) {
+      JCIf enclosingIf = ASTHelpers.findEnclosingNode(state.getPath().getParentPath(), JCIf.class);
+      if (enclosingIf != null && enclosingIf.getElseStatement() == null) {
+        CharSequence ifSource = state.getSourceForNode(enclosingIf);
+        if (ifSource == null) {
+          // Source isn't available, so we can't construct a fix
+          return description.build();
+        }
+        String replacement = ifSource.toString().replaceFirst("if", "while");
+        return description.addFix(SuggestedFix.replace(enclosingIf, replacement)).build();
       }
     }
 
-    // Intent is to wait forever -> wrap in while (true)
-    // Heuristic: this is the last statement in a method called main, inside a synchronized block.
-    /*
-    if (enclosingIf == null
-        && (ASTHelpers.findEnclosingNode(state.getPath().getParentPath(), WhileLoopTree.class) == null)
-        && (ASTHelpers.findEnclosingNode(state.getPath().getParentPath(), ForLoopTree.class) == null)
-        && (ASTHelpers.findEnclosingNode(state.getPath().getParentPath(), EnhancedForLoopTree.class) == null)
-        && (ASTHelpers.findEnclosingNode(state.getPath().getParentPath(), DoWhileLoopTree.class) == null)) {
-      TreeMaker treeMaker = TreeMaker.instance(state.context);
-      JCLiteral trueLiteral = treeMaker.Literal(true);
-      treeMaker.WhileLoop(trueLiteral,
-    }
-    */
-
-    return describeMatch(tree);
+    return description.build();
   }
 }

core/src/main/java/com/google/errorprone/matchers/Matchers.java

@@ -1256,6 +1256,36 @@ public boolean matches(EnhancedForLoopTree t, VisitorState state) {
     };
   }
 
+  /**
+   * Matches if the given tree is inside a loop.
+   */
+  public static <T extends Tree> Matcher<T> inLoop() {
+    return new Matcher<T>() {
+      @Override
+      public boolean matches(Tree tree, VisitorState state) {
+        TreePath path = state.getPath().getParentPath();
+        Tree node = path.getLeaf();
+        while (path != null) {
+          switch (node.getKind()) {
+            case METHOD:
+            case CLASS:
+              return false;
+            case WHILE_LOOP:
+            case FOR_LOOP:
+            case ENHANCED_FOR_LOOP:
+            case DO_WHILE_LOOP:
+              return true;
+            default:
+              path = path.getParentPath();
+              node = path.getLeaf();
+              break;
+          }
+        }
+        return false;
+      }
+    };
+  }
+
   /**
    * Matches an assignment operator AST node if both of the given matchers match.
    *

core/src/main/java/com/google/errorprone/matchers/WaitMatchers.java

@@ -0,0 +1,51 @@
+/*
+ * Copyright 2015 Google Inc. All Rights Reserved.
+ *
+ * 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.matchers;
+
+import static com.google.errorprone.matchers.Matchers.anyOf;
+import static com.google.errorprone.matchers.method.MethodMatchers.instanceMethod;
+
+import com.sun.source.tree.MethodInvocationTree;
+
+import java.util.regex.Pattern;
+
+/**
+ * Matchers for method invocations related to Object.wait() and Condition.await();
+ */
+public class WaitMatchers {
+
+  private static final String OBJECT_FQN = "java.lang.Object";
+  private static final String CONDITION_FQN = "java.util.concurrent.locks.Condition";
+
+  /**
+   * Matches any wait/await method.
+   */
+  public static final Matcher<MethodInvocationTree> waitMethod = anyOf(
+      instanceMethod().onExactClass(OBJECT_FQN).named("wait"),
+      instanceMethod().onDescendantOf(CONDITION_FQN).withNameMatching(Pattern.compile("await.*")));
+
+  /**
+   * Matches wait/await methods that have a timeout.
+   */
+  public static final Matcher<MethodInvocationTree> waitMethodWithTimeout = anyOf(
+      instanceMethod().onExactClass(OBJECT_FQN).withSignature("wait(long)"),
+      instanceMethod().onExactClass(OBJECT_FQN).withSignature("wait(long,int)"),
+      instanceMethod().onDescendantOf(CONDITION_FQN)
+          .withSignature("await(long,java.util.concurrent.TimeUnit)"),
+      instanceMethod().onDescendantOf(CONDITION_FQN).named("awaitNanos"),
+      instanceMethod().onDescendantOf(CONDITION_FQN).named("awaitUntil"));
+}

core/src/test/java/com/google/errorprone/MatcherChecker.java

@@ -0,0 +1,59 @@
+/*
+ * Copyright 2015 Google Inc. All Rights Reserved.
+ *
+ * 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;
+
+import static com.google.errorprone.BugPattern.Category.ONE_OFF;
+import static com.google.errorprone.BugPattern.MaturityLevel.MATURE;
+import static com.google.errorprone.BugPattern.SeverityLevel.ERROR;
+
+import com.google.errorprone.bugpatterns.BugChecker;
+import com.google.errorprone.bugpatterns.BugChecker.ExpressionStatementTreeMatcher;
+import com.google.errorprone.matchers.Description;
+import com.google.errorprone.matchers.Matcher;
+
+import com.sun.source.tree.ExpressionStatementTree;
+import com.sun.source.tree.Tree;
+
+/**
+ * A {@link BugChecker} that flags {@link ExpressionStatementTree}s that satisfy both of the
+ * following:
+ * <ul>
+ * <li>The text of the tree is the same as the given {@code expressionStatement}, and
+ * <li>The given {@code matcher} matches the tree
+ * </ul>
+ *
+ * <p>Useful for testing {@link Matcher}s.
+ */
+@BugPattern(name = "MatcherChecker",
+   summary = "Checker that flags the given expression statement if the given matcher matches",
+   category = ONE_OFF, maturity = MATURE, severity = ERROR)
+public class MatcherChecker extends BugChecker implements ExpressionStatementTreeMatcher {
+  private final String expressionStatement;
+  private final Matcher<Tree> matcher;
+
+  public MatcherChecker(String expressionStatement, Matcher<Tree> matcher) {
+    this.expressionStatement = expressionStatement;
+    this.matcher = matcher;
+  }
+
+  @Override
+  public Description matchExpressionStatement(ExpressionStatementTree tree, VisitorState state) {
+    return (tree.toString().equals(expressionStatement) && matcher.matches(tree, state))
+        ? describeMatch(tree)
+        : Description.NO_MATCH;
+  }
+}

core/src/test/java/com/google/errorprone/bugpatterns/testdata/WaitNotInLoopNegativeCases.java

@@ -14,7 +14,7 @@
  * limitations under the License.
  */
 
-package com.google.errorprone.bugpatterns;
+package com.google.errorprone.bugpatterns.testdata;
 
 /**
  * @author eaftan@google.com (Eddie Aftandilian)
@@ -58,6 +58,18 @@ public void test3() {
     }
   }
 
+  // This code is incorrect, but this check should not flag it.
+  public void testLoopNotInSynchronized() {
+    while (!flag) {
+      synchronized (this) {
+        try {
+          wait();
+        } catch (InterruptedException e) {
+        }
+      }
+    }
+  }
+
   public void testDoLoop() {
     synchronized (this) {
       do {
@@ -71,7 +83,7 @@ public void testDoLoop() {
 
   public void testForLoop() {
     synchronized (this) {
-      for (;!flag;) {
+      for (; !flag; ) {
         try {
           wait();
         } catch (InterruptedException e) {