gh-103492: Clarify SyntaxWarning with literal comparison #103493
Conversation
hauntsaninja
commented
Apr 13, 2023
•
hauntsaninja
commented
- Issue: Clarify SyntaxWarning with literal comparison #103492
Misc/NEWS.d/next/Core and Builtins/2023-04-13-00-58-55.gh-issue-103492.P4k0Ay.rst
Outdated
Show resolved
Hide resolved
Lib/test/test_codeop.py
Outdated
| @@ -277,7 +277,7 @@ def test_filename(self): | |||
| def test_warning(self): | |||
| # Test that the warning is only returned once. | |||
| with warnings_helper.check_warnings( | |||
| ('"is" with a literal', SyntaxWarning), | |||
| ('"is" with str literal', SyntaxWarning), | |||
There was a problem hiding this comment.
would it be clearer to have "is" with literal of type 'str'?
Note that types are often in `:
>>> 1+'d'
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
TypeError: unsupported operand type(s) for +: 'int' and 'str'
There was a problem hiding this comment.
But consistently with only double quotes, or only single quotes?
There was a problem hiding this comment.
Thanks, I've added quotes to the types. I kept the phrase 'str' literal instead of literal of type 'str', since I found it a little easier to read and is similar to the way we just mention the types in the add example you give (instead of e.g. saying value of type 'int' and value of type 'str')
I'm happy with whatever quote type, let me know!
Python/compile.c
Outdated
| : "\"is not\" with '%.200s' literal. Did you mean \"!=\"?"; | ||
| expr_ty literal = !left ? e->v.Compare.left : right_expr; | ||
| return compiler_warn( | ||
| c, LOC(e), msg, Py_TYPE(literal->v.Constant.value)->tp_name |
There was a problem hiding this comment.
| c, LOC(e), msg, Py_TYPE(literal->v.Constant.value)->tp_name | |
| c, LOC(e), msg, infer_type(literal)->tp_name |
There was a problem hiding this comment.
I had this originally, but it needed a forward reference / we know it's a Constant_kind so we could skip the switch :-)
I'll add it back!
There was a problem hiding this comment.
Addressed, let me know if I should move the function instead
Python/compile.c
Outdated
| return compiler_warn(c, LOC(e), msg); | ||
| ? "\"is\" with '%.200s' literal. Did you mean \"==\"?" | ||
| : "\"is not\" with '%.200s' literal. Did you mean \"!=\"?"; | ||
| expr_ty literal = !left ? e->v.Compare.left : right_expr; |
There was a problem hiding this comment.
left gets updated in each iteration, but e->v.Compare.left is always the same thing. Is this correct?
Should we just check the first item before the loop and then here just look at right_expr?
There was a problem hiding this comment.
I might be missing something, but bool left = check_is_arg(e->v.Compare.left); happens once outside the loop and it's only right that is updated in each iteration.
There was a problem hiding this comment.
Ah, of course! Thank you so much, this is buggy. I added the fix and a test case to catch the issue.
I think we can't do the first item check before the loop since all comparators may not be is, e.g. x == 3 is y, but let me know if I'm missing something!
|
Thank you for the review! |
* main: pythongh-100227: Only Use deepfreeze for the Main Interpreter (pythongh-103794) pythongh-103492: Clarify SyntaxWarning with literal comparison (python#103493) pythongh-101100: Fix Sphinx warnings in `argparse` module (python#103289)
* superopt: pythongh-100227: Only Use deepfreeze for the Main Interpreter (pythongh-103794) pythongh-103492: Clarify SyntaxWarning with literal comparison (python#103493) pythongh-101100: Fix Sphinx warnings in `argparse` module (python#103289)
