Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork32k
gh-103492: Clarify SyntaxWarning with literal comparison#103493
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Misc/NEWS.d/next/Core and Builtins/2023-04-13-00-58-55.gh-issue-103492.P4k0Ay.rst OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Good idea, the code looks correct to me as well :)
Thank you!
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" witha literal', SyntaxWarning), | |||
| ('"is" withstr literal', SyntaxWarning), | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
But consistently with only double quotes, or only single quotes?
hauntsaninjaApr 24, 2023 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Thanks, I've added quotes to the types. I kept the phrase'str' literal instead ofliteral 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. sayingvalue 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
| 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
left gets updated in each iteration, bute->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 atright_expr?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I might be missing something, butbool left = check_is_arg(e->v.Compare.left); happens once outside the loop and it's onlyright that is updated in each iteration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
line 2296?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 beis, e.g.x == 3 is y, but let me know if I'm missing something!
Uh oh!
There was an error while loading.Please reload this page.
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)
Uh oh!
There was an error while loading.Please reload this page.