Skip to content

bpo-46170: Improve the error message when subclassing NewType #30268

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 17 commits into from
Mar 8, 2022

Conversation

Gobot1234
Copy link
Contributor

@Gobot1234 Gobot1234 commented Dec 26, 2021

@AlexWaygood
Copy link
Member

You can use https://blurb-it.herokuapp.com/ to add a News entry 🙂

@Gobot1234
Copy link
Contributor Author

I've just realised that what I thought was the cls param to __mro_entries__ isn't the class that's subclassing it's the instance, totally my bad. I'm not entirely sure how to get the name of the subclassing type without some byte-code hack. Any advice?

@Gobot1234
Copy link
Contributor Author

I've had the idea to return a dummy class from mro entries and have the dummy class implement init subclass which then raises the type error.

@AlexWaygood
Copy link
Member

I've had the idea to return a dummy class from mro entries and have the dummy class implement init subclass which then raises the type error.

That looks like it might work, but I'd beware of making the implementation too complex for NewType. The whole point of NewType is that it's meant to be a super-simple thing that does basically nothing at runtime. A better error message would be a great thing to have, but I doubt it would be accepted if it comes at the cost of a much more complex implementation (and I think probably has a very low chance of being accepted if it has a performance penalty).

@gvanrossum
Copy link
Member

What Alex says.

@Gobot1234
Copy link
Contributor Author

Should I update this to use BaseException.__note__?

@AlexWaygood
Copy link
Member

Should I update this to use BaseException.__note__?

Hmm, I don't think this is really the use case for which that was designed. How exactly are you thinking it might help? 🙂

@AlexWaygood
Copy link
Member

AlexWaygood commented Dec 29, 2021

With this patch applied, we get the following traceback if we try to subclass a pseudo-class created with NewType:

>>> from typing import NewType
>>> UserID = NewType('UserID', int)
>>> class Foo(UserID): pass
...
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "C:\Users\Alex\Desktop\Code dump\cpython\Lib\typing.py", line 2478, in __init_subclass__
    raise TypeError(
    ^^^^^^^^^^^^^^^^
TypeError: Cannot subclass UserID, perhaps you were looking for: Foo = NewType('Foo', UserID)

A simpler patch might simply define __mro_entries__ like this:

class NewType:
    def __mro_entries__(cls, bases):
        raise TypeError(f'Cannot subclass an instance of `NewType`')

With that patch applied, you get the following traceback:

>>> from typing import NewType
>>> UserID = NewType('UserID', int)
>>> class Foo(UserID): pass
...
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "C:\Users\Alex\Desktop\Code dump\cpython\Lib\typing.py", line 2473, in __mro_entries__
    raise TypeError(f'Cannot subclass an instance of `NewType`')
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
TypeError: Cannot subclass an instance of `NewType`

The existing patch has the benefit that it tells you what to do instead, but I think a simpler implementation would still be much better than the status quo, and would still be easily googleable.

@Gobot1234, what do you think? 🙂

@Gobot1234
Copy link
Contributor Author

I'm personally a bit fan of being told how to fix something but if you think the implementation is too much added maintenance I'd be happy to remove it

@Gobot1234
Copy link
Contributor Author

Should I update this to use BaseException.__note__?

Hmm, I don't think this is really the use case for which that was designed. How exactly are you thinking it might help? 🙂

I thought the point of the added attribute was to enrich exceptions.

@AlexWaygood
Copy link
Member

Should I update this to use BaseException.__note__?

Hmm, I don't think this is really the use case for which that was designed. How exactly are you thinking it might help? 🙂

I thought the point of the added attribute was to enrich exceptions.

It is, of course, but the intended use case (as far as I understand PEP 678) is generally to add information to exceptions that "make sense". The error message when you try to subclass an instance of NewType is currently a bit nonsensical: it tells you that you've supplied the wrong number of arguments to NewType.__init__. I think we want to replace that error message entirely, rather than add additional information to it.

…' into better-new-type-subclass-message

# Conflicts:
#	Lib/test/test_typing.py
Copy link
Member

@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

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

Personally, I would still go for the simpler implementation myself, but this is looking a lot better to me now 👍

Co-authored-by: Alex Waygood <[email protected]>
Copy link
Member

@JelleZijlstra JelleZijlstra left a comment

Choose a reason for hiding this comment

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

This is a creative abuse of __mro_entries__, but it doesn't seem likely to cause maintenance or performance issues and it gives a much nicer error message. I'm fine with moving forward with it.

Copy link
Member

@Fidget-Spinner Fidget-Spinner left a comment

Choose a reason for hiding this comment

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

This is such a clever workaround. I'm at a loss for words. My only minor concern is that it feels slightly hacky.

@Fidget-Spinner
Copy link
Member

@JelleZijlstra I plan to merge, is that alright? Does anyone else have any objections?

@JelleZijlstra
Copy link
Member

Sounds good, I was planning to hold off until the next alpha is out since it's bumpy and I don't want to give Pablo more headaches.

@Fidget-Spinner
Copy link
Member

Sounds good, I was planning to hold off until the next alpha is out since it's bumpy and I don't want to give Pablo more headaches.

Good idea, I forgot about that. Let's do that then :).

@JelleZijlstra JelleZijlstra merged commit f391f9b into python:main Mar 8, 2022
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.

9 participants