Skip to content

[3.7] bpo-40807: Show warnings once from codeop._maybe_compile (GH-20486) #20673

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

Merged
merged 3 commits into from
Jun 9, 2020

Conversation

miss-islington
Copy link
Contributor

@miss-islington miss-islington commented Jun 6, 2020

  • bpo-40807: Show warnings once from codeop._maybe_compile

  • Move catch_warnings

  • news

Co-authored-by: Terry Jan Reedy [email protected]
(cherry picked from commit 052d3fc)

Co-authored-by: Cheryl Sabella [email protected]

https://bugs.python.org/issue40807

)

* bpo-40807: Show warnings once from codeop._maybe_compile

* Move catch_warnings

* news

Co-authored-by: Terry Jan Reedy <[email protected]>
(cherry picked from commit 052d3fc)

Co-authored-by: Cheryl Sabella <[email protected]>
@miss-islington
Copy link
Contributor Author

@csabella and @terryjreedy: Status check is done, and it's a failure ❌ .

1 similar comment
@miss-islington
Copy link
Contributor Author

@csabella and @terryjreedy: Status check is done, and it's a failure ❌ .

@terryjreedy
Copy link
Member

With the test.support change backported, the new test runs on 3.7 and fails on
self.assertEqual(len(w.warnings), 1) because '0 is 0' does not cause SyntaxWarning in 3.7. I believe the same might be true of '\e' and DeprecationWarning. We could make the test pass by changing 1 to 0, but that would be equivalent to deleting the test, and the blurb would still be wrong.

I think we should hold off on this until we learn of code that will make the current test (and blurb) relevant, or until we are sure that there is no such code, or until the next and final 3.7 bugfix release (in about a month).

Copy link
Member

@terryjreedy terryjreedy left a comment

Choose a reason for hiding this comment

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

See previous comment. Either change test with relevant code, so it passes, or close upon deciding there is no such code or 3.7 goes into security mode.

def test_warning(self):
# Test that the warning is only returned once.
with support.check_warnings((".*invalid", DeprecationWarning)) as w:
compile_command("\e")
Copy link
Member

@terryjreedy terryjreedy Jun 9, 2020

Choose a reason for hiding this comment

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

You forgot that the outer quotes are not part of the compiled syntax. (I have done this too.) \e is a syntax error, '\e' is valid and gives no warning in 3.7.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry about that. I was actually having trouble getting the '\e' to show the Deprecation Warning on any release until I started the REPL or IDLE with the ./python -Wall. I did this change quickly just to see how the tests would be run and it looks like I did it too quickly. :-(

Copy link
Member

Choose a reason for hiding this comment

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

The important point is '\e' warns when starting IDLE (legitimately) with py -3.7 -Wall -m idlelib. So the backport is needed.

Should the tests always pass, not just with CI? At least if one does not explicitly turn off warnings for tests (which seems like a bad idea)? I made a local 3.7 branch from this PR and ran python -m test test_codeop and it passed normally. python -m test.test_codeop reports 5 tests rather than the previous 4 and prints the DeprecationWarning.

So I will merge and we will see how this fares on buildbots.

Copy link
Member

Choose a reason for hiding this comment

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

20 seconds later I realize that warnings <= 1 is sufficient to prove not 3 or even 2. But 0 could also result from a bad patch that suppressed too much. So if a fix is needed, better to add a context forcing the warning on.

@csabella csabella requested a review from terryjreedy June 9, 2020 10:43
@miss-islington miss-islington merged commit 4b378ac into python:3.7 Jun 9, 2020
@miss-islington miss-islington deleted the backport-052d3fc-3.7 branch June 9, 2020 12:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants