Skip to content
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.

Sign up for GitHub

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

Jump to bottom

gh-119049: Fix incorrect usage of GET_WARNINGS_ATTR #119063

Merged
merged 8 commits into from
May 16, 2024
Merged

gh-119049: Fix incorrect usage of GET_WARNINGS_ATTR #119063

merged 8 commits into from
May 16, 2024

Conversation

Eclips4
Copy link
Member

@Eclips4 Eclips4 commented May 15, 2024 •
edited
Loading

GET_WARNINGS_ATTR is trying to get the ATTR attribute of the Python warnings module. If the third parameter is true, it tries to import the warnings module if it hasn't been imported already.
All of the calls to call_show_warning are made with the source = NULL argument, so this statement show_fn = GET_WARNINGS_ATTR(interp, _showwarnmsg, source != NULL); always becomes show_fn = GET_WARNINGS_ATTR(interp, _showwarnmsg, 0);.
So it never triess to import the warnings module if it has not already imported, which leads us to the issue described in the #119049.
This is why test_io.test_check_encoding_warning works when import warnings is used in pathlib._local.py and fails when we remove this statement.

@bedevere-app bedevere-app bot mentioned this pull request May 15, 2024
Closed
serhiy-storchaka
serhiy-storchaka approved these changes May 15, 2024
View reviewed changes
Copy link
Member

@serhiy-storchaka serhiy-storchaka left a comment

Choose a reason for hiding this comment

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

LGTM.

Modules/_testcapimodule.c Outdated Show resolved Hide resolved
serhiy-storchaka
serhiy-storchaka reviewed May 15, 2024
View reviewed changes
Copy link
Member

@serhiy-storchaka serhiy-storchaka left a comment

Choose a reason for hiding this comment

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

On other hand, what happens when it is called at the late stage of Python shutdown, when import does not work? Does it emit any warning or raise an exception?

@Eclips4
Copy link
Member Author

Eclips4 commented May 15, 2024 •
edited
Loading

On other hand, what happens when it is called at the late stage of Python shutdown, when import does not work? Does it emit any warning or raise an exception?

No, it's just omit the source in the warning. Should we emit warning or raise a exception?

@serhiy-storchaka
Copy link
Member

How difficult is to write a test for this case?

@Eclips4
Copy link
Member Author

Eclips4 commented May 16, 2024

How difficult is to write a test for this case?

It's not difficult.

@Eclips4 Eclips4 requested a review from serhiy-storchaka May 16, 2024 10:32
serhiy-storchaka
serhiy-storchaka reviewed May 16, 2024
View reviewed changes
Copy link
Member

@serhiy-storchaka serhiy-storchaka left a comment

Choose a reason for hiding this comment

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

Thank you for the new test. Even if its result was not affected by this change, it is nice to check this.

Lib/test/test_capi/test_exceptions.py Outdated Show resolved Hide resolved
serhiy-storchaka
serhiy-storchaka reviewed May 16, 2024
View reviewed changes
Lib/test/test_capi/test_exceptions.py Outdated Show resolved Hide resolved
Lib/test/test_capi/test_exceptions.py Outdated Show resolved Hide resolved
Lib/test/test_capi/test_exceptions.py Outdated Show resolved Hide resolved
Eclips4 and others added 3 commits May 16, 2024 22:18
serhiy-storchaka
serhiy-storchaka approved these changes May 16, 2024
View reviewed changes
Copy link
Member

@serhiy-storchaka serhiy-storchaka left a comment

Choose a reason for hiding this comment

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

LGTM.

Lib/test/test_capi/test_exceptions.py Outdated Show resolved Hide resolved
@serhiy-storchaka serhiy-storchaka added needs backport to 3.12 bug and security fixes needs backport to 3.13 bugs and security fixes and removed skip news labels May 16, 2024
@serhiy-storchaka serhiy-storchaka enabled auto-merge (squash) May 16, 2024 20:05
@serhiy-storchaka
Copy link
Member

A NEWS entry is needed since this is a user visible change.

@Eclips4
Copy link
Member Author

Eclips4 commented May 16, 2024

Thanks for the review, Serhiy.

@serhiy-storchaka serhiy-storchaka merged commit 100c7ab into python:main May 16, 2024
36 checks passed
@miss-islington-app
Copy link

Thanks @Eclips4 for the PR, and @serhiy-storchaka for merging it 🌮🎉.. I'm working now to backport this PR to: 3.12, 3.13.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request May 16, 2024
@Eclips4 @miss-islington
pythongh-119049: Fix incorrect display of warning which is constructe…
0de65e5 
…d by C API (pythonGH-119063)

The source line was not displayed if the warnings module had not yet
been imported.
(cherry picked from commit 100c7ab)

Co-authored-by: Kirill Podoprigora <kirill.bast9@mail.ru>
@miss-islington-app
Copy link

Sorry, @Eclips4 and @serhiy-storchaka, I could not cleanly backport this to 3.12 due to a conflict.
Please backport using cherry_picker on command line.

cherry_picker 100c7ab00ab66a8c0d54582f35e38d8eb691743c 3.12

@bedevere-app
Copy link

bedevere-app bot commented May 16, 2024

GH-119106 is a backport of this pull request to the 3.13 branch.

@bedevere-app bedevere-app bot removed the needs backport to 3.13 bugs and security fixes label May 16, 2024
@Eclips4
Copy link
Member Author

Eclips4 commented May 17, 2024 •
edited
Loading

@serhiy-storchaka I'm trying to do backport to the 3.12 branch manually, and realize that this cannot be done without the backport of e1d8c65.
In short: to get the sources we need a new version of linecache which has _register_code function. This function is used in pythonrun.c which saves our code to the linecache.cache.

@serhiy-storchaka
Copy link
Member

Is it because of passing the code via the -c option? Then I guess we can just write it into a file.

@bedevere-app
Copy link

bedevere-app bot commented May 17, 2024

GH-119119 is a backport of this pull request to the 3.12 branch.

@bedevere-app bedevere-app bot removed the needs backport to 3.12 bug and security fixes label May 17, 2024
@Eclips4
Copy link
Member Author

Eclips4 commented May 17, 2024

Is it because of passing the code via the -c option? Then I guess we can just write it into a file.

Yes, you're right! Thanks for the hint. Check please the #119119.

@Eclips4 Eclips4 deleted the issue-119049 branch May 17, 2024 11:53
serhiy-storchaka pushed a commit that referenced this pull request May 17, 2024
@miss-islington @Eclips4
[3.13] gh-119049: Fix incorrect display of warning which is construct…
ced71d3 
…ed by C API (GH-119063) (GH-119106)

The source line was not displayed if the warnings module had not yet
been imported.
(cherry picked from commit 100c7ab)

Co-authored-by: Kirill Podoprigora <kirill.bast9@mail.ru>
@serhiy-storchaka serhiy-storchaka removed their assignment May 17, 2024
@JelleZijlstra JelleZijlstra mentioned this pull request May 28, 2024
Closed
estyxx pushed a commit to estyxx/cpython that referenced this pull request Jul 17, 2024
@Eclips4 @estyxx
pythongh-119049: Fix incorrect display of warning which is constructe…
ba0c110 
…d by C API (pythonGH-119063)

The source line was not displayed if the warnings module had not yet
been imported.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@serhiy-storchaka serhiy-storchaka serhiy-storchaka approved these changes

@vstinner vstinner Awaiting requested review from vstinner

@erlend-aasland erlend-aasland Awaiting requested review from erlend-aasland

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

Successfully merging this pull request may close these issues.
2 participants
@Eclips4 @serhiy-storchaka