-
-
Notifications
You must be signed in to change notification settings - Fork 32.3k
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 6 commits
5247c04
12ce76c
bc6d13f
d0f2ea3
9a13700
41c9ad9
30b29bb
8f16b1a
b7b4f1a
20d0554
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
Clarify :exc:`SyntaxWarning` with literal ``is`` comparison by specifying which literal is problematic, since comparisons using ``is`` with e.g. None and bool literals are idiomatic. |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -2280,13 +2280,17 @@ check_compare(struct compiler *c, expr_ty e) | |||||
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); | ||||||
bool right = check_is_arg((expr_ty)asdl_seq_GET(e->v.Compare.comparators, i)); | ||||||
expr_ty right_expr = (expr_ty)asdl_seq_GET(e->v.Compare.comparators, i); | ||||||
bool right = check_is_arg(right_expr); | ||||||
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 \"!=\"?"; | ||||||
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 commentThe reason will be displayed to describe this comment to others. Learn more.
Should we just check the first item before the loop and then here just look at There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I might be missing something, but There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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 be |
||||||
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 commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. Addressed, let me know if I should move the function instead |
||||||
); | ||||||
} | ||||||
} | ||||||
left = right; | ||||||
|
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 `:
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?
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!