GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — gh-103492: Clarify SyntaxWarning with literal comparison by hauntsaninja · Pull Request #103493 · python/cpython

Conversation

hauntsaninja commented Apr 13, 2023 •

edited by bedevere-bot

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Contributor

Issue: Clarify SyntaxWarning with literal comparison #103492

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions


          pythongh-103492: Clarify SyntaxWarning with literal comparison

5247c04

bedevere-bot mentioned this pull request

Apr 13, 2023

Clarify SyntaxWarning with literal comparison #103492

Closed

bedevere-bot added the awaiting core review label

Apr 13, 2023

hauntsaninja and others added 2 commits

April 12, 2023 17:55

12ce76c


          📜🤖 Added by blurb_it.

bc6d13f

hauntsaninja marked this pull request as ready for review

April 13, 2023 00:59

hauntsaninja requested review from iritkatriel and markshannon as code owners

April 13, 2023 00:59

d0f2ea3

hauntsaninja commented

Apr 13, 2023

View reviewed changes

Comment thread

Misc/NEWS.d/next/Core and Builtins/2023-04-13-00-58-55.gh-issue-103492.P4k0Ay.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

hauntsaninja added 2 commits

April 12, 2023 18:49


          Update Misc/NEWS.d/next/Core and Builtins/2023-04-13-00-58-55.gh-issu…

9a13700

…e-103492.P4k0Ay.rst


          Merge branch 'main' into pythongh-103492

41c9ad9

sobolevn approved these changes

Apr 13, 2023

View reviewed changes

sobolevn left a comment

Copy link

Copy Markdown

Member

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!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

iritkatriel reviewed

Apr 14, 2023

View reviewed changes

Comment thread

Lib/test/test_codeop.py Outdated

		# Test that the warning is only returned once.
		with warnings_helper.check_warnings(
		('"is" with a literal', SyntaxWarning),
		('"is" with str literal', SyntaxWarning),

iritkatriel Apr 14, 2023

Copy link

Copy Markdown

Member

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'

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

hugovk Apr 19, 2023

Copy link

Copy Markdown

Member

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

hauntsaninja Apr 24, 2023 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Contributor Author

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 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!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

hauntsaninja added 2 commits

April 19, 2023 12:13


          single quote types

30b29bb


          simplify warning code (tested with and without -W error)

8f16b1a

iritkatriel reviewed

Apr 24, 2023

View reviewed changes

Comment thread

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

iritkatriel Apr 24, 2023

Copy link

Copy Markdown

Member

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.

Suggested change

	c, LOC(e), msg, Py_TYPE(literal->v.Constant.value)->tp_name
	c, LOC(e), msg, infer_type(literal)->tp_name

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

hauntsaninja Apr 24, 2023

Copy link

Copy Markdown

Contributor Author

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!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

hauntsaninja Apr 24, 2023

Copy link

Copy Markdown

Contributor Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

iritkatriel reviewed

Apr 24, 2023

View reviewed changes

Comment thread

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;

iritkatriel Apr 24, 2023

Copy link

Copy Markdown

Member

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, 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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

hauntsaninja Apr 24, 2023

Copy link

Copy Markdown

Contributor Author

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 only right that is updated in each iteration.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

iritkatriel Apr 24, 2023

Copy link

Copy Markdown

Member

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

hauntsaninja Apr 24, 2023

Copy link

Copy Markdown

Contributor Author

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!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

hauntsaninja added 2 commits

April 24, 2023 14:13


          fix bug irit mentions, add test

b7b4f1a


          use infer_type

20d0554

iritkatriel reviewed

Apr 24, 2023

View reviewed changes

Comment thread

Python/compile.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

iritkatriel approved these changes

Apr 24, 2023

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting core review labels

Apr 24, 2023

hauntsaninja commented Apr 24, 2023

Copy link

Copy Markdown

Contributor Author

Thank you for the review!

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

hauntsaninja merged commit ae25855 into python:main

Apr 24, 2023

bedevere-bot removed the awaiting merge label

Apr 24, 2023

carljm added a commit to carljm/cpython that referenced this pull request

Apr 24, 2023


          Merge branch 'main' into superopt

dbe1665

* 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)

carljm added a commit to carljm/cpython that referenced this pull request

Apr 24, 2023


          Merge branch 'superopt' into superopt_spec

793a69a

* 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)

hauntsaninja deleted the gh-103492 branch

April 29, 2023 07:53

qthequartermasterman mentioned this pull request

Nov 19, 2025

SyntaxWarning/SyntaxError upon import in Python>=3.12 cenpy-devs/cenpy#165

Closed

Reviewers

hugovk

hugovk left review comments

iritkatriel

iritkatriel approved these changes

sobolevn

sobolevn approved these changes

markshannon

Awaiting requested review from markshannon

markshannon is a code owner

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

5 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

Conversation

hauntsaninja commented Apr 13, 2023 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

sobolevn left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hauntsaninja Apr 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

hauntsaninja commented Apr 24, 2023

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

hauntsaninja commented Apr 13, 2023 •

edited by bedevere-bot

Loading

Uh oh!

There was an error while loading. Please reload this page.

hauntsaninja Apr 24, 2023 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.