gh-122353: Handle `ValueError` during imports #122389

picnixz · 2024-07-29T09:30:06Z

This is a simple fix but I am not an expert enough concerning the import system. The idea is to also catch the ValueError that could be raised by any function using the path_t AC converter (or by any other handler that is explicitly raising it).

Issue: Import errors with loo long entry in sys.path or PYTHONPATH in Windows #122353

Lib/importlib/_bootstrap_external.py

Lib/test/test_importlib/import_/test_path.py

serhiy-storchaka

Thank you. I have only two more comments.

Doc/library/importlib.rst

Lib/importlib/_bootstrap_external.py

zooba

The patch is good overall, but I want to be more fully convinced that (a) users shouldn't be told when they provide bad values, or (b) we shouldn't be raising OSError from the path converter instead. Discussion on #122353 please.

zooba · 2024-08-06T22:22:13Z

Doc/library/importlib.rst

+        If the *path* cannot be handled, this raises an :exc:`OSError`
+        or a :exc:`ValueError` depending on the reason.


We deliberately don't document ValueError in most cases because it's assumed to be a possibility whenever you pass in an invalid value. If we start documenting it, we'll end up having to put it on every single function.

I'd rather skip all these doc changes and keep with our usual conventions. It may be surprising at first, but the error does only occur when an invalid value is provided, and not due to OS/environment issues (which are the cause of the OSError, and we document functions that may do something that could result in an OSError because most do not).

Since OSError is already explicitly documented here, I do not see harm in mentioning also ValueError which depends on OS and environment. For example, UnicodeEncodeError (which is a subclass of ValueError) can be raised depending on the locale.

This is also important for end user to know which exceptions they should catch. OSError and ValueError are not raised in normal cases, but they can be raised depending on user data, OS and environment. It is easy to miss ValueError.

Such exceptions like TypeError, MemoryError, RecursionError should not be explicitly documented.

It depends on the OS, but not on the state of the OS (what I meant by "environment", as opposed to "environment variables" which are only one aspect of the overall state).

ValueError can be raised practically anywhere. If you aren't sure you're passing a valid value, you always need to handle ValueError. It's in the same category as TypeError, MemoryError and RecursionError (and AttributeError when calling a function, as this is essentially the same as TypeError).

Another phrasing issue here is the switch from the imperative "is to be raised" to the descriptive "this raises". It's an abstract method definition giving instructions to subclass authors, so the "is to be raised" phrasing should be kept.

Due to that, I do think it's reasonable to mention ValueError explicitly here, specifically so subclass method implementors don't feel obliged to catch it and turn it into OSError.

Lib/importlib/_bootstrap_external.py

bedevere-app · 2024-08-06T22:28:52Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

serhiy-storchaka

LGTM.

Lib/importlib/_bootstrap_external.py

serhiy-storchaka · 2024-08-07T08:12:08Z

Doc/library/importlib.rst

+        If the *path* cannot be handled, this raises an :exc:`OSError`
+        or a :exc:`ValueError` depending on the reason.


Since OSError is already explicitly documented here, I do not see harm in mentioning also ValueError which depends on OS and environment. For example, UnicodeEncodeError (which is a subclass of ValueError) can be raised depending on the locale.

This is also important for end user to know which exceptions they should catch. OSError and ValueError are not raised in normal cases, but they can be raised depending on user data, OS and environment. It is easy to miss ValueError.

Such exceptions like TypeError, MemoryError, RecursionError should not be explicitly documented.

ncoghlan

The test case update looks good to me, but several requests and suggestions inline for the details of the docs and implementation changes.

ncoghlan · 2024-08-09T07:54:32Z

Doc/library/importlib.rst

+        If the *path* cannot be handled, this raises an :exc:`OSError`
+        or a :exc:`ValueError` depending on the reason.


Another phrasing issue here is the switch from the imperative "is to be raised" to the descriptive "this raises". It's an abstract method definition giving instructions to subclass authors, so the "is to be raised" phrasing should be kept.

Due to that, I do think it's reasonable to mention ValueError explicitly here, specifically so subclass method implementors don't feel obliged to catch it and turn it into OSError.

Doc/library/importlib.rst

ncoghlan · 2024-08-09T08:05:08Z

Doc/library/importlib.rst

+      If the *path* cannot be handled, this raises an :exc:`OSError`
+      or a :exc:`ValueError` depending on the reason.


Exceptions are covered in ResourceLoader abstract method documentation above.

Suggested change

If the *path* cannot be handled, this raises an :exc:`OSError`

or a :exc:`ValueError` depending on the reason.

Actually, I duplicated the entry because the ABC marked as deprecated since 3.7. As such, I'm not sure whether the ABC should update its documentation or not...

Doc/library/importlib.rst

ncoghlan · 2024-08-09T08:10:00Z

Doc/library/importlib.rst

+        When writing to the path fails by raising an :class:`OSError`
+        or a :class:`ValueError`, the exception is not propagated.


This change is not correct, only a lack of write permissions should be silently ignored, everything else should be propagated (despite importlib itself being overly broad in the errors it suppresses).

Suggested change

When writing to the path fails by raising an :class:`OSError`

or a :class:`ValueError`, the exception is not propagated.

When writing to the path fails because the path is read-only

(:const:`errno.EACCES`/:exc:`PermissionError`), do not propagate the

exception. Other exceptions should still be propagated.

Since the tests are failing, should we also protect against FileExistsError when writing the data? By suppressing OSError altogether, we actually suppressed FileExistsError in free-threaded builds and read-only errors (those with errno 30 but that are not explicitly PermissionError).

It seems that the FileExistsError only happens in free-threaded builds of Mac OS but I'm not sure whether it's related or not...

ncoghlan · 2024-08-09T08:14:33Z

Lib/importlib/_bootstrap_external.py

-            except OSError as exc:
-                # Could be a permission error, read-only filesystem: just forget
-                # about writing the data.
+            except (OSError, ValueError) as exc:


This should be made narrower rather than broader (i.e. only ignoring PermissionError)

ncoghlan · 2024-08-09T08:15:44Z

Lib/importlib/_bootstrap_external.py

                _bootstrap._verbose_message('could not create {!r}: {!r}',
                                            parent, exc)
                return
        try:
            _write_atomic(path, data, _mode)
-            _bootstrap._verbose_message('created {!r}', path)
-        except OSError as exc:
+        except (OSError, ValueError) as exc:


Ditto on the narrower catch.

bedevere-app · 2024-08-09T08:17:52Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

picnixz · 2024-08-12T15:52:05Z

There is a pending doc question I have but otherwise it should be good for a second round of reviews (btw, thank you all for your time). I have made the requested changes; please review again.

bedevere-app · 2024-08-12T15:52:10Z

Thanks for making the requested changes!

@serhiy-storchaka, @zooba, @ncoghlan: please review the changes made to this pull request.

…thon into handle-valueerror-on-imports

picnixz · 2024-11-07T10:05:59Z

@ncoghlan Could you have a look at the modified text and the question I had please (#122389 (comment))? Thank you! @zooba Could you confirm whether the latest changes are fine? Thank you!

picnixz added 3 commits July 29, 2024 11:25

handle ValueError

d4336d8

add tests

a7e34ca

blurb

a14a08a

picnixz requested review from brettcannon, ericsnowcurrently, ncoghlan and warsaw as code owners July 29, 2024 09:30

bedevere-app bot mentioned this pull request Jul 29, 2024

Import errors with loo long entry in sys.path or PYTHONPATH in Windows #122353

Open

bedevere-app bot added the awaiting review label Jul 29, 2024

serhiy-storchaka reviewed Jul 31, 2024

View reviewed changes

picnixz added 4 commits July 31, 2024 12:31

relax guards

b4619f0

use a (safe) bare except

16c22b9

remove file extension in test

9aa048d

update docs

b554b2c

picnixz requested a review from serhiy-storchaka July 31, 2024 10:51

serhiy-storchaka reviewed Jul 31, 2024

View reviewed changes

Doc/library/importlib.rst Outdated Show resolved Hide resolved

Lib/importlib/_bootstrap_external.py Show resolved Hide resolved

picnixz added 5 commits July 31, 2024 13:41

keep the correct guard!

f0fb424

simplify docs

622d3a1

simplify

b2b1190

fixup! comment

ef21e28

fixup! even better

aa99397

zooba requested changes Aug 6, 2024

View reviewed changes

bedevere-app bot added awaiting changes and removed awaiting review labels Aug 6, 2024

serhiy-storchaka approved these changes Aug 7, 2024

View reviewed changes

bedevere-app bot added awaiting merge and removed awaiting changes labels Aug 7, 2024

ncoghlan requested changes Aug 9, 2024

View reviewed changes

bedevere-app bot removed the awaiting merge label Aug 9, 2024

bedevere-app bot added the awaiting changes label Aug 9, 2024

picnixz added 3 commits August 12, 2024 17:34

use imperative style

b039c62

narrow exceptions guards in SourceFileLoader.set_data

3a27cb0

Merge branch 'main' into handle-valueerror-on-imports

06cf720

bedevere-app bot added awaiting change review and removed awaiting changes labels Aug 12, 2024

bedevere-app bot requested review from ncoghlan, serhiy-storchaka and zooba August 12, 2024 15:52

picnixz added 6 commits August 12, 2024 18:06

guard against permissions and EROFS errors

f2b269a

Merge branch 'handle-valueerror-on-imports' of github.com:picnixz/cpy…

482a716

…thon into handle-valueerror-on-imports

guard against permissions and EROFS errors

cbce6d3

fixup indentation

24c95a5

Merge branch 'main' into handle-valueerror-on-imports

e9283f5

Merge branch 'main' into handle-valueerror-on-imports

1825481

picnixz added the stale Stale PR or inactive for long period of time. label Mar 2, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

gh-122353: Handle `ValueError` during imports #122389

gh-122353: Handle `ValueError` during imports #122389

picnixz commented Jul 29, 2024 •

edited by bedevere-app bot

Loading

serhiy-storchaka left a comment

zooba left a comment

zooba Aug 6, 2024

serhiy-storchaka Aug 7, 2024

zooba Aug 7, 2024

ncoghlan Aug 9, 2024

bedevere-app bot commented Aug 6, 2024

serhiy-storchaka left a comment

serhiy-storchaka Aug 7, 2024

ncoghlan left a comment

ncoghlan Aug 9, 2024

ncoghlan Aug 9, 2024

picnixz Aug 12, 2024

ncoghlan Aug 9, 2024 •

edited

Loading

picnixz Aug 12, 2024 •

edited

Loading

ncoghlan Aug 9, 2024

ncoghlan Aug 9, 2024

bedevere-app bot commented Aug 9, 2024

picnixz commented Aug 12, 2024

bedevere-app bot commented Aug 12, 2024

picnixz commented Nov 7, 2024

		If the path cannot be handled, this raises an :exc:`OSError`
		or a :exc:`ValueError` depending on the reason.

		When writing to the path fails by raising an :class:`OSError`
		or a :class:`ValueError`, the exception is not propagated.

-        When writing to the path fails by raising an :class:`OSError`
-        or a :class:`ValueError`, the exception is not propagated.
+        When writing to the path fails because the path is read-only
+        (:const:`errno.EACCES`/:exc:`PermissionError`), do not propagate the
+        exception. Other exceptions should still be propagated.

gh-122353: Handle ValueError during imports #122389

Are you sure you want to change the base?

gh-122353: Handle ValueError during imports #122389

Conversation

picnixz commented Jul 29, 2024 • edited by bedevere-app bot Loading

serhiy-storchaka left a comment

Choose a reason for hiding this comment

zooba left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bedevere-app bot commented Aug 6, 2024

serhiy-storchaka left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ncoghlan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ncoghlan Aug 9, 2024 • edited Loading

Choose a reason for hiding this comment

picnixz Aug 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bedevere-app bot commented Aug 9, 2024

picnixz commented Aug 12, 2024

bedevere-app bot commented Aug 12, 2024

picnixz commented Nov 7, 2024

gh-122353: Handle `ValueError` during imports #122389

gh-122353: Handle `ValueError` during imports #122389

picnixz commented Jul 29, 2024 •

edited by bedevere-app bot

Loading

ncoghlan Aug 9, 2024 •

edited

Loading

picnixz Aug 12, 2024 •

edited

Loading