New InspectError instead of OSError

Ideas
1

While reviewing a PR related to inspect, it was noted that it raises OSError for many things unrelated to operating system APIs (this is could technically count as a bug, since the raised exception has errno set to None, for example).

This use of OSError is semantically wrong, but unfortunately, changing this to something like ValueError would be a breaking change. The ideal solution would be to make a new exception called inspect.InspectError – feel free to bikeshed this.

This would still inherit from OSError, to not break existing code that does except OSError, but all of the OS-related fields (such as errno, per my example from above) would emit a DeprecationWarning, and then sometime in the future InspectError could stop inheriting from OSError.

6 Likes
2

If the error type is buggy, we can fix in a new version but not backport. I have not looked at the doc to see what you are referring to. Some quoted examples would be good, and even how many changes you suggest.

3

FWIW, I’m not sure it’s necessarily a bug, but the semantic use is definitely incorrect. In the following, errno is set to None, which shouldn’t be possible, at least according to typeshed and the docs:

try:
    inspect.getsourcelines(lambda x: 1)
except OSError as e:
    print(e.errno)  # None
1 Like
4

Thank you Peter for creating this topic.
FYI, We’re found out this during the review of gh-121331: Make inspect.getsource work for genexp code objects by MarkRotchell · Pull Request #121933 · python/cpython · GitHub.

My thoughts:
If we want to implement a new feature in the inspect module, we need to create a new exception (inspect.InspectError), and therefore use it whenever it needed.
Changing all occurrences of OSError in the inspect module isn’t possible - as you said, it’ll break backwards compatibility.
So let’s just use the new exception for a new features.

In terms of the PR mentioned above, I think we should continue to use OSError, as this PR doesn’t add a new feature but rather extends the behavior of an existing feature.

1 Like
5

I think you could transition, or at least half-transition to InspectError. You could subclass InspectError from OSError, or jointly subclass LegacyInspectError(OSError, InspectError) and set deprecation warnings on attempting to access OSError-specific attributes. Tools like pyupgrade might be able to detect some cases and auto-convert OSError → InspectError when an appropriate minimum Python is reached.

1 Like
6

Yeah, that works too, but adds an extra layer of complexity later on. When we get around to removing LegacyInspectError, we have to remove something from inspect itself, not just remove some attributes from an object.

7

FWIW, I’m not sure it’s necessarily a bug, but the semantic use is definitely incorrect. In the following, errno is set to None, which shouldn’t be possible, at least according to typeshed and the docs:

One reason why OSError was used is possibly because of the use of linecache which does have a bunch of try-except around OSError (so linecache shouldn’t raise OSError technically since it handles them internally). If linecache fails to read a file, it returns a list of empty lines instead of raising OSError. So maybe the reason why OSError was used was for the same logic, namely if inspect fails to read a file, then should it actually raise ValueError or OSError?

In addition, inspect.findsource which is used by inspect.getsourcelines explicitly says:

An OSError is raised if the source code cannot be retrieved.

8

Right, the OSError is documented, which is why turning it into a ValueError would be a breaking change. We need an InspectError to mediate the transition from an OSError to something else.

9

Yeah, None values in OSError are a bug: see #101601. Nonetheless, as Kirill said in the PR, the semantic use of OSError is incorrect here – it should be reserved for operating system related failures.

Any final objections?

10

Just trying to understand the plan, is your new InspectError planned to be a subclass of OSError forever?

11

Yeah, but not the properties. The definition would look something like this (at least on typeshed):

class InspectError(OSError):
    @property
    @deprecated("InspectError.errno is deprecated and will be removed in Python 3.16")
    def errno(self) -> None: ...

    @property
    @deprecated("InspectError.strerror is deprecated and will be removed in Python 3.16")
    def strerror(self) -> None: ...

Although, we could potentially remove the OSError subclass at some point, but I’m not sure how we would deprecate that.

12

If you mean that in general OSError exceptions having errno set to None is a bug, then FWIW that’s the opposite of my interpretation of the docs? They explicitly show two forms of constructor and state the first one sets attributes to None.
For the same reason, I think if InspectError is a subclass then the deprecated properties couldn’t be removed without dropping OSError as base class, since this would violate typical expectations.

13

I didn’t think about this, so I retract my previous point — we very much should plan to remove the subclass, I’m just not too sure what the deprecation period would look like.

Theoretically, by adding InspectError, we could hope to rely on people not catching OSError in the future, but I’m not aware of any way to explicitly deprecate that.

14

If except would actually use __subclasscheck__, it would potentially be possible by adding a custom metaclass for OSError that raises a deprecation warning. But that is probably not going to happen because of concerns about performance.

We for sure can’t add a deprecation warning without C code modifications (it would also be possible to add special code to except statement), but I doubt that this will happen. The core devs will have to decide if this is an acceptable break of the deprecation policy.

1 Like