Emit a syntax warning for "is" with a literal #79031
Comments
|
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: 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. |
serhiy-storchaka
added
3.8 (EOL)
|
It also fixes an incorrect use of "is" with an empty string in IDLE. |
|
I like this solution of a syntax warning for (And for what its worth, I strongly disagree with making |
|
Suggestions about the wording of the warning message are welcome. |
|
I have catched using 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. |
|
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. |
|
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. |
|
This is a nice approach to the problem. I completely agree that we cannot change |
|
Yet, Greg’s point is that this only works if the developer tests their code I’m not sure that his proposal is better though. I think static checkers On Sun, Sep 30, 2018 at 10:02 AM Serhiy Storchaka <report@bugs.python.org>
-- |
|
To me, this issue is about unnecessary dependence on implementation details, with the particular example being 'is' versus '=='. Perhaps PEP-8, 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 (bpo-34857): the warning is not displayed in IDLE and the warning in the Shell causes looping.] |
|
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? |
|
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. |
|
Yeah, something like that. Or sys.enable_chaos_mode(), that pytest or whoever could call before running tests. |
|
On Sun, Sep 30, 2018 at 10:24:41PM +0000, Nathaniel Smith wrote:
I'm not really sure that "chaos" is a good name for that. Contrast |
|
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>
-- |
|
Please move the "chaos" discussion to bpo-34867 |
+1 for me as well. Besides catching potential bugs, it will be of instant benefit for teaching Python. |
Turn the check on only when PYTHONDEVMODE is set? Seems like it solves the issue with the wrong people seeing the warning. |
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. |
|
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. |
|
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? |
|
Given that linters like pylint can already detect the common case of this issue when using 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. |
|
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 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 |
|
I'm actually fine either way. Consider me a solid ±0 |
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: ipython/ipython#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 :-) |
|
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. |
|
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:
|
|
Lets move forward with this as a SyntaxWarning in 3.8 and see if anyone complains during the betas. |
|
Thanks. |
|
The warning is emited twice, see: bpo-35798. |
|
Should this also produce warnings for |
|
(message from a code of conduct violating troll deleted) |
How did Bachsau violate the code of conduct, and what evidence do you From where I am sitting, the only person violating the CoC is you It is not open to criticism: in #msg333923 Gregory wrote "let's see if Think about the message that sends about our openess to criticism. Its not considerate: Bachsau took the time to comment about something Backsau attacked the feature. Instead of assuming good faith until Being called a troll is not a small thing: it is an accusation that the Accusations of trolling should only be made on the basis of a pattern of Vigourous and passionate disagreement with a feature is not trolling. It |
|
"This is bullshit." struck me as jarring and rude. I will stop there. |
|
Bachsau, to respond to the substance of your comments: it is not the *primary* This is not the first such warning in Python: it has warned on incorrect py> assert (flag, "error") Ideally a language should have no warts, gotchas or features which are If you go back to the very first post in the discussion which launched https://discuss.python.org/t/demoting-the-is-operator-to-avoid-an-identity-crisis/86 As the Zen says, "practicality beats purity" and this is a low-impact Apart from your aesthetic sense that the compiler shouldn't try to Evidence of code that breaks because of this change is especially |
No, because these cases are useless expressions, but they are not hidden bugs. The result is always False and does not rely on the implementation.
SyntaxErrors should be raised on problems with the syntax. SyntaxWarnings *can* be raised for the syntax which is valid but is definitely a bug. |
|
What if using identity equality on integer literals is intended? I'm now trying to speed up the generated code via the integer interning mechanism told by https://docs.python.org/3/c-api/long.html#c.PyLong_FromLong . I think it okay to not complain when the operand of |
|
That would potentially let an invalid usage slip through, since you know what you're doing, you can suppress the warning with: warnings.filterwarnings('ignore', category=SyntaxWarning, message='"is" with a literal') |
|
Thanks. Use following example: import warnings
warnings.filterwarnings('ignore')
warnings.warn(SyntaxWarning("test"))
def f(x):
return x is 5
print(f(5))I succeeded in suppressing my own SyntaxWarning("test") , but the output is still: python a.py |
|
It cannot be suppressed by the code in the module caused a warning, because the warning is emitted at compile time. You need to silence it before compiling that module. Once you have compiled it, warning will no longer be emitted. |
Since python 3.8 identity operators 'is' / 'is not' should only be applied to mutative literals, not the constant ones (string, tupple, integer, float). Proof: python/cpython#79031
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: