bpo-34850: Emit a warning for "is" and "is not" with a literal. (GH-9642)

5 files changed, 95 insertions, 11 deletions

diff --git a/Doc/whatsnew/3.8.rst b/Doc/whatsnew/3.8.rst
index bf8d8f1..0360be6 100644
--- a/Doc/whatsnew/3.8.rst
+++ b/Doc/whatsnew/3.8.rst
@@ -442,6 +442,13 @@ Changes in Python behavior
   in the leftmost :keyword:`!for` clause).
   (Contributed by Serhiy Storchaka in :issue:`10544`.)
 
+* The compiler now produces a :exc:`SyntaxWarning` when identity checks
+  (``is`` and ``is not``) are used with certain types of literals
+  (e.g. strings, ints).  These can often work by accident in CPython,
+  but are not guaranteed by the language spec.  The warning advises users
+  to use equality tests (``==`` and ``!=``) instead.
+  (Contributed by Serhiy Storchaka in :issue:`34850`.)
+
 
 Changes in the Python API
 -------------------------
diff --git a/Lib/test/test_ast.py b/Lib/test/test_ast.py
index 2c8d8ab..897e705 100644
--- a/Lib/test/test_ast.py
+++ b/Lib/test/test_ast.py
@@ -1146,11 +1146,12 @@ class ASTValidatorTests(unittest.TestCase):
         tests = [fn for fn in os.listdir(stdlib) if fn.endswith(".py")]
         tests.extend(["test/test_grammar.py", "test/test_unpack_ex.py"])
         for module in tests:
-            fn = os.path.join(stdlib, module)
-            with open(fn, "r", encoding="utf-8") as fp:
-                source = fp.read()
-            mod = ast.parse(source, fn)
-            compile(mod, fn, "exec")
+            with self.subTest(module):
+                fn = os.path.join(stdlib, module)
+                with open(fn, "r", encoding="utf-8") as fp:
+                    source = fp.read()
+                mod = ast.parse(source, fn)
+                compile(mod, fn, "exec")
 
 
 class ConstantTests(unittest.TestCase):
diff --git a/Lib/test/test_grammar.py b/Lib/test/test_grammar.py
index 3d8b151..3ed19ff 100644
--- a/Lib/test/test_grammar.py
+++ b/Lib/test/test_grammar.py
@@ -1226,11 +1226,33 @@ class GrammarTests(unittest.TestCase):
         if 1 > 1: pass
         if 1 <= 1: pass
         if 1 >= 1: pass
-        if 1 is 1: pass
-        if 1 is not 1: pass
+        if x is x: pass
+        if x is not x: pass
         if 1 in (): pass
         if 1 not in (): pass
-        if 1 < 1 > 1 == 1 >= 1 <= 1 != 1 in 1 not in 1 is 1 is not 1: pass
+        if 1 < 1 > 1 == 1 >= 1 <= 1 != 1 in 1 not in x is x is not x: pass
+
+    def test_comparison_is_literal(self):
+        def check(test, msg='"is" with a literal'):
+            with self.assertWarnsRegex(SyntaxWarning, msg):
+                compile(test, '<testcase>', 'exec')
+            with warnings.catch_warnings():
+                warnings.filterwarnings('error', category=SyntaxWarning)
+                with self.assertRaisesRegex(SyntaxError, msg):
+                    compile(test, '<testcase>', 'exec')
+
+        check('x is 1')
+        check('x is "thing"')
+        check('1 is x')
+        check('x is y is 1')
+        check('x is not 1', '"is not" with a literal')
+
+        with warnings.catch_warnings():
+            warnings.filterwarnings('error', category=SyntaxWarning)
+            compile('x is None', '<testcase>', 'exec')
+            compile('x is False', '<testcase>', 'exec')
+            compile('x is True', '<testcase>', 'exec')
+            compile('x is ...', '<testcase>', 'exec')
 
     def test_binary_mask_ops(self):
         x = 1 & 1
@@ -1520,9 +1542,11 @@ class GrammarTests(unittest.TestCase):
         self.assertEqual(16 // (4 // 2), 8)
         self.assertEqual((16 // 4) // 2, 2)
         self.assertEqual(16 // 4 // 2, 2)
-        self.assertTrue(False is (2 is 3))
-        self.assertFalse((False is 2) is 3)
-        self.assertFalse(False is 2 is 3)
+        x = 2
+        y = 3
+        self.assertTrue(False is (x is y))
+        self.assertFalse((False is x) is y)
+        self.assertFalse(False is x is y)
 
     def test_matrix_mul(self):
         # This is not intended to be a comprehensive test, rather just to be few
diff --git a/Misc/NEWS.d/next/Core and Builtins/2018-09-30-11-19-55.bpo-34850.CbgDwb.rst b/Misc/NEWS.d/next/Core and Builtins/2018-09-30-11-19-55.bpo-34850.CbgDwb.rst
new file mode 100644
index 0000000..bc5d5d1
--- /dev/null
+++ b/Misc/NEWS.d/next/Core and Builtins/2018-09-30-11-19-55.bpo-34850.CbgDwb.rst
@@ -0,0 +1,5 @@
+The compiler now produces a :exc:`SyntaxWarning` when identity checks
+(``is`` and ``is not``) are used with certain types of literals
+(e.g. strings, ints).  These can often work by accident in CPython,
+but are not guaranteed by the language spec.  The warning advises users
+to use equality tests (``==`` and ``!=``) instead.
diff --git a/Python/compile.c b/Python/compile.c
index 45e78cb..5aebda0 100644
--- a/Python/compile.c
+++ b/Python/compile.c
@@ -2284,6 +2284,47 @@ compiler_class(struct compiler *c, stmt_ty s)
     return 1;
 }
 
+/* Return 0 if the expression is a constant value except named singletons.
+   Return 1 otherwise. */
+static int
+check_is_arg(expr_ty e)
+{
+    if (e->kind != Constant_kind) {
+        return 1;
+    }
+    PyObject *value = e->v.Constant.value;
+    return (value == Py_None
+         || value == Py_False
+         || value == Py_True
+         || value == Py_Ellipsis);
+}
+
+/* Check operands of identity chacks ("is" and "is not").
+   Emit a warning if any operand is a constant except named singletons.
+   Return 0 on error.
+ */
+static int
+check_compare(struct compiler *c, expr_ty e)
+{
+    Py_ssize_t i, n;
+    int left = check_is_arg(e->v.Compare.left);
+    n = asdl_seq_LEN(e->v.Compare.ops);
+    for (i = 0; i < n; i++) {
+        cmpop_ty op = (cmpop_ty)asdl_seq_GET(e->v.Compare.ops, i);
+        int right = check_is_arg((expr_ty)asdl_seq_GET(e->v.Compare.comparators, i));
+        if (op == Is || op == IsNot) {
+            if (!right || !left) {
+                const char *msg = (op == Is)
+                        ? "\"is\" with a literal. Did you mean \"==\"?"
+                        : "\"is not\" with a literal. Did you mean \"!=\"?";
+                return compiler_warn(c, msg);
+            }
+        }
+        left = right;
+    }
+    return 1;
+}
+
 static int
 cmpop(cmpop_ty op)
 {
@@ -2363,6 +2404,9 @@ compiler_jump_if(struct compiler *c, expr_ty e, basicblock *next, int cond)
         return 1;
     }
     case Compare_kind: {
+        if (!check_compare(c, e)) {
+            return 0;
+        }
         Py_ssize_t i, n = asdl_seq_LEN(e->v.Compare.ops) - 1;
         if (n > 0) {
             basicblock *cleanup = compiler_new_block(c);
@@ -3670,6 +3714,9 @@ compiler_compare(struct compiler *c, expr_ty e)
 {
     Py_ssize_t i, n;
 
+    if (!check_compare(c, e)) {
+        return 0;
+    }
     VISIT(c, expr, e->v.Compare.left);
     assert(asdl_seq_LEN(e->v.Compare.ops) > 0);
     n = asdl_seq_LEN(e->v.Compare.ops) - 1;