GV

GeezerVison

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

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — bpo-41520: codeop no longer ignores SyntaxWarning by vstinner · Pull Request #21838 · python/cpython

Conversation

@vstinner

@vstinner vstinner commented Aug 12, 2020 •
edited by bedevere-bot
Loading

Uh oh!

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

Copy link
Copy Markdown
Member

https://bugs.python.org/issue41520

Sorry, something went wrong.

Uh oh!

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

All reactions
@vstinner
bpo-41520: codeop no longer ignores SyntaxWarning
a623add
@the-knights-who-say-ni the-knights-who-say-ni added the CLA signed label Aug 12, 2020
@bedevere-bot bedevere-bot added the awaiting core review label Aug 12, 2020
@vstinner

vstinner commented Aug 12, 2020

Copy link
Copy Markdown
Member Author

cc @csabella @terryjreedy @Carreau @1st1

All reactions

Sorry, something went wrong.

Uh oh!

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

vstinner
vstinner commented Aug 12, 2020
View reviewed changes
Comment thread Lib/codeop.py Resolved

Uh oh!

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

csabella
csabella approved these changes Aug 12, 2020
View reviewed changes

@csabella csabella left a comment

Copy link
Copy Markdown
Contributor

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.

LGTM. @vstinner, thank you for fixing this. 🙇‍♀️

Sorry, something went wrong.

Uh oh!

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

All reactions
@bedevere-bot bedevere-bot added awaiting merge and removed awaiting core review labels Aug 12, 2020
@vstinner vstinner added needs backport to 3.8 labels Aug 12, 2020
@vstinner
vstinner merged commit 369a1cb into python:master Aug 12, 2020
@miss-islington

miss-islington commented Aug 12, 2020

Copy link
Copy Markdown
Contributor

Thanks @vstinner for the PR 🌮🎉.. I'm working now to backport this PR to: 3.8, 3.9.
🐍🍒⛏🤖 I'm not a witch! I'm not a witch!

All reactions

Sorry, something went wrong.

Uh oh!

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

@bedevere-bot bedevere-bot removed the awaiting merge label Aug 12, 2020
@csabella

csabella commented Aug 12, 2020

Copy link
Copy Markdown
Contributor

@Carreau, thank you for catching this.

All reactions

Sorry, something went wrong.

Uh oh!

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

@bedevere-bot bedevere-bot removed the needs backport to 3.9 label Aug 12, 2020
@bedevere-bot

bedevere-bot commented Aug 12, 2020

Copy link
Copy Markdown

GH-21840 is a backport of this pull request to the 3.9 branch.

All reactions

Sorry, something went wrong.

Uh oh!

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

@vstinner
vstinner deleted the codeop_warn branch August 12, 2020 12:53
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Aug 12, 2020
@vstinner @miss-islington
bpo-41520: codeop no longer ignores SyntaxWarning (pythonGH-21838)
f269618
(cherry picked from commit 369a1cb)

Co-authored-by: Victor Stinner <[email protected]>
@bedevere-bot

bedevere-bot commented Aug 12, 2020

Copy link
Copy Markdown

GH-21841 is a backport of this pull request to the 3.8 branch.

All reactions

Sorry, something went wrong.

Uh oh!

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

@bedevere-bot bedevere-bot removed the needs backport to 3.8 label Aug 12, 2020
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Aug 12, 2020
@vstinner @miss-islington
bpo-41520: codeop no longer ignores SyntaxWarning (pythonGH-21838)
3a6cdf3
(cherry picked from commit 369a1cb)

Co-authored-by: Victor Stinner <[email protected]>
@vstinner

vstinner commented Aug 12, 2020

Copy link
Copy Markdown
Member Author

Thanks for the review @csabella!

All reactions

Sorry, something went wrong.

Uh oh!

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

miss-islington added a commit that referenced this pull request Aug 12, 2020
@miss-islington @vstinner
bpo-41520: codeop no longer ignores SyntaxWarning (GH-21838)
afff51f
(cherry picked from commit 369a1cb)

Co-authored-by: Victor Stinner <[email protected]>
miss-islington added a commit that referenced this pull request Aug 12, 2020
@miss-islington @vstinner
bpo-41520: codeop no longer ignores SyntaxWarning (GH-21838)
90eff4e
(cherry picked from commit 369a1cb)

Co-authored-by: Victor Stinner <[email protected]>
terryjreedy
terryjreedy reviewed Aug 12, 2020
View reviewed changes
Comment thread Lib/codeop.py
# to emit SyntaxWarning at most once.
with warnings.catch_warnings():
warnings.simplefilter("ignore")
warnings.simplefilter("error", SyntaxWarning)

@terryjreedy terryjreedy Aug 12, 2020 •
edited
Loading

Uh oh!

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

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.

Ignoring all warnings was intentional. Compile also issues DeprecationWarnings. Narrowing the filter to SyntaxWarning only reintroduces the error of DeprecationWarning being issued thrice, which was fixed by the previous code. I will write more on the issue.

Followup PR fixes regression by removing SyntaxWarning.

Sorry, something went wrong.

Uh oh!

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

All reactions

@Carreau Carreau Aug 12, 2020

Copy link
Copy Markdown
Contributor

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.

Then maybe the catch warnings need to be put in there 3 times, to reset the warning filter on each subsequent compilations ?

Sorry, something went wrong.

Uh oh!

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

All reactions
terryjreedy
terryjreedy reviewed Aug 12, 2020
View reviewed changes
Comment thread Lib/test/test_codeop.py
# bpo-41520: check SyntaxWarning treated as an SyntaxError
with self.assertRaises(SyntaxError):
warnings.simplefilter('error', SyntaxWarning)
compile_command('1 is 1\n', symbol='exec')

@terryjreedy terryjreedy Aug 12, 2020 •
edited
Loading

Uh oh!

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

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.

This source should not have '\n' as single lines are passed without it. That is why to recompile with \n and \n\n added. Fixed in followup PR.

Sorry, something went wrong.

Uh oh!

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

All reactions

@Carreau Carreau Aug 12, 2020

Copy link
Copy Markdown
Contributor

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.

Even w/o the \n 3.8.5 does not raise the SyntaxError.

$ cat foo.py
import warnings
from codeop import compile_command

warnings.simplefilter('error', SyntaxWarning)
res = compile_command('1 is 1', symbol='exec')
print('Res', res)

$ python foo.py
Res None

Sorry, something went wrong.

Uh oh!

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

All reactions

@terryjreedy terryjreedy Aug 12, 2020

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.

I expect that, but aside from your issue, the test if better if it calls compile_command the way it is intended to be used and is used by code.InteractiveInterpreter.

Sorry, something went wrong.

Uh oh!

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

All reactions
@terryjreedy

terryjreedy commented Aug 12, 2020 •
edited by bedevere-bot
Loading

Uh oh!

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

Copy link
Copy Markdown
Member

The title is misleading because before this patch, codeop was not ignoring the first SyntaxWarning. The test we added in bpo-40807 ensured that. The issue, rather, is that _maybe_compile requires 3 SyntaxErrors, not just 1, to raise a SyntaxError. As I note above, the fix to a regression introduces another regression. Fixed in #21848.

All reactions

Sorry, something went wrong.

Uh oh!

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

shihai1991 pushed a commit to shihai1991/cpython that referenced this pull request Aug 20, 2020
@vstinner @shihai1991
bpo-41520: codeop no longer ignores SyntaxWarning (pythonGH-21838)
97c14a7
xzy3 pushed a commit to xzy3/cpython that referenced this pull request Oct 18, 2020
@vstinner @xzy3-cdc
bpo-41520: codeop no longer ignores SyntaxWarning (pythonGH-21838)
a44101b
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@terryjreedy terryjreedy terryjreedy left review comments
@csabella csabella csabella approved these changes
+1 more reviewer
@Carreau Carreau Carreau left review comments
Reviewers whose approvals may not affect merge requirements

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.

7 participants

@vstinner @miss-islington @csabella @bedevere-bot @terryjreedy @Carreau @the-knights-who-say-ni
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.