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

Support PEP 695 generics #348

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

viccie30
Copy link
Contributor

@viccie30 viccie30 commented Jan 5, 2025

This would fix #342.

There are a few things about which I'm not sure:

  • How to do name resolution in annotation scopes. The last commit in this series adds an annotation_scope attribute to ExprName, because parent is already overloaded: either the parent scope or the previous part of the path in a ExprAttribute.
  • Whether to support pre-PEP 695 generics. In the current patch set I have not added support for "traditional" Python generics.
  • Whether to record variance for type variables. In the current patch set I have not added support for variance.
  • How to describe type parameters and type aliases in Sphinx and Numpy docstring styles.

@pawamoy
Copy link
Member

pawamoy commented Jan 6, 2025

Wow, fantastic work @viccie30! 🚀 🚀 It looks very good already.

Annotation scopes

I wonder if we need to do anything special. Griffe's way of resolving names is not the same as Python itself. The following order is applied when resolving a name in a given scope:

  1. member of the current object
  2. parent class (if parent is a class)
  3. repeat 1-2 within parent's scope

To me it sounds like the annotation scope is therefore already supported. Have you noticed resolution failures, or incorrect resolution for type parameters while working on this feature?

Pre-PEP 695 generics

No need to add support in this PR. Nobody ever asked for it yet.

Recording variance

Could be interesting, simply for documentation purposes, but not critical here either. This can be done in subsequent work.

Sphinx-style / Numpydoc-style

Let's not modify those. Sphinx-style is very limited anyway. Numpydoc-style has a somewhat precise style-guide: I'd like to wait and see if they update it for type aliases / type parameters, and how, before we implement anything (even if we've already taken a few liberties in our own parser).

I'll even have to think a bit more about adding support in the Google-style parser itself (do you have use-cases yourself? please share even if they seem completely obvious to you!), but since we have a new kind of object (TypeAlias), it sounds reasonable to have a new docstring section kind too. Generally speaking, the answer to whether we should have a section is driven by the answer to whether the items can be sufficiently annotated outside of docstrings, or without a docstring style (for example with PEP 727's Annotated[..., Doc("...")]). And that depends on what syntax is allowed, ultimately.

@viccie30
Copy link
Contributor Author

viccie30 commented Jan 6, 2025

Wow, fantastic work @viccie30! 🚀 🚀 It looks very good already.

Thank you!

Annotation scopes

I wonder if we need to do anything special. Griffe's way of resolving names is not the same as Python itself. The following order is applied when resolving a name in a given scope:

  1. member of the current object
  2. parent class (if parent is a class)
  3. repeat 1-2 within parent's scope

To me it sounds like the annotation scope is therefore already supported. Have you noticed resolution failures, or incorrect resolution for type parameters while working on this feature?

Yes, the current code cannot correctly resolve references to type parameters in function parameter annotations, function return annotations and class base annotations. In all cases the parent argument passed to safe_get_annotation or safe_get_base_class is the module or class where the function or class is defined. There is no way to access the type parameters of a generic function, if all the code has access to is that function's parent.

An example:

class Class[T](list[T]):
    def func[U](self, arg1: T, arg2: U) -> U: ...

When safe_get_base_classs is called for Class, the parent argument points to the module containing the definition, so the code can never resolve T. When safe_get_annotation is called for arg1 and arg2, the parent argument points to Class. This means the code can now resolve T, but it cannot resolve U because that is defined in the scope of the function itself.

I can see two possible solutions:

  1. Let parent point to the function or class definition when calling safe_get_annotation or safe_get_base_class. This would change the meaning of parent compared to the current code.
  2. Add another attribute to ExprName, which is what I've currently coded.

Solution 1 would be cleaner in my opinion, but it would be a breaking change for consumers.

Pre-PEP 695 generics

No need to add support in this PR. Nobody ever asked for it yet.

Recording variance

Could be interesting, simply for documentation purposes, but not critical here either. This can be done in subsequent work.

I won't work on these additions for now then.

Sphinx-style / Numpydoc-style

Let's not modify those. Sphinx-style is very limited anyway. Numpydoc-style has a somewhat precise style-guide: I'd like to wait and see if they update it for type aliases / type parameters, and how, before we implement anything (even if we've already taken a few liberties in our own parser).
I will remove the numpydoc changes I had already made in the next revision then.

I'll even have to think a bit more about adding support in the Google-style parser itself (do you have use-cases yourself? please share even if they seem completely obvious to you!), but since we have a new kind of object (TypeAlias), it sounds reasonable to have a new docstring section kind too. Generally speaking, the answer to whether we should have a section is driven by the answer to whether the items can be sufficiently annotated outside of docstrings, or without a docstring style (for example with PEP 727's Annotated[..., Doc("...")]). And that depends on what syntax is allowed, ultimately.

My use cases for documenting type parameters are mostly documenting custom generic containers or decorators:

class MyCustomMapping[K, V]:
    """A custom mapping.

    Type Parameters:
        K: Key type. Does not have to be hashable, but has to be symmetrically equality comparable.
        V: Value type. Must be serializable to JSON for processing.
    """

def my_decorator[**P, R](callable: Callable[P, R]) -> Callable[Concatenate[int, P], str]:
    """Decorator for auto-stringizing.

    Parameters:
        callable: Callable to wrap.

    Type Parameters:
        P: Original parameters of `callable`.
        R: Original return type of `callable`. Must be convertible to `str`.
    """

Personally I like to document type aliases the same as attributes: with a docstring immediately below it. If PEP 727 is accepted, then also parsing documentation using typing.Doc would be useful. I don't really like that style myself, because for me it makes the difference between code and documentation less clear.

I only use sections for objects I can't document more directly, like parameters and return values. For classes, attributes, functions, etc. I always document them in their own docstrings. Since it's not possible to add a docstring for type parameters, I think that the section for type parameters is needed. I simply added the type aliases section for completeness, but I don't intend on ever using it myself. It is however used by mkdocstrings/python to auto-generate a type aliases section in summaries, which makes it useful.

@pawamoy
Copy link
Member

pawamoy commented Jan 6, 2025

Thanks a lot, that's super clear.

So, given your first example, I understand that the code cannot resolve T in list[T], or U in arg2: U. What I'm not sure to understand is why do we want to resolve them? Is it simply because the current code tries to resolve them anyway, so we want to allow it to do so? Or is there maybe a use-case where such type parameters would cross-reference to... something? 🤔

(I'm not used to generics, even less PEP 695, so please bear with me 😅 )

I can definitely see the usefulness of the "Type Parameters" section. I suppose the following is not even valid (would require inheriting from Generic[K, V] instead):

K = TypeVar("K")
"""Key type. Does not have to be hashable, but has to be symmetrically equality comparable."""

V = TypeVar("V")
"""Value type. Must be serializable to JSON for processing."""

class MyCustomMapping[K, V]:
    """A custom mapping."""

Same for this, not valid I suppose:

class MyCustomMapping[Annotated[K, ...], Annotated[V, ...]]:
    """A custom mapping."""

Is there any way with modern syntax to attach more information to K or V, without relying on docstring styles?

@pawamoy
Copy link
Member

pawamoy commented Jan 6, 2025

OK so my use of TypeVar is valid (at least the syntax), even though it feels like a step backward. Will type checkers trip over this?

@viccie30
Copy link
Contributor Author

viccie30 commented Jan 6, 2025

Thanks a lot, that's super clear.

So, given your first example, I understand that the code cannot resolve T in list[T], or U in arg2: U. What I'm not sure to understand is why do we want to resolve them? Is it simply because the current code tries to resolve them anyway, so we want to allow it to do so? Or is there maybe a use-case where such type parameters would cross-reference to... something? 🤔

If a generic class has type parameters and methods in that class use those type parameters, I think it would be useful to link them. See for instance this screenshot I took using mkdocstrings/python#221:
Schermafbeelding 2025-01-06 213928

(I'm not used to generics, even less PEP 695, so please bear with me 😅 )

I can definitely see the usefulness of the "Type Parameters" section. I suppose the following is not even valid (would require inheriting from Generic[K, V] instead):

K = TypeVar("K")
"""Key type. Does not have to be hashable, but has to be symmetrically equality comparable."""

V = TypeVar("V")
"""Value type. Must be serializable to JSON for processing."""

class MyCustomMapping[K, V]:
    """A custom mapping."""

That is valid, but does not mean what you might expect it to mean. K and V in MyCustomMapping are completely independent from the global TypeVars. The Python reference explains how a generic class definition is interpreted. PEP 695 explains what is allowed when mixing traditional and new TypeVars.

Same for this, not valid I suppose:

class MyCustomMapping[Annotated[K, ...], Annotated[V, ...]]:
    """A custom mapping."""

Is there any way with modern syntax to attach more information to K or V, without relying on docstring styles?

This is indeed invalid, type parameter lists must only contain identifiers, possibly prefixed with stars and followed by bound or constraint, or default. I don't see any way to document type parameters outside of docstrings.

@pawamoy
Copy link
Member

pawamoy commented Jan 6, 2025

use-case

Right! Very valid use-case indeed. Thanks. I'll mention the griffe-generics third-party extension, which helps with subclasses of generic base classes, but not base classes themselves.

K and V in MyCustomMapping are completely independent from the global TypeVars.

Yeah OK that's what I gathered. We can safely expect that no one will use that.

OK, well that's unfortunate. I'll keep thinking about it, and in the meantime we can move forward with type alias and type parameter docstring sections (which you already implemented 👍)

That leaves the question of the implementation for annotation scopes. I agree with you option 1 would be more elegant. I'll see how much it could break things.

@viccie30
Copy link
Contributor Author

viccie30 commented Jan 6, 2025

use-case

Right! Very valid use-case indeed. Thanks. I'll mention the griffe-generics third-party extension, which helps with subclasses of generic base classes, but not base classes themselves.

I looked at that extension, but I did not figure out a way to use it in my implementation.

K and V in MyCustomMapping are completely independent from the global TypeVars.

Yeah OK that's what I gathered. We can safely expect that no one will use that.

There's probably more weird Python tricks that can't be parsed by inspecting the AST, like dynamically adding stuff at run time and other things, so I don't think it's necessary to add this.

OK, well that's unfortunate. I'll keep thinking about it, and in the meantime we can move forward with type alias and type parameter docstring sections (which you already implemented 👍)

That leaves the question of the implementation for annotation scopes. I agree with you option 1 would be more elegant. I'll see how much it could break things.

I'll see if I can push an alternate version of the third commit tomorrow that changes the semantics of parent instead.

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.

feature: Support PEP 695 Type Parameter Syntax
2 participants