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.

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

pydoc renders from builtins.type note, even if it is incorrect #97959

Closed
sobolevn opened this issue Oct 6, 2022 · 9 comments
Closed

pydoc renders from builtins.type note, even if it is incorrect #97959

sobolevn opened this issue Oct 6, 2022 · 9 comments
Labels
stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error

Comments

@sobolevn
Copy link
Member

sobolevn commented Oct 6, 2022

While working on #97958 I've noticed that there's something strange with help() and classmethods.

Take a look at this example:

import pydoc

class My:
    @classmethod
    def __init_subclass__(cls, *args, **kwargs):
        pass

    @classmethod
    def custom(cls):
        pass

print(pydoc.plain(pydoc.render_doc(My)))

It prints:

Python Library Documentation: class My in module __main__

class My(builtins.object)
 |  Class methods defined here:
 |
 |  __init_subclass__(*args, **kwargs) from builtins.type
 |      This method is called when a class is subclassed.
 |
 |      The default implementation does nothing. It may be
 |      overridden to extend subclasses.
 |
 |  custom() from builtins.type
 |
 |  ----------------------------------------------------------------------
 |  Data descriptors defined here:
 |
 |  __dict__
 |      dictionary for instance variables (if defined)
 |
 |  __weakref__
 |      list of weak references to the object (if defined)

Take a look at these two entries:

  1. __init_subclass__(*args, **kwargs) from builtins.type
  2. custom() from builtins.type

While type has __init_subclass__, there's no type.custom. But, help says that there is!

>>> type.__init_subclass__
<built-in method __init_subclass__ of type object at 0x10a50c360>
>>> type.custom
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
AttributeError: type object 'type' has no attribute 'custom'

I think that it is incorrect and can lead to confusion.
Instead it should be:

 |  __init_subclass__(*args, **kwargs) from builtins.type
 |      This method is called when a class is subclassed.
 |
 |      The default implementation does nothing. It may be
 |      overridden to extend subclasses.
 |
 |  custom()

Linked PRs

@sobolevn sobolevn added the type-bug An unexpected behavior, bug, or error label Oct 6, 2022
@sobolevn
Copy link
Member Author

sobolevn commented Oct 6, 2022

A couple of extra thoughts after reading some more source code:
https://github.com/python/cpython/blame/e39ae6bef2c357a88e232dcab2e4b4c0f367544b/Lib/pydoc.py#L1032-L1042

        if _is_bound_method(object):
            imclass = object.__self__.__class__
            if cl:
                if imclass is not cl:
                    note = ' from ' + classname(imclass, mod)

Ok, looks like this is in sync with the following example:

import pydoc
from types import MethodType

class My:
    some = MethodType(some, 1)

print(pydoc.plain(pydoc.render_doc(My)))

which outputs:

Python Library Documentation: class My in module __main__

class My(builtins.object)
 |  Methods defined here:
 |
 |  some(*args, **kwargs) from builtins.int

I think that classmethods should just completely skip this part.
It brings more confusion than value. So, it should be:

|  __init_subclass__(*args, **kwargs)
 |      This method is called when a class is subclassed.
 |
 |      The default implementation does nothing. It may be
 |      overridden to extend subclasses.
 |
 |  custom()

@sobolevn sobolevn added the stdlib Python modules in the Lib dir label Oct 6, 2022
sobolevn added a commit to sobolevn/cpython that referenced this issue Oct 9, 2022
@serhiy-storchaka
Copy link
Member

Good catch.

Why this "from ..." was added in the first case? In what cases it was useful.

@sobolevn
Copy link
Member Author

sobolevn commented Oct 10, 2022

I found only one: when MethodType is defined in a class body:

class Some:
    a = MethodType(some_func, some_instance)

Plus, Enum uses from enum.EnumType on some methods. I am still testing other things.

@serhiy-storchaka
Copy link
Member

I found only one: when MethodType is defined in a class body:

But where is it used in the real code?

If make this code raising an error instead of adding "from ..." and run pydoc for all stdlib modules, what will fail? Investigate also the history of that code, commit message or issue text can explain its purpose.

@sobolevn
Copy link
Member Author

sobolevn commented Nov 7, 2022

So, I got back to it. Here's what I found.

test results

Running pydoc tests with raise ValueError instead of note = ' from ' + classname(imclass, mod) showed this:

2 tests failed:
    test_enum test_pydoc

Only pydoc itself and enum are affected.
Basically, the same modules I've changed in #98120

history

There are two commits that touch this line:

  1. 15 years ago, when unbound methods were removed: ff73795
  2. 22 years ago, when b7a4830 was merged. It does not have a description. Its title is Fix linking to classes (in class tree, and add links on unbound methods)

So, I can conclude that these lines have something to do with old unbound methods.

= MethodType() usage

There are some usage of it:

» ag '= MethodType' 
Lib/test/test_importlib/import_/test_caching.py
60:        mock.load_module = MethodType(load_module, mock)

Lib/test/test_call.py
754:                meth = MethodType(func, args[0])

But, lets think about whether is this important to show from ... part. Compare these two (with and without from):

>>> import types
>>> class A:
...    x = types.MethodType(some, 1)
... 
>>> help(A)
Help on class A in module __main__:

class A(builtins.object)
 |  Methods defined here:
 |
 |  x = some(*args, **kwargs) from builtins.int
 |
 |  ----------------------------------------------------------------------
 |  Data descriptors defined here:

Without:

>>> help(A)
Help on class A in module __main__:

class A(builtins.object)
 |  Methods defined here:
 |
 |  x = some(*args, **kwargs)
 |
 |  ----------------------------------------------------------------------
 |  Data descriptors defined here:
  1. Has anything really changed?
  2. Will users be able to decrypt that x is a MethodType bound to int from this short message? I don't think so. I've spent several hours trying to decode what it means and how it works.

lib usage

Let's take a look at the enum case: it kinda is using this feature.

 |  ----------------------------------------------------------------------
 |  Methods inherited from enum.EnumType:
 |
 |  __contains__(value) from enum.EnumType
 |      Return True if `value` is in `cls`.
 |
 |      `value` is in `cls` if:
 |      1) `value` is a member of `cls`, or
 |      2) `value` is the value of one of the `cls`'s members.
 |
 |  __getitem__(name) from enum.EnumType
 |      Return the member matching `name`.
 |
 |  __iter__() from enum.EnumType
 |      Return members in definition order.
 |
 |  __len__() from enum.EnumType
 |      Return the number of members (no aliases)
 |
 |  ----------------------------------------------------------------------

Is it useful here? Again, I don't think so: we already have Methods inherited from enum.EnumType: part which clearly says where these methods come from.

In my opinion, it brings more confusion than value.

solution

I think that we should at least protect @classmethod from been reported incorrectly, as I did in #98120

Or we can completely remove note = ' from ' + classname(imclass, mod) part, because:

  1. It is an artifact from another era: when unbound methods were a thing
  2. It does not bring any value to existing Lib/ code
  3. It is quite cryptic to understand for regular users
  4. It is easy to do, nothing will probably break because of that

@serhiy-storchaka what do you think? :)

@ethanfurman
Copy link
Member

I do not see any value in the from some_module.some_type. I'd like to see it removed.

@ethanfurman
Copy link
Member

Not sure if this is related, but help(enum.Enum) shows:

Help on class Enum in module enum:

class Enum(builtins.object)
 |  . . .

It seems to me that instead of builtins.object that should say enum.EnumType.

serhiy-storchaka added a commit to serhiy-storchaka/cpython that referenced this issue Jan 11, 2024
* Class methods no longer have "method of builtins.type instance" note.
* Corresponding notes are now added for class and unbound methods.
* Method and function aliases now have references to the module or the
  class where the origin was defined if it differs from the current.
* Bound methods are now listed in the static methods section.
* Methods of builtin classes are now supported as well as methods of
  Python classes.
@serhiy-storchaka
Copy link
Member

This issue is much much more complex than it looked at first glance. If you look at the code, you will see other issues with notes for routines. For example an "unbound method" note is never added to unbound methods. It does not work with methods of builtin classes. It seems that this code was written to handle classic classes, and the support of new-style classes was never complete. Some changes was made later to fix runtime errors, but it was not checked that the result is correct and that all cases are supported.

Other issue, is that class members that are bound methods were included in the methods section. But they behave rather like static methods, because they do not accept the self argument.

#113941 fixes these and other issues.

There are still unfixed issues, for example nested classes are not supported (#85799), and unbound methods of builtin classes are not visible as module members (#113942), but this PR is already large.

serhiy-storchaka added a commit that referenced this issue Feb 11, 2024
* Class methods no longer have "method of builtins.type instance" note.
* Corresponding notes are now added for class and unbound methods.
* Method and function aliases now have references to the module or the
  class where the origin was defined if it differs from the current.
* Bound methods are now listed in the static methods section.
* Methods of builtin classes are now supported as well as methods of
  Python classes.
serhiy-storchaka added a commit to serhiy-storchaka/cpython that referenced this issue Feb 11, 2024
…13941)

* Class methods no longer have "method of builtins.type instance" note.
* Corresponding notes are now added for class and unbound methods.
* Method and function aliases now have references to the module or the
  class where the origin was defined if it differs from the current.
* Bound methods are now listed in the static methods section.
* Methods of builtin classes are now supported as well as methods of
  Python classes.
(cherry picked from commit 2939ad0)

Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
serhiy-storchaka added a commit that referenced this issue Feb 11, 2024
…15296)

* Class methods no longer have "method of builtins.type instance" note.
* Corresponding notes are now added for class and unbound methods.
* Method and function aliases now have references to the module or the
  class where the origin was defined if it differs from the current.
* Bound methods are now listed in the static methods section.
* Methods of builtin classes are now supported as well as methods of
  Python classes.
(cherry picked from commit 2939ad0)
serhiy-storchaka added a commit to serhiy-storchaka/cpython that referenced this issue Feb 11, 2024
…honGH-113941) (pythonGH-115296)

* Class methods no longer have "method of builtins.type instance" note.
* Corresponding notes are now added for class and unbound methods.
* Method and function aliases now have references to the module or the
  class where the origin was defined if it differs from the current.
* Bound methods are now listed in the static methods section.
* Methods of builtin classes are now supported as well as methods of
  Python classes.
(cherry picked from commit 2939ad0)
(cherry picked from commit cfb79ca)

Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
serhiy-storchaka added a commit that referenced this issue Feb 11, 2024
GH-115296) (GH-115302)

* Class methods no longer have "method of builtins.type instance" note.
* Corresponding notes are now added for class and unbound methods.
* Method and function aliases now have references to the module or the
  class where the origin was defined if it differs from the current.
* Bound methods are now listed in the static methods section.
* Methods of builtin classes are now supported as well as methods of
  Python classes.
(cherry picked from commit 2939ad0)
(cherry picked from commit cfb79ca)
@serhiy-storchaka
Copy link
Member

About the enum.Enum case. It was:

  |  Methods inherited from enum.EnumType:
  |
  |  __contains__(value) from enum.EnumType
  |      Return True if `value` is in `cls`.

First, "from enum.EnumType" is redundant, because there is "from enum.EnumType" in the section title. Usually "from" notes are not repeated for every method.

Second, the section title is not completely correct. They are not normal methods. The first argument of normal arguments is self (whatever its name). You cannot call enuminstance.__contains__(). You should call enumclass.__contains__(value) -- this is the behavior of static or class method. It is difficult to distinguish them at that level, so for now they are qualified as static methods.

BTW, for the "from" example look at the enum.StrEnum help. Now it contains:

 |  Methods defined here:
...
 |  __repr__(self) from enum.Enum
 |      Return repr(self).
 |
 |  __str__(self, /) from builtins.str
 |      Return str(self).

Note that __str__ is "from builtins.str", and __repr__ is "from enum.Enum", despite both are "methods defined here". You can see that __str__ is the same as for strings, and __repr__ is implemented in Enum, and that they override methods inherited from base classes.

fsc-eriker pushed a commit to fsc-eriker/cpython that referenced this issue Feb 14, 2024
* Class methods no longer have "method of builtins.type instance" note.
* Corresponding notes are now added for class and unbound methods.
* Method and function aliases now have references to the module or the
  class where the origin was defined if it differs from the current.
* Bound methods are now listed in the static methods section.
* Methods of builtin classes are now supported as well as methods of
  Python classes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error
Projects
None yet
Development

No branches or pull requests

3 participants