classification
Title: Emit a syntax warning for "is" with a literal
Type: enhancement Stage: resolved
Components: Interpreter Core Versions: Python 3.8
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: serhiy.storchaka Nosy List: barry, gregory.p.smith, jwilk, miss-islington, nascheme, njs, rhettinger, serhiy.storchaka, steven.daprano, terry.reedy, vstinner, xiang.zhang
Priority: normal Keywords: patch

Created on 2018-09-30 08:15 by serhiy.storchaka, last changed 2019-01-21 15:19 by vstinner. This issue is now closed.

Pull Requests
URL Status Linked Edit
PR 9642 merged serhiy.storchaka, 2018-09-30 08:30
PR 9649 merged terry.reedy, 2018-09-30 20:38
PR 9650 merged miss-islington, 2018-09-30 21:17
PR 9651 merged miss-islington, 2018-09-30 21:17
Messages (36)
msg326714 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2018-09-30 08:15
Gregory have noticed that the "is" and "is not" operator sometimes is used with string and numerical literals. This code "works" on CPython by accident, because of caching on different levels (small integers and strings caches, interned strings, deduplicating constants at compile time). But it shouldn't work on other implementations, and can not work even on early or future CPython versions.

https://discuss.python.org/t/demoting-the-is-operator-to-avoid-an-identity-crisis/86

I think that the adequate solution of this issue is not demoting the "is" operator, but emitting a syntax warning. In general, this is a work for third-party checkers. But many people don't use checkers for their one-time scripts, using "is" with a literal is always a mistake, and it is easy to add a check for this in the compiler.

Currently the compiler emits SyntaxWarning only for parenthesis in assert: `assert(x, "msg")`. But in earlier versions there were more warnings (they are errors). In 3.8 yet one SyntaxWarning will be added instead of DeprecationWarning for unknown escape sequences in string literals.

The proposed PR makes the compiler emitting a SyntaxWarning when the "is" or "is not" operators are used with a constant (except singletons None, False, True and ...). A warning will be replaced with a SyntaxError if the warnings system is configured to raise errors. This is because SyntaxError contains more information and provides better traceback. The same change was made for "assert(...)". Added tests, including tests for "assert(...)".

Barry have noted that using the "==" operator with None can be also classified as an error. But I think that in this case there may be legal uses of this, and the compiler should not complain. It is a work for third-party checkers, which can provide configuration options for enabling and disabling particular kinds of checks.
msg326715 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2018-09-30 08:32
It also fixes an incorrect use of "is" with an empty string in IDLE.
msg326721 - (view) Author: Steven D'Aprano (steven.daprano) * (Python committer) Date: 2018-09-30 12:28
I like this solution of a syntax warning for `is <literal>` and I agree that `== None` should not be a warning.

(And for what its worth, I strongly disagree with making `is` a hybrid sometimes-check-identity-sometimes-check-equality operator.)
msg326722 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2018-09-30 12:45
Suggestions about the wording of the warning message are welcome.
msg326729 - (view) Author: Xiang Zhang (xiang.zhang) * (Python committer) Date: 2018-09-30 15:33
I have catched using `is` to do string equality check in our codebase by linters before. Not all my colleagues know what's not suitable here.

The only common and suitable case I can think of is programmers are exploring CPython internals, like what stackoverflow Q&As and some Python blogs.
msg326731 - (view) Author: Gregory P. Smith (gregory.p.smith) * (Python committer) Date: 2018-09-30 16:26
The problem with a SyntaxWarning is that the wrong people will see it. It gets in the way of users of applications that happen to be written in Python.

scenarios:

(a) A Python interpreter gets upgraded, and suddenly the _users_ of an application start seeing nasty warnings when they invoke the tool that happens to be implemented in Python. Warnings that they have no power to do anything about.

(b) A developer installs the shiny new version of Python with the warning feature, then pip install's all of the dependencies for the application they are working on.  Some of those contain "is" problems so they're left in a situation where they either have to figure out how to fix code from N different transitive dependencies libraries that they are not the author of - or not use that Python version to ship their code.  Keep in mind that it is common for deps to be pinned to ranges of versions instead of "always the latest" so even when deps fix their code, many people won't see it for a long time.

These scenarios are real, this is exactly what happened with the DeprecationWarning added to the old md5.py and sha.py modules.

A warning raised at import time isn't even one that can be meaningfully caught in a well structured application.  Doing so requires importing the warnings module first thing and setting up warning filters before any other imports.  This is ugly boilerplate for anyone to need to resort to.

As much as I want us to do something to ameliorate this anti-pattern in code, I don't think a SyntaxWarning is a great way to do it.
msg326734 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2018-09-30 17:02
There is a large difference with the DeprecationWarning in the md5 and sha modules.

A SyntaxWarning is emitted when the compiler compiles Python sources to bytecode. Since bytecode is cached in pyc files, you will see it at most once at first run of the application. If the application compiles Python files at install time, warnings will be emitted at that time, and will be not emitted when run the application. If the application is distributed with precompiled pyc files, the user will not see warnings at all. If the developer installs dependencies that contain this error, his will see a warning only once, and can either ignore it (taking the current state), or report a bug. Warnings will not annoy him when he debug his code.

In contrary, the DeprecationWarning was emitted every time when you import the md5 or sha modules.

Professional applications likely already use checkers which caught this error. This warning will help non-professional applications distributed as a single script or handful of scripts. Users of such application often seat near its author. In many cases the only user is its author.
msg326741 - (view) Author: Barry A. Warsaw (barry) * (Python committer) Date: 2018-09-30 18:28
This is a nice approach to the problem.  I completely agree that we cannot change `is` semantics.  I'm okay with leaving it to checkers to catch `== None`.
msg326744 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2018-09-30 18:41
Yet, Greg’s point is that this only works if the developer tests their code
with the new Python version.

I’m not sure that his proposal is better though. I think static checkers
are the better remedy.

On Sun, Sep 30, 2018 at 10:02 AM Serhiy Storchaka <report@bugs.python.org>
wrote:

>
> Serhiy Storchaka <storchaka+cpython@gmail.com> added the comment:
>
> There is a large difference with the DeprecationWarning in the md5 and sha
> modules.
>
> A SyntaxWarning is emitted when the compiler compiles Python sources to
> bytecode. Since bytecode is cached in pyc files, you will see it at most
> once at first run of the application. If the application compiles Python
> files at install time, warnings will be emitted at that time, and will be
> not emitted when run the application. If the application is distributed
> with precompiled pyc files, the user will not see warnings at all. If the
> developer installs dependencies that contain this error, his will see a
> warning only once, and can either ignore it (taking the current state), or
> report a bug. Warnings will not annoy him when he debug his code.
>
> In contrary, the DeprecationWarning was emitted every time when you import
> the md5 or sha modules.
>
> Professional applications likely already use checkers which caught this
> error. This warning will help non-professional applications distributed as
> a single script or handful of scripts. Users of such application often seat
> near its author. In many cases the only user is its author.
>
> ----------
>
> _______________________________________
> Python tracker <report@bugs.python.org>
> <https://bugs.python.org/issue34850>
> _______________________________________
>
-- 
--Guido (mobile)
msg326749 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2018-09-30 21:17
New changeset 5fa247d60d4f3f2b8c8ae8cb57363aca234344c2 by Terry Jan Reedy in branch 'master':
bpo-34850: Replace is with == in idlelib.iomenu (GH-9649)
https://github.com/python/cpython/commit/5fa247d60d4f3f2b8c8ae8cb57363aca234344c2
msg326750 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2018-09-30 21:23
To me, this issue is about unnecessary dependence on implementation details, with the particular example being 'is' versus '=='.  Perhaps PEP8, Programming Recommendations, should have a new subsection 'Implementation Dependencies' to recommend against such dependency when not necessary.

Although IDLE depends on CPython's tkinter, I agree that it should follow this principle.  I extracted the idlelib change to PR9649 to be applied and backported regardless of the fate of Serhiy's main proposal.

At least on Windows, "assert (0, 'bad')" raises SyntaxWarning in freshly compiled 3.6.7+ but not in 3.7.1+ or 3.8.

[A separate issue (#34857): the warning is not displayed in IDLE and the warning in the Shell causes looping.]
msg326751 - (view) Author: miss-islington (miss-islington) Date: 2018-09-30 21:35
New changeset 214c0b3d153c4bad14086888b9de0826a7abc083 by Miss Islington (bot) in branch '3.7':
bpo-34850: Replace is with == in idlelib.iomenu (GH-9649)
https://github.com/python/cpython/commit/214c0b3d153c4bad14086888b9de0826a7abc083
msg326752 - (view) Author: miss-islington (miss-islington) Date: 2018-09-30 21:40
New changeset cb0bec37dd8279555bc01fa03a259eaf7dbb6d5d by Miss Islington (bot) in branch '3.6':
bpo-34850: Replace is with == in idlelib.iomenu (GH-9649)
https://github.com/python/cpython/commit/cb0bec37dd8279555bc01fa03a259eaf7dbb6d5d
msg326754 - (view) Author: Nathaniel Smith (njs) * (Python committer) Date: 2018-09-30 22:24
Would it make sense to implement a "chaos" mode (that e.g. testing tools could enable unconditionally), that disables the small integer and small string caches?
msg326755 - (view) Author: Gregory P. Smith (gregory.p.smith) * (Python committer) Date: 2018-10-01 01:39
If we were to ship a "chaos" mode in the CPython interpreter itself, I assume you envision an interpreter flag and/or env var?  If it required someone compiling the interpreter a special way I don't think it would be widely adopted within continuous integration testing systems.

I was actually pondering doing a "chaos" mode of sorts at work because I have the power to cause everyone's tests to be run that way mode by default.  (I'd basically just disable interning and str/int singletons)  But while it's a nice idea, it's low on my work priorities.  While we had thousands of is literal comparisons that we're about to fix en-masse, they are only a tiny fraction of all literal comparisons in our codebase.  And pylint is now more widely used which should help prevent new ones.
msg326759 - (view) Author: Nathaniel Smith (njs) * (Python committer) Date: 2018-10-01 02:32
Yeah, something like that. Or sys.enable_chaos_mode(), that pytest or whoever could call before running tests.
msg326799 - (view) Author: Steven D'Aprano (steven.daprano) * (Python committer) Date: 2018-10-01 13:33
On Sun, Sep 30, 2018 at 10:24:41PM +0000, Nathaniel Smith wrote:
> Would it make sense to implement a "chaos" mode (that e.g. testing 
> tools could enable unconditionally), that disables the small integer 
> and small string caches?

I'm not really sure that "chaos" is a good name for that. Contrast 
the rather restricted scope ("disable caches") with this example of 
chaos:

https://en.wikipedia.org/wiki/Chaos_Monkey
msg326803 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2018-10-01 14:34
Consider moving the chaos discussion to a new issue.

On Mon, Oct 1, 2018 at 6:33 AM Steven D'Aprano <report@bugs.python.org>
wrote:

>
> Steven D'Aprano <steve+python@pearwood.info> added the comment:
>
> On Sun, Sep 30, 2018 at 10:24:41PM +0000, Nathaniel Smith wrote:
> > Would it make sense to implement a "chaos" mode (that e.g. testing
> > tools could enable unconditionally), that disables the small integer
> > and small string caches?
>
> I'm not really sure that "chaos" is a good name for that. Contrast
> the rather restricted scope ("disable caches") with this example of
> chaos:
>
> https://en.wikipedia.org/wiki/Chaos_Monkey
>
> ----------
>
> _______________________________________
> Python tracker <report@bugs.python.org>
> <https://bugs.python.org/issue34850>
> _______________________________________
>
-- 
--Guido (mobile)
msg326841 - (view) Author: Steven D'Aprano (steven.daprano) * (Python committer) Date: 2018-10-02 00:46
Please move the "chaos" discussion to #34867
msg326861 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2018-10-02 06:48
> I like this solution of a syntax warning for `is <literal>` 
> and I agree that `== None` should not be a warning.

+1 for me as well.  Besides catching potential bugs, it will be of instant benefit for teaching Python.
msg327074 - (view) Author: Neil Schemenauer (nascheme) * (Python committer) Date: 2018-10-04 18:23
> The problem with a SyntaxWarning is that the wrong people will see it. It gets in the way of users of applications that happen to be written in Python.

Turn the check on only when PYTHONDEVMODE is set?  Seems like it solves the issue with the wrong people seeing the warning.
msg327080 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2018-10-04 19:59
> Turn the check on only when PYTHONDEVMODE is set?

This will reduce the value of this warning to zero. If the user doesn't know that using "is" with string or numerical literals is bad and doesn't use checkers, it is unlikely that he uses PYTHONDEVMODE.
msg328852 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2018-10-29 18:36
Gregory, do you still have objections against adding these warnings?
msg330769 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2018-11-30 11:09
Ping.
msg330825 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2018-11-30 20:28
I like this proposal better than a separate test mode.  But users will see compiler warnings for main modules and for packages recompiled for a new version or for a maintenance releases with a byte code bump, and for unprecompiled packages on systems that do not allow .pyc caching.

The first will not matter for packages with minimal main modules.  Recompiles will be rare.  I suspect that the 3rd possibility is also rare.
msg330831 - (view) Author: Nathaniel Smith (njs) * (Python committer) Date: 2018-11-30 22:55
Would it be more acceptable to use a DeprecationWarning? It's not really the right thing because we're not planning to ever actually remove the functionality. But, we've already gone to a lot of trouble to try to show DeprecationWarnings specifically to devs and not end users (hiding them by default, except if the source was the REPL or a test, getting patches into every test framework to handle this, etc.). And the issues here seem to be identical.

Or maybe LintWarning inherits from DeprecationWarning? Or the share a common superclass?
msg330834 - (view) Author: Gregory P. Smith (gregory.p.smith) * (Python committer) Date: 2018-11-30 23:10
Given that linters like pylint can already detect the common case of this issue when using `is` and `is not` to compare to a literal where == or != seems more appropriate, I don't think a warning is very useful.

In my experience people are more likely to run code through a linter than they are to ever run an interpreter with DeprecationWarning enabled.

I do like the concept of using a not visible by default warning, but I don't think adding a LintWarning or misusing DeprecationWarning adds much value.

I suggest closing this issue as won't fix / not a bug.
msg330836 - (view) Author: Steven D'Aprano (steven.daprano) * (Python committer) Date: 2018-12-01 00:59
Looks like we have a stand-off between core devs, and no BDFL to make a ruling.

There is support for the feature from at least Barry and Raymond  (although I think Guido was cool on the idea, maybe?). Gregory, do you feel strongly enough about this that you would reverse the commit if Serhiy just went ahead and committed it? Nobody wants a revert war :-)

To me, this seems like a low-cost feature that will help some developers without inconveniencing anyone. As Serhiy points out, SyntaxWarning is only emitted when the code is compiled, so it is only going to be visible to the developer of the module, not the user of the module.

Let's not allow the perfect be the enemy of the good enough. I think this is a helpful feature for developers, and as Raymond says it will help teach beginners the difference between `is` and equality. The people who are most prone to making this mistake are the ones least likely to be using a linter. And we know it is helpful: Serhiy already used it to find a bug in IDLE.

Gregory, you wanted to close this as not a bug, but nobody says it is a bug, it is an enhancement. And I think there is consensus that this *is* an enhancement. Nobody has said that there are good use cases for using `is` on literals in production code.
msg330837 - (view) Author: Barry A. Warsaw (barry) * (Python committer) Date: 2018-12-01 01:21
I'm actually fine either way.  Consider me a solid ±0
msg330838 - (view) Author: Nathaniel Smith (njs) * (Python committer) Date: 2018-12-01 01:44
> In my experience people are more likely to run code through a linter than they are to ever run an interpreter with DeprecationWarning enabled.

This used to be true, and it was a disaster. So there's been a lot of work to fix it, and it's not true anymore.

Starting in 3.7, the built-in REPL has DeprecationWarning enabled by default, as do all standalone scripts (see PEP 565).

IPython/Jupyter has enabled DeprecationWarning by default for interactive code for about 3 years now: https://github.com/ipython/ipython/issues/8478

pytest started showing DeprecationWarnings by default a few months ago, and unittest has done so for ages.

I don't think I've ever met someone who skipped straight to linting, without ever having used a REPL, or written a script, or written a test :-)
msg330845 - (view) Author: Gregory P. Smith (gregory.p.smith) * (Python committer) Date: 2018-12-01 09:22
Oh neat, I didn't realize that Nathaniel.  That makes such warnings much more useful.

I'm fine if you go ahead with this change.  In the unlikely event it turns out to cause user annoyance problems in betas due to third party code that can't be updated, we can reconsider.
msg333461 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2019-01-11 11:52
If you are fine with this change Gregory, do you mind to withdraw your request and remove the DO-NOT-MERGE label on GitHub?

As for DeprecationWarning vs SyntaxWarning:

1. It looks as misuse of DeprecationWarning. We do not plan to remove this feature in future. Emitting it will send a false signal to users. And SyntaxWarning was added such kind of warning: syntactically valid code that for sure is an error.

2. Seems there is no differences between DeprecationWarning and SyntaxWarning if they are emitted at compile time. They will be shown by default in all cases, in the main script and in modules.
msg333923 - (view) Author: Gregory P. Smith (gregory.p.smith) * (Python committer) Date: 2019-01-18 01:09
Lets move forward with this as a SyntaxWarning in 3.8 and see if anyone complains during the betas.
msg333941 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2019-01-18 05:47
New changeset 3bcbedc9f1471d957a30a90f9d1251516b422416 by Serhiy Storchaka in branch 'master':
bpo-34850: Emit a warning for "is" and "is not" with a literal. (GH-9642)
https://github.com/python/cpython/commit/3bcbedc9f1471d957a30a90f9d1251516b422416
msg333942 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2019-01-18 05:48
Thanks.
msg334145 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2019-01-21 15:19
The warning is emited twice, see: bpo-35798.
History
Date User Action Args
2019-01-21 15:19:17vstinnersetnosy: + vstinner
messages: + msg334145
2019-01-18 05:48:42serhiy.storchakasetstatus: open -> closed
resolution: fixed
messages: + msg333942

stage: patch review -> resolved
2019-01-18 05:47:52serhiy.storchakasetmessages: + msg333941
2019-01-18 01:09:22gregory.p.smithsetmessages: + msg333923
2019-01-11 11:52:24serhiy.storchakasetmessages: + msg333461
2018-12-01 09:22:41gregory.p.smithsetmessages: + msg330845
2018-12-01 01:45:00njssetmessages: + msg330838
2018-12-01 01:21:02barrysetmessages: + msg330837
2018-12-01 00:59:28steven.dapranosetmessages: + msg330836
2018-11-30 23:10:18gregory.p.smithsetmessages: + msg330834
2018-11-30 22:55:54njssetmessages: + msg330831
2018-11-30 21:18:40gvanrossumsetnosy: - gvanrossum
2018-11-30 20:28:18terry.reedysetmessages: + msg330825
2018-11-30 11:09:11serhiy.storchakasetmessages: + msg330769
2018-10-29 18:36:31serhiy.storchakasetmessages: + msg328852
2018-10-04 19:59:07serhiy.storchakasetmessages: + msg327080
2018-10-04 18:23:24naschemesetnosy: + nascheme
messages: + msg327074
2018-10-02 06:48:34rhettingersetnosy: + rhettinger
messages: + msg326861
2018-10-02 00:46:54steven.dapranosetmessages: + msg326841
2018-10-01 14:34:15gvanrossumsetmessages: + msg326803
2018-10-01 13:33:57steven.dapranosetmessages: + msg326799
2018-10-01 09:27:41jwilksetnosy: + jwilk
2018-10-01 02:32:44njssetmessages: + msg326759
2018-10-01 01:39:36gregory.p.smithsetmessages: + msg326755
2018-09-30 22:24:41njssetnosy: + njs
messages: + msg326754
2018-09-30 21:41:00miss-islingtonsetmessages: + msg326752
2018-09-30 21:35:57miss-islingtonsetnosy: + miss-islington
messages: + msg326751
2018-09-30 21:23:18terry.reedysetassignee: terry.reedy -> serhiy.storchaka
messages: + msg326750
components: - IDLE
2018-09-30 21:17:37miss-islingtonsetpull_requests: + pull_request9042
2018-09-30 21:17:30miss-islingtonsetpull_requests: + pull_request9041
2018-09-30 21:17:21terry.reedysetmessages: + msg326749
2018-09-30 20:38:24terry.reedysetpull_requests: + pull_request9040
2018-09-30 18:41:05gvanrossumsetmessages: + msg326744
2018-09-30 18:28:54barrysetmessages: + msg326741
2018-09-30 17:02:10serhiy.storchakasetmessages: + msg326734
2018-09-30 16:26:29gregory.p.smithsetmessages: + msg326731
2018-09-30 15:33:55xiang.zhangsetnosy: + xiang.zhang
messages: + msg326729
2018-09-30 12:45:20serhiy.storchakasetmessages: + msg326722
2018-09-30 12:28:58steven.dapranosetnosy: + steven.daprano
messages: + msg326721
2018-09-30 08:32:27serhiy.storchakasetnosy: + terry.reedy
messages: + msg326715

assignee: terry.reedy
components: + IDLE
2018-09-30 08:30:50serhiy.storchakasetkeywords: + patch
stage: patch review
pull_requests: + pull_request9034
2018-09-30 08:15:19serhiy.storchakacreate