From 97593cc6a05fb2a8761ed738b3b6ac38cea63f3c Mon Sep 17 00:00:00 2001 From: Serhiy Storchaka Date: Sun, 30 Sep 2018 11:20:04 +0300 Subject: [PATCH 1/5] bpo-34850: Emit a warning for "is" and "is not" with a literal. --- Doc/whatsnew/3.8.rst | 6 ++ Lib/idlelib/iomenu.py | 4 +- Lib/test/test_ast.py | 11 +-- Lib/test/test_grammar.py | 47 +++++++++-- .../2018-09-30-11-19-55.bpo-34850.CbgDwb.rst | 4 + Python/compile.c | 84 ++++++++++++++++--- 6 files changed, 131 insertions(+), 25 deletions(-) create mode 100644 Misc/NEWS.d/next/Core and Builtins/2018-09-30-11-19-55.bpo-34850.CbgDwb.rst diff --git a/Doc/whatsnew/3.8.rst b/Doc/whatsnew/3.8.rst index a146249178f40d..2de9116ed92ee1 100644 --- a/Doc/whatsnew/3.8.rst +++ b/Doc/whatsnew/3.8.rst @@ -305,6 +305,12 @@ Changes in Python behavior in the leftmost :keyword:`for` clause). (Contributed by Serhiy Storchaka in :issue:`10544`.) +* The compiler will emit a syntax warning when the ``is`` or ``is not`` + operator is used with a literal (except singletons ``None``, ``False``, + ``True`` and ``...``). It will became a :exc:`SyntaxError` if run the + interpreter with the ``-W error::SyntaxWarning`` option. + (Contributed by Serhiy Storchaka in :issue:`34850`.) + Changes in the Python API ------------------------- diff --git a/Lib/idlelib/iomenu.py b/Lib/idlelib/iomenu.py index 63d107dd9003df..fcd8dcc1393da7 100644 --- a/Lib/idlelib/iomenu.py +++ b/Lib/idlelib/iomenu.py @@ -40,7 +40,7 @@ # resulting codeset may be unknown to Python. We ignore all # these problems, falling back to ASCII locale_encoding = locale.nl_langinfo(locale.CODESET) - if locale_encoding is None or locale_encoding is '': + if locale_encoding is None or locale_encoding == '': # situation occurs on Mac OS X locale_encoding = 'ascii' codecs.lookup(locale_encoding) @@ -50,7 +50,7 @@ # bugs that can cause ValueError. try: locale_encoding = locale.getdefaultlocale()[1] - if locale_encoding is None or locale_encoding is '': + if locale_encoding is None or locale_encoding == '': # situation occurs on Mac OS X locale_encoding = 'ascii' codecs.lookup(locale_encoding) diff --git a/Lib/test/test_ast.py b/Lib/test/test_ast.py index 10be02eee0c03c..4b8daadb342192 100644 --- a/Lib/test/test_ast.py +++ b/Lib/test/test_ast.py @@ -1098,11 +1098,12 @@ def test_stdlib_validates(self): 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 462e77a0be554a..fa74bf0f3f8e3b 100644 --- a/Lib/test/test_grammar.py +++ b/Lib/test/test_grammar.py @@ -5,6 +5,7 @@ import inspect import unittest import sys +import warnings # testing import * from sys import * @@ -989,7 +990,7 @@ def g(): f((yield from ())) def g(): f((yield from ()), 1) # Do not require parenthesis for tuple unpacking def g(): rest = 4, 5, 6; yield 1, 2, 3, *rest - self.assertEquals(list(g()), [(1, 2, 3, 4, 5, 6)]) + self.assertEqual(list(g()), [(1, 2, 3, 4, 5, 6)]) check_syntax_error(self, "def g(): f(yield 1)") check_syntax_error(self, "def g(): f(yield 1, 1)") check_syntax_error(self, "def g(): f(yield from ())") @@ -1099,6 +1100,14 @@ def testAssert2(self): else: self.fail("AssertionError not raised by 'assert False'") + with self.assertWarnsRegex(SyntaxWarning, 'assertion is always true'): + compile('assert(x, "msg")', '', 'exec') + with warnings.catch_warnings(): + warnings.filterwarnings('error', category=SyntaxWarning) + with self.assertRaisesRegex(SyntaxError, 'assertion is always true'): + compile('assert(x, "msg")', '', 'exec') + compile('assert x, "msg"', '', 'exec') + ### compound_stmt: if_stmt | while_stmt | for_stmt | try_stmt | funcdef | classdef # Tested below @@ -1217,11 +1226,33 @@ def test_comparison(self): 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, '', 'exec') + with warnings.catch_warnings(): + warnings.filterwarnings('error', category=SyntaxWarning) + with self.assertRaisesRegex(SyntaxError, msg): + compile(test, '', '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', '', 'exec') + compile('x is False', '', 'exec') + compile('x is True', '', 'exec') + compile('x is ...', '', 'exec') def test_binary_mask_ops(self): x = 1 & 1 @@ -1511,9 +1542,11 @@ def test_paren_evaluation(self): 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 00000000000000..f9af755d3b2299 --- /dev/null +++ b/Misc/NEWS.d/next/Core and Builtins/2018-09-30-11-19-55.bpo-34850.CbgDwb.rst @@ -0,0 +1,4 @@ +The compiler will emit a syntax warning when the ``is`` or ``is not`` +operator is used with a literal (except singletons ``None``, ``False``, +``True`` and ``...``). If :exc:`SyntaxWarning` is raised as error, it will +be replaced with a :exc:`SyntaxError`. diff --git a/Python/compile.c b/Python/compile.c index 096b762f36bb0d..8c022d9a9dea08 100644 --- a/Python/compile.c +++ b/Python/compile.c @@ -174,6 +174,7 @@ static int compiler_addop(struct compiler *, int); static int compiler_addop_i(struct compiler *, int, Py_ssize_t); static int compiler_addop_j(struct compiler *, int, basicblock *, int); static int compiler_error(struct compiler *, const char *); +static int compiler_warn(struct compiler *, const char *); static int compiler_nameop(struct compiler *, identifier, expr_context_ty); static PyCodeObject *compiler_mod(struct compiler *, mod_ty); @@ -2155,6 +2156,41 @@ compiler_class(struct compiler *c, stmt_ty s) return 1; } +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); +} + +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) { @@ -2234,6 +2270,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); @@ -2970,7 +3009,6 @@ compiler_assert(struct compiler *c, stmt_ty s) { static PyObject *assertion_error = NULL; basicblock *end; - PyObject* msg; if (c->c_optimize) return 1; @@ -2980,18 +3018,13 @@ compiler_assert(struct compiler *c, stmt_ty s) return 0; } if (s->v.Assert.test->kind == Tuple_kind && - asdl_seq_LEN(s->v.Assert.test->v.Tuple.elts) > 0) { - msg = PyUnicode_FromString("assertion is always true, " - "perhaps remove parentheses?"); - if (msg == NULL) - return 0; - if (PyErr_WarnExplicitObject(PyExc_SyntaxWarning, msg, - c->c_filename, c->u->u_lineno, - NULL, NULL) == -1) { - Py_DECREF(msg); + asdl_seq_LEN(s->v.Assert.test->v.Tuple.elts) > 0) + { + if (!compiler_warn(c, "assertion is always true, " + "perhaps remove parentheses?")) + { return 0; } - Py_DECREF(msg); } end = compiler_new_block(c); if (end == NULL) @@ -3546,6 +3579,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; @@ -4792,6 +4828,32 @@ compiler_error(struct compiler *c, const char *errstr) return 0; } +/* Emits a SyntaxWarning and returns 1 on success. + If a SyntaxWarning is raised as error, replaces it with a SyntaxError + and returns 0. +*/ + +static int +compiler_warn(struct compiler *c, const char *errstr) +{ + PyObject *msg = PyUnicode_FromString(errstr); + if (msg == NULL) { + return 0; + } + if (PyErr_WarnExplicitObject(PyExc_SyntaxWarning, msg, c->c_filename, + c->u->u_lineno, NULL, NULL) < 0) + { + Py_DECREF(msg); + if (PyErr_ExceptionMatches(PyExc_SyntaxWarning)) { + PyErr_Clear(); + return compiler_error(c, errstr); + } + return 0; + } + Py_DECREF(msg); + return 1; +} + static int compiler_handle_subscr(struct compiler *c, const char *kind, expr_context_ty ctx) From b4a5ca13204d77d8c4dda00b4788c023df3fc9d7 Mon Sep 17 00:00:00 2001 From: Serhiy Storchaka Date: Sun, 30 Sep 2018 15:42:02 +0300 Subject: [PATCH 2/5] Remove unintentionally inserted character. --- Lib/test/test_grammar.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Lib/test/test_grammar.py b/Lib/test/test_grammar.py index fa74bf0f3f8e3b..3ed19ff1cb04c1 100644 --- a/Lib/test/test_grammar.py +++ b/Lib/test/test_grammar.py @@ -1233,7 +1233,7 @@ def test_comparison(self): 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'): + def check(test, msg='"is" with a literal'): with self.assertWarnsRegex(SyntaxWarning, msg): compile(test, '', 'exec') with warnings.catch_warnings(): From 4f906c30ad8ed5f2e8196eeaa671f05f2a301d96 Mon Sep 17 00:00:00 2001 From: Terry Jan Reedy Date: Sun, 30 Sep 2018 16:33:19 -0400 Subject: [PATCH 3/5] Update iomenu.py Revert change here; will make separate PR to be applied and backported regardless of fate of this PR. --- Lib/idlelib/iomenu.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Lib/idlelib/iomenu.py b/Lib/idlelib/iomenu.py index fcd8dcc1393da7..63d107dd9003df 100644 --- a/Lib/idlelib/iomenu.py +++ b/Lib/idlelib/iomenu.py @@ -40,7 +40,7 @@ # resulting codeset may be unknown to Python. We ignore all # these problems, falling back to ASCII locale_encoding = locale.nl_langinfo(locale.CODESET) - if locale_encoding is None or locale_encoding == '': + if locale_encoding is None or locale_encoding is '': # situation occurs on Mac OS X locale_encoding = 'ascii' codecs.lookup(locale_encoding) @@ -50,7 +50,7 @@ # bugs that can cause ValueError. try: locale_encoding = locale.getdefaultlocale()[1] - if locale_encoding is None or locale_encoding == '': + if locale_encoding is None or locale_encoding is '': # situation occurs on Mac OS X locale_encoding = 'ascii' codecs.lookup(locale_encoding) From a7d4628cad1cd8a977c23e26e6a96c7e7bd2c230 Mon Sep 17 00:00:00 2001 From: Serhiy Storchaka Date: Fri, 5 Oct 2018 20:42:28 +0300 Subject: [PATCH 4/5] Update documentation and add missed "?". --- Doc/whatsnew/3.8.rst | 9 +++++---- .../2018-09-30-11-19-55.bpo-34850.CbgDwb.rst | 9 +++++---- Python/compile.c | 4 ++-- 3 files changed, 12 insertions(+), 10 deletions(-) diff --git a/Doc/whatsnew/3.8.rst b/Doc/whatsnew/3.8.rst index 2de9116ed92ee1..3453c0a1d90a09 100644 --- a/Doc/whatsnew/3.8.rst +++ b/Doc/whatsnew/3.8.rst @@ -305,10 +305,11 @@ Changes in Python behavior in the leftmost :keyword:`for` clause). (Contributed by Serhiy Storchaka in :issue:`10544`.) -* The compiler will emit a syntax warning when the ``is`` or ``is not`` - operator is used with a literal (except singletons ``None``, ``False``, - ``True`` and ``...``). It will became a :exc:`SyntaxError` if run the - interpreter with the ``-W error::SyntaxWarning`` option. +* 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`.) 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 index f9af755d3b2299..bc5d5d1d5808f2 100644 --- 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 @@ -1,4 +1,5 @@ -The compiler will emit a syntax warning when the ``is`` or ``is not`` -operator is used with a literal (except singletons ``None``, ``False``, -``True`` and ``...``). If :exc:`SyntaxWarning` is raised as error, it will -be replaced with a :exc:`SyntaxError`. +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 8c022d9a9dea08..f06a8b7bde527a 100644 --- a/Python/compile.c +++ b/Python/compile.c @@ -2181,8 +2181,8 @@ check_compare(struct compiler *c, expr_ty e) 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 \"!=\""; + ? "\"is\" with a literal. Did you mean \"==\"?" + : "\"is not\" with a literal. Did you mean \"!=\"?"; return compiler_warn(c, msg); } } From 05e81bd18c15b6c848c6e000bfd0b27c6a039192 Mon Sep 17 00:00:00 2001 From: Serhiy Storchaka Date: Sun, 21 Oct 2018 10:22:53 +0300 Subject: [PATCH 5/5] Add comments. --- Python/compile.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/Python/compile.c b/Python/compile.c index c2532a167d606d..c4addfada18c06 100644 --- a/Python/compile.c +++ b/Python/compile.c @@ -2156,6 +2156,8 @@ 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) { @@ -2169,6 +2171,10 @@ check_is_arg(expr_ty e) || 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) {