Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

bpo-27494: Fix a 2to3 parser bug (trailing comma)#60

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

Closed
jstasiak wants to merge1 commit intopython:masterfromjstasiak:fix-lib2to3

Conversation

jstasiak
Copy link
Contributor

@jstasiakjstasiak commentedFeb 13, 2017•
edited by bedevere-bot
Loading

While this is a valid Python 2 and Python 3 syntax lib2to3 would choke
on it:

set(xforxin [],)

This patch changes the grammar definition used by lib2to3 so that the
actual Python syntax is supported now and backwards compatibility is
preserved.

https://bugs.python.org/issue27494

stuarteberg reacted with thumbs up emoji
@codecov
Copy link

codecovbot commentedFeb 13, 2017

Codecov Report

Merging#60 intomaster willincrease coverage by<.01%.
The diff coverage is100%.

@@            Coverage Diff             @@##           master      #60      +/-   ##==========================================+ Coverage   82.37%   82.37%   +<.01%==========================================  Files        1427     1427                Lines      350948   350951       +3     ==========================================+ Hits       289091   289099       +8+ Misses      61857    61852       -5

Continue to review full report at Codecov.

Legend -Click here to learn more
Δ = absolute <relative> (impact),ø = not affected,? = missing data
Powered byCodecov. Last update649a7ca...d7f230d. Read thecomment docs.

Copy link
Contributor

@stuartebergstuarteberg left a comment•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I'm not a core dev, but I took some time to review this anyway. (I recently familiarized myself withlib2to3 in order to make adifferent PR -- reviews welcome.)

Background:

This PR changes thelib2to3 grammar's handling offor syntax, to distinguish between the following two cases:

  1. list comprehensions and generator expressions, e.g.

    • [x for x in foo]
    • (x for x in foo)

  2. set and dictionary comprehensions, e.g.

    • {x for x in foo}
    • {x : y for x,y in zip(foo,bar)}

Previously, the grammar used the same node type for those two cases. Unfortunately, that node specification is overly complicated due to backwards compatibility considerations. As a consequence, the grammar failed to handle some valid programs.

Review:

LGTM. The problematic case is indeed fixed, and now covered by the test suite. The three "fixers" that are affected by this Grammar change have been updated. (I see no others that need editing.)

# was created specifically to handle list comprehensions and generator expressions
# enclosed with parentheses I believe it's safe to only use it in those. That
# avoids the ambiguity mentioned above.
#
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I agree with the need for a verbose comment here, since it's not immediately obvious whyold_comp_iter, etc. are necessary. IMHO, you should remove overly cautious words like "seems" and "I believe". Also, I think it might be worth adding a reference to this PR and/orbpo-27494 in the comment itself.

Copy link
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Thanks for the feedback, it makes sense; let me improve this a little.

@jstasiak
Copy link
ContributorAuthor

Yes, just to be precise:

This PR changes the lib2to3 grammar's handling of for syntax, to distinguish between the following two cases:

  1. list comprehensions and generator expressions, e.g. (...)
  2. set and dictionary comprehensions, e.g (...)

The second group would be set/dictionary comprehensions and call arguments (likeprint(x for x in [],))

stuarteberg reacted with thumbs up emoji

@jstasiak
Copy link
ContributorAuthor

cc@benjaminp@serhiy-storchaka@vadmium@gpshead@1st1 (I looked up who changedlib/lib2to3 recently, apologies for nagging)

I'm happy to make any changes necessary to get this merged, I just need to know what's required.

@jstasiak
Copy link
ContributorAuthor

jstasiak commentedJul 17, 2017•
edited
Loading

@ambv is this something you could review? I'm asking you because I see you're an author of another 2to3 patch (#1724)

@ambvambv self-requested a reviewJuly 17, 2017 22:36
akruis pushed a commit to akruis/cpython that referenced this pull requestSep 9, 2017
This commit adds a test case to trigger an assertion failure during shutdown andfixes the bug. The assertion was added by my first attempt to fix issuepython#60.Thanks to Masamitsu Murase for reporting the bug and for providing the fix.(grafted from 655f2a2292a237636849b4a82fb0a2dde1ee9847 and 109d5d067b14)
@jstasiak
Copy link
ContributorAuthor

Closed by accident, apologies.

@jstasiakjstasiak reopened thisSep 26, 2017
@jstasiakjstasiakforce-pushed thefix-lib2to3 branch 3 times, most recently from1818dc6 to4ae43d5CompareSeptember 26, 2017 09:00
…essionWhile this is a valid Python 2 and Python 3 syntax lib2to3 would chokeon it:    set(x for x in [],)This patch changes the grammar definition used by lib2to3 so that theactual Python syntax is supported now and backwards compatibility ispreserved.
@jstasiak
Copy link
ContributorAuthor

OK, I'm lost regarding Travis build failures. It seems that the configuration that Travis uses to build the docs (https://travis-ci.org/python/cpython/jobs/279869243/config) is different than in builds of some more recent PR-s (https://travis-ci.org/python/cpython/jobs/279831444/config for example).

I suspect for whatever reason it's not picking up updates to .travis.yml (made since I created my branch even though I pushed them to the branch now) and that makes it fail.

One thing I suspect may work around this is closing this PR and opening a new one...

@jstasiak
Copy link
ContributorAuthor

I gave up on trying to make this pull request work with Travis and created another one:#3771. The new one has been actually built successfully now!

Mr3x33 added a commit to Mr3x33/cpython that referenced this pull requestSep 17, 2021
colesbury referenced this pull request in colesbury/nogilOct 6, 2021
@gvanrossumgvanrossum mentioned this pull requestApr 10, 2022
jaraco pushed a commit that referenced this pull requestDec 2, 2022
jaraco added a commit to jaraco/cpython that referenced this pull requestFeb 17, 2023
* Update .coveragerc* Keep whitespace consistent.Co-authored-by: Jason R. Coombs <jaraco@jaraco.com>
isidentical pushed a commit to isidentical/cpython that referenced this pull requestMar 28, 2023
oraluben pushed a commit to oraluben/cpython that referenced this pull requestJul 12, 2023
* Fix: Correct target_bb_id in jump array* Doc: Updated doc for add_metadata_to_jump_2d_array
AA-Turner pushed a commit to AA-Turner/cpython that referenced this pull requestApr 10, 2025
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@stuartebergstuartebergstuarteberg approved these changes

@ambvambvAwaiting requested review from ambv

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

3 participants
@jstasiak@stuarteberg@the-knights-who-say-ni
[8]ページ先頭
©2009-2025 Movatter.jp