-
-
Notifications
You must be signed in to change notification settings - Fork 31k
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
gh-103492: Clarify SyntaxWarning with literal comparison #103493
Changes from 8 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; | ||||||
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.
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 at
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.
I might be missing something, but
bool 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 be
is
, e.g.x == 3 is y
, but let me know if I'm missing something!