Merge "Tweak assertion checker: Don't flag nulls" into gradle_0.8

3 files changed, 76 insertions, 9 deletions

diff --git a/lint/cli/src/test/java/com/android/tools/lint/checks/AssertDetectorTest.java b/lint/cli/src/test/java/com/android/tools/lint/checks/AssertDetectorTest.java
index dfb2aeb1fe..9b64e00f94 100644
--- a/lint/cli/src/test/java/com/android/tools/lint/checks/AssertDetectorTest.java
+++ b/lint/cli/src/test/java/com/android/tools/lint/checks/AssertDetectorTest.java
@@ -28,12 +28,18 @@ public class AssertDetectorTest extends AbstractCheckTest {
     public void test() throws Exception {
         assertEquals(""
                 + "src/test/pkg/Assert.java:7: Warning: Assertions are unreliable. Use BuildConfig.DEBUG conditional checks instead. [Assert]\n"
-                + "        assert false;\n"
+                + "        assert false;                              // ERROR\n"
                 + "        ~~~~~~~~~~~~\n"
                 + "src/test/pkg/Assert.java:8: Warning: Assertions are unreliable. Use BuildConfig.DEBUG conditional checks instead. [Assert]\n"
-                + "        assert param > 5 : \"My description\";\n"
+                + "        assert param > 5 : \"My description\";       // ERROR\n"
                 + "        ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~\n"
-                + "0 errors, 2 warnings\n",
+                + "src/test/pkg/Assert.java:9: Warning: Assertions are unreliable. Use BuildConfig.DEBUG conditional checks instead. [Assert]\n"
+                + "        assert param2 == param3;                   // ERROR\n"
+                + "        ~~~~~~~~~~~~~~~~~~~~~~~\n"
+                + "src/test/pkg/Assert.java:10: Warning: Assertions are unreliable. Use BuildConfig.DEBUG conditional checks instead. [Assert]\n"
+                + "        assert param2 != null && param3 == param2; // ERROR\n"
+                + "        ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~\n"
+                + "0 errors, 4 warnings\n",
 
             lintProject("src/test/pkg/Assert.java.txt=>src/test/pkg/Assert.java"));
     }
diff --git a/lint/cli/src/test/java/com/android/tools/lint/checks/data/src/test/pkg/Assert.java.txt b/lint/cli/src/test/java/com/android/tools/lint/checks/data/src/test/pkg/Assert.java.txt
index a497207475..63a9ffdd40 100644
--- a/lint/cli/src/test/java/com/android/tools/lint/checks/data/src/test/pkg/Assert.java.txt
+++ b/lint/cli/src/test/java/com/android/tools/lint/checks/data/src/test/pkg/Assert.java.txt
@@ -3,13 +3,25 @@ package test.pkg;
 import android.annotations.tools.SuppressLint;
 
 public class Assert {
-    public Assert(int param) {
-        assert false;
-        assert param > 5 : "My description";
+    public Assert(int param, Object param2, Object param3) {
+        assert false;                              // ERROR
+        assert param > 5 : "My description";       // ERROR
+        assert param2 == param3;                   // ERROR
+        assert param2 != null && param3 == param2; // ERROR
+        assert true;                               // OK
+        assert param2 == null;                     // OK
+        assert param2 != null && param3 == null;   // OK
+        assert param2 == null && param3 != null;   // OK
+        assert param2 != null && param3 != null;   // OK
+        assert null != param2;                     // OK
+        assert param2 != null;                     // OK
+        assert param2 != null : "My description";  // OK
+        assert checkSuppressed(5) != null;         // OK
     }
 
     @SuppressLint("Assert")
-    public void checkSuppressed(int param) {
+    public Object checkSuppressed(int param) {
         assert param > 5 : "My description";
+        return null;
     }
 }
diff --git a/lint/libs/lint-checks/src/main/java/com/android/tools/lint/checks/AssertDetector.java b/lint/libs/lint-checks/src/main/java/com/android/tools/lint/checks/AssertDetector.java
index 53fd3cb444..1a862c01d9 100644
--- a/lint/libs/lint-checks/src/main/java/com/android/tools/lint/checks/AssertDetector.java
+++ b/lint/libs/lint-checks/src/main/java/com/android/tools/lint/checks/AssertDetector.java
@@ -32,8 +32,13 @@ import java.util.List;
 
 import lombok.ast.Assert;
 import lombok.ast.AstVisitor;
+import lombok.ast.BinaryExpression;
+import lombok.ast.BinaryOperator;
+import lombok.ast.BooleanLiteral;
+import lombok.ast.Expression;
 import lombok.ast.ForwardingAstVisitor;
 import lombok.ast.Node;
+import lombok.ast.NullLiteral;
 
 /**
  * Looks for assertion usages.
@@ -50,7 +55,14 @@ public class AssertDetector extends Detector implements Detector.JavaScanner {
             "many places and can not be relied upon. Instead, perform conditional checking " +
             "inside `if (BuildConfig.DEBUG) { }` blocks. That constant is a static final boolean " +
             "which is true in debug builds and false in release builds, and the Java compiler " +
-            "completely removes all code inside the if-body from the app.",
+            "completely removes all code inside the if-body from the app.\n" +
+            "\n" +
+            "For example, you can replace `assert speed > 0` with " +
+            "`if (BuildConfig.DEBUG && !(speed > 0)) { throw new AssertionError() }`.\n" +
+            "\n" +
+            "(Note: This lint check does not flag assertions purely asserting nullness or " +
+            "non-nullness; these are typically more intended for tools usage than runtime " +
+            "checks.)",
 
             Category.CORRECTNESS,
             6,
@@ -82,10 +94,47 @@ public class AssertDetector extends Detector implements Detector.JavaScanner {
         return new ForwardingAstVisitor() {
             @Override
             public boolean visitAssert(Assert node) {
-                String message = "Assertions are unreliable. Use BuildConfig.DEBUG conditional checks instead.";
+                Expression assertion = node.astAssertion();
+                // Allow "assert true"; it's basically a no-op
+                if (assertion instanceof BooleanLiteral) {
+                    Boolean b = ((BooleanLiteral) assertion).astValue();
+                    if (b != null && b) {
+                        return false;
+                    }
+                } else {
+                    // Allow assertions of the form "assert foo != null" because they are often used
+                    // to make statements to tools about known nullness properties. For example,
+                    // findViewById() may technically return null in some cases, but a developer
+                    // may know that it won't be when it's called correctly, so the assertion helps
+                    // to clear nullness warnings.
+                    if (isNullCheck(assertion)) {
+                        return false;
+                    }
+                }
+                String message
+                        = "Assertions are unreliable. Use BuildConfig.DEBUG conditional checks instead.";
                 context.report(ISSUE, node, context.getLocation(node), message, null);
                 return false;
             }
         };
     }
+
+    /**
+     * Checks whether the given expression is purely a non-null check, e.g. it will return
+     * true for expressions like "a != null" and "a != null && b != null" and
+     * "b == null || c != null".
+     */
+    private static boolean isNullCheck(Expression expression) {
+        if (expression instanceof BinaryExpression) {
+            BinaryExpression binExp = (BinaryExpression) expression;
+            if (binExp.astLeft() instanceof NullLiteral ||
+                    binExp.astRight() instanceof NullLiteral) {
+                return true;
+            } else {
+                return isNullCheck(binExp.astLeft()) && isNullCheck(binExp.astRight());
+            }
+        } else {
+            return false;
+        }
+    }
 }