Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions Doc/whatsnew/3.8.rst
Original file line number Diff line number Diff line change
Expand Up @@ -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
-------------------------
Expand Down
4 changes: 2 additions & 2 deletions Lib/idlelib/iomenu.py
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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)
Expand Down
11 changes: 6 additions & 5 deletions Lib/test/test_ast.py
Original file line number Diff line number Diff line change
Expand Up @@ -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):
Expand Down
47 changes: 40 additions & 7 deletions Lib/test/test_grammar.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
import inspect
import unittest
import sys
import warnings
# testing import *
from sys import *

Expand Down Expand Up @@ -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 ())")
Expand Down Expand Up @@ -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")', '<testcase>', 'exec')
with warnings.catch_warnings():
warnings.filterwarnings('error', category=SyntaxWarning)
with self.assertRaisesRegex(SyntaxError, 'assertion is always true'):
compile('assert(x, "msg")', '<testcase>', 'exec')
compile('assert x, "msg"', '<testcase>', 'exec')


### compound_stmt: if_stmt | while_stmt | for_stmt | try_stmt | funcdef | classdef
# Tested below
Expand Down Expand Up @@ -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, '<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
Expand Down Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
@@ -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`.
84 changes: 73 additions & 11 deletions Python/compile.c
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wouldn't special case True and False, those are not normally identity checked. I don't doubt that someone may have create APIs returning multiple possible types that requires this to differentiate between "real" values and bools... but yuck.

|| 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 \"!=\"";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this warning looks good. I'd like to, but can't come up with a good way to work in the phrase "identity check".

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you have suggestions for the wording of the news or What's New entry?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about:

The compiler now produces a SyntaxWarning when identity checks (is and is not) are used with string literals. 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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! But a warning will be emitted not just for string literals, but for numerical literals and tuples (empty tuple is a singleton in CPython, this is an implementation detail).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can say "certain types of literals (e.g. strings, ints)" rather than "string literals".

return compiler_warn(c, msg);
}
}
left = right;
}
return 1;
}

static int
cmpop(cmpop_ty op)
{
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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;
Expand All @@ -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)
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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)
Expand Down