Skip to content

gh-138432: zoneinfo: improve error message for PathLike relative paths#138433

Merged
serhiy-storchaka merged 8 commits intopython:mainfrom
tungol:zoneinfo
Sep 11, 2025
Merged

gh-138432: zoneinfo: improve error message for PathLike relative paths#138433
serhiy-storchaka merged 8 commits intopython:mainfrom
tungol:zoneinfo

Conversation

@tungol
Copy link
Contributor

@tungol tungol commented Sep 3, 2025 •
edited by bedevere-app bot
Loading

This corrects a small inconsistency with the error handling of zoneinfo.reset_tzpath. A sequence of os.PathLike instances is documented as valid input, but the error pathway for relative paths doesn't handle these correctly. This pull request corrects this, and adds a relevant test case.

@bedevere-app bedevere-app bot mentioned this pull request Sep 3, 2025
Closed
StanFromIreland
StanFromIreland reviewed Sep 3, 2025
View reviewed changes
Copy link
Member

@StanFromIreland StanFromIreland left a comment

Choose a reason for hiding this comment

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

Please add a news entry.

@tungol
Copy link
Contributor Author

tungol commented Sep 3, 2025

Done.

uranusjr
uranusjr reviewed Sep 5, 2025
View reviewed changes
@serhiy-storchaka
Copy link
Member

serhiy-storchaka commented Sep 5, 2025

I suggest to call os.fspath() for all items before assigning them to TZPATH (and before checking that they are absolute paths). We can also add a check that they all are now strings (bytes object will cause error later in find_tzfile() and available_timezones()).

@tungol
Copy link
Contributor Author

tungol commented Sep 5, 2025 •
edited
Loading

I suggest to call os.fspath() for all items before assigning them to TZPATH (and before checking that they are absolute paths).

We can do that, but I don't think it's really necessary. Checking for absolute status is done with os.path.isabs, which already handles os.PathLike objects correctly. Within our code, TZPATH is consumed by find_tzfile, which uses os.path.join to produce a string, so it's again automatically handled.

I found this issue while working on a program to apply typeshed stubs to the stdlib and then analyze with mypy, so I'm relatively confident that this is the only place in (python-source) stdlib where os.Pathlike values for TZPATH were overlooked. (Typeshed correctly annotates TZPATH as tuple[str | PathLike[str], ...])

This leaves the possibility of third-party consumers of TZPATH being surprised by encountering an os.PathLike object instead of a string. If we want to change TZPATH to be restricted to str only on that basis, that's probably reasonable, but I think that's probably best left for a different MR.

@serhiy-storchaka
Copy link
Member

serhiy-storchaka commented Sep 5, 2025

It will help to check that if it is a path-like object, it is PathLike[str], not PathLike[Any]. bytes and PathLike[bytes] in TZPATH will cause runtime error in os.path.join().

Anyway, if os.fspath() is already implicitly called in os.path.isabs() and os.path.join(), why not call it explicitly and save the result?

@tungol
Copy link
Contributor Author

tungol commented Sep 10, 2025

@serhiy-storchaka Is this about what you were thinking?

serhiy-storchaka
serhiy-storchaka reviewed Sep 10, 2025
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 your response @tungol. Yes, this is what I suggested.

Few more nitpicks.

serhiy-storchaka
serhiy-storchaka approved these changes Sep 11, 2025
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. 👍

Thank you for your contribution.

@serhiy-storchaka serhiy-storchaka merged commit 859aecc into python:main Sep 11, 2025
49 checks passed
@serhiy-storchaka serhiy-storchaka added needs backport to 3.13 bugs and security fixes needs backport to 3.14 bugs and security fixes labels Sep 11, 2025
@miss-islington-app
Copy link

miss-islington-app bot commented Sep 11, 2025

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

@miss-islington-app
Copy link

miss-islington-app bot commented Sep 11, 2025

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

@miss-islington-app
Copy link

miss-islington-app bot commented Sep 11, 2025

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

cherry_picker 859aecc33b82f45e5b7ae30138d28f2a2f33a575 3.13

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Sep 11, 2025
@tungol @miss-islington
pythongh-138432: Improved invalid path checking in zoneinfo.reset_tzp…
b296541 
…ath() (pythonGH-138433)

* Improve error messages for path-like relative paths and path-like bytes paths.
* TZPATH is now always a tuple of strings.
(cherry picked from commit 859aecc)

Co-authored-by: Stephen Morton <[email protected]>
@bedevere-app
Copy link

bedevere-app bot commented Sep 11, 2025

GH-138777 is a backport of this pull request to the 3.14 branch.

@bedevere-app bedevere-app bot removed the needs backport to 3.14 bugs and security fixes label Sep 11, 2025
serhiy-storchaka pushed a commit to serhiy-storchaka/cpython that referenced this pull request Sep 11, 2025
@tungol @serhiy-storchaka
[3.13] pythongh-138432: Improved invalid path checking in zoneinfo.re…
5d3e413 
…set_tzpath() (pythonGH-138433)

* Improve error messages for path-like relative paths and path-like bytes paths.
* TZPATH is now always a tuple of strings.
(cherry picked from commit 859aecc)

Co-authored-by: Stephen Morton <[email protected]>
@bedevere-app
Copy link

bedevere-app bot commented Sep 11, 2025

GH-138778 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 Sep 11, 2025
@tungol tungol deleted the zoneinfo branch September 11, 2025 07:17
serhiy-storchaka added a commit that referenced this pull request Sep 11, 2025
@serhiy-storchaka @tungol
[3.13] gh-138432: Improved invalid path checking in zoneinfo.reset_tz…
72b28ca 
…path() (GH-138433) (GH-138778)

* Improve error messages for path-like relative paths and path-like bytes paths.
* TZPATH is now always a tuple of strings.
(cherry picked from commit 859aecc)

Co-authored-by: Stephen Morton <[email protected]>
encukou pushed a commit that referenced this pull request Oct 7, 2025
@miss-islington @tungol
[3.14] gh-138432: Improved invalid path checking in zoneinfo.reset_tz…
dfdda53 
…path() (GH-138433) (GH-138777)

* Improve error messages for path-like relative paths and path-like bytes paths.
* TZPATH is now always a tuple of strings.
(cherry picked from commit 859aecc)

Co-authored-by: Stephen Morton <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@StanFromIreland StanFromIreland StanFromIreland left review comments

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

+1 more reviewer

@uranusjr uranusjr uranusjr left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@serhiy-storchaka serhiy-storchaka

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@tungol @serhiy-storchaka @uranusjr @StanFromIreland