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

[WIP] Stub generation #176

Open
wants to merge 49 commits into
base: master
Choose a base branch
from
Open

[WIP] Stub generation #176

wants to merge 49 commits into from

Conversation

kaiw
Copy link
Collaborator

@kaiw kaiw commented Dec 21, 2018

This is just a request for feedback... you probably don't want to merge this.

This PR adds actual typing information to some parts of the stub generation. Currently enums, flags, consts and functions mostly work, though there are some issues around name handling for function arguments. The goal here is to get typing annotations for mypy checking. Personally I don't care at all about the IDE completion aspect of #79 here, though they're probably related.

This is very much a work in progress, but I wanted to check whether this is of interest before I start on the actually hard stuff (e.g., figuring out out classes, GObject property annotations, etc.).

This also retains the original constant value on the Constant class so
that the stubbing can introspect its type. We could instead do the
typing-annotation style formatting on the constant class, but this same
logic is going to be needed for e.g., formatting function parameter
types.
This also retains the full function signature on functions so that we
can get the Python typing annotations for args and return values
correct.
@lazka
Copy link
Member

lazka commented Dec 22, 2018

Sure, looks good!

We probably wont need documentation for this so I'm wondering if I should add a mode that skips all the doc parsing to speed things up (??)

This makes the virtualenv-installed setup work.
This makes it a lot easier because we can just add a `typing` import to
the top of the `.pyi` file and forget about everything else.
The problem here is that we need class/struct/etc. references within the
module being annotated to not have the module prefix, because otherwise
we just get unresolvable typing references. We also need all of these
to be forward references (i.e., strings) unless we want to do some kind
of dependency ordering, which I don't.

I'm not a fan of doing this with a global, but the other solutions are
much more complicated. Maybe we can make a stubber state class instead
of this, but it's not obvious how much better that would be.
While there's dependency information in the namespace class, it's not
reliable when we need to have all relevant modules in the current
namespace for name resolution. The solution here is to constantly update
a list of modules that have been referenced by current-module
annotations, since that's what mypy, etc. are going to need.

This change also means that we can't write import order until the end of
the stubbing, so we're now keeping the stubs in a StringIO.
This doesn't cover everything, but now at least we get GType, which was
causing a lot of issues.
Firstly, it's worth noting that this only ignores the typing error.
Things that call these APIs will still be checked! The problem is that
the checking may be incorrect because of the API itself.

There's a long discussion about this and related Liskov issues at:
    python/mypy#1237

The short version is that this API is wrong-ish, but there's no
mechanism within mypy to correctly annotate what's going on here. There
*is* some of this around, because mypy treats __init__ and __new__
differently itself, but we can't apply that treatment to our
constructors.

This would be better if we actually knew which methods were
constructors, instead of the name-based guessing here... but that's way
too much complexity for me right now.
@kaiw
Copy link
Collaborator Author

kaiw commented Dec 25, 2018

This is approaching the point of generating usable stubs. There's still a lot of errors, but I think they're starting to get down to things like GError aliases, Liskov substitution problems, and missing Python annotations for overrides, etc.

I think I need to sit down and unify a bunch of the class/flag/enum logic, and then figure out how to handle a static list of "here are exceptions that we can't handle" to the stubbing.

...and then there should be some tests.

We probably wont need documentation for this so I'm wondering if I should add a mode that skips all the doc parsing to speed things up (??)

When setting PGIDOCGEN_CACHE, this isn't too bad for me at least... but I'm only annotating half a dozen modules. It would be nice if it was faster, but honestly I think most of the remaining time is constructing the namespaces.

@lazka
Copy link
Member

lazka commented Dec 26, 2018

Feel free to merge whenever you want. You should have a repo invite so you can push directly if you want.

@kaiw
Copy link
Collaborator Author

kaiw commented Jan 13, 2019

Sorry, I haven't had a lot of time to work on this.

The current state is that the above issues are all fixed, except for the Liskov violations (that are correctly annotating the API). If I'm reading python/mypy#5260 correctly, this is probably only happening because I'm testing with a custom MYPY_PATH, and if they were distributed as a PEP 561 package then users wouldn't see these.

The remaining major issues are:

  • Missing annotations for aliases (e.g., GObject.GObject) or things missing from pgi (e.g., GObject.Signal)
  • Functions have no default argument annotations
  • Needs testing

I can probably work around the first. The second of these... I have no idea how much work that will be. The tests I'll start on when I next get a chance to work on this.

Are the current stubs being used for anything? I'm happy to merge this as being pretty good, but I'd be worried at breakage if they're actually in use.

flake8 is trying to do the right thing here, but in this instance we're
dealing with getting an actual type, so the "use isinstance instead"
error really isn't helpful or correct.
@bochecha
Copy link

Are the current stubs being used for anything?

I don't use the current stubs as they don't actually have any type hints.

I'd be very willing to use the generated stubs, though. 🙂

For now I have my own stubs defined in my project, manually written, covering only the API I need.

@kaiw
Copy link
Collaborator Author

kaiw commented Aug 1, 2019

I still haven't had any real time or motivation to poke this much. The basics are still there, but there's many issues.

  • There are disagreements between the introspection information and about what's exposed and how (e.g., GObject.Property is a module-level alias and the stub generation doesn't pick that up).
  • The lack of any mypy-exposed typing information for GObject properties and the like makes it not very useful for some codebases (Meld suffers from this when accessing e.g., widget.props.label).
  • Closely related to the above is that all Object.new() constructors that take GObject props as kwargs have incorrect signatures.
  • GObject/GTK's inheritance hierarchy means that in pygobject, completely unrelated methods shadow each other constantly. There's many, many examples of this, but one that ends up coming up in really odd places with mypy is TypeModule.use(), which adds a boolean return that's incompatible with TypePlugin.use(); seems weird and I never use these directly, but it's in the Gio hierarchy and that's enough to cause very distracting and frankly useless type errors.

Having said that, I still run with the stubs because they've found some issues for me... they just need a bit of love and time.

@lazka
Copy link
Member

lazka commented Aug 2, 2019

Maybe the new python/mypy#6830 would help by just ignoring all the errors in the stubs.

@kaiw
Copy link
Collaborator Author

kaiw commented Aug 9, 2019

I've just tested in 3.7, and there the behaviour is to completely ignore the entire file. In other words, if I add # type: ignore to the top of a stubs file, it's as though the file is empty. I think the behaviour is the same in 3.8, but I'm not 100% clear and haven't tested.

kaiw added 10 commits August 11, 2019 07:57
These aren't in the module requirements, and aren't installed by default
on non-Debian-based systems.
The check for whether a record is private assumes a fixed child node
structure that isn't the case in some environments. This patch tries
to keep the spirit of the check by filtering out all known-unimportant
child nodes and simply checking whether there's anything left.

The expected unimportant nodes are:

 * empty text nodes containing formatting white-space
 * source-position elements that map the member to the original source
   file

neither of which indicate that the member actually needs to be
annotated.
This is needed for e.g., classes that inherit from both Atk.Object and
Atk.Action, which both define methods like `get_description` with
very different signatures.
GObject-derived widgets always accept keyword args for setting GObject
properties. Ideally we'd check that the kwargs matched the object's
properties, but for now we'll just accept all kwargs to avoid the type
errors.
This is currently only for GObject, but there's several cases where
backwards-compatible aliases have been provided.
Ideally this would come from the GI constructor information, but we
don't maintain that through to the point of use here yet. This list
should be able to be removed with some work in the future though.
@kaiw
Copy link
Collaborator Author

kaiw commented Aug 14, 2019

I only recently realised that the Liskov violations are split into direct subclass issues and multiple inheritance ones. I've started on a list of exclusions for the direct subclass problems, and that's silenced a lot of noise. The multiple-inheritance-based violations are workable, but I'm hitting python/mypy#7336. I think I can find a workaround, but it might be fun.

Object.Property and Object.Signal are the main two missing parts for
general use, and both require hand-written stub information.

The Property stub here is probably wrong in various ways and could
definitely be a lot better, but it's better than nothing.
@skewty
Copy link

skewty commented Feb 17, 2020

This seems to be stalled (no activity for 6 months). Is there anything I can do to help this along?

pygobject/pygobject-stubs#5

@kaiw
Copy link
Collaborator Author

kaiw commented Feb 20, 2020

Every once in a while I pick this up again and make a small amount of progress. Every time I've been stymied a bit by mypy assumptions/limitations, but honestly mostly by the way GTK's class hierarchy (and its mapping into overrides) works... it just doesn't match some common assumptions of type systems.

There's probably things that could be done to work around these, but it's just a lot of work to go through and figure out how to handle each individual case.

Honestly, I think the only way to make progress here is to try to use the generated stubs on a real application and just... pick the first error (or lack of error where one should be) and figure out what's going wrong. I have managed to get some small but real examples correctly type-checked this way, but it also doesn't take long to run into real issues that need thought.

@bochecha
Copy link

Honestly, I think the only way to make progress here is to try to use the generated stubs on a real application and just... pick the first error (or lack of error where one should be) and figure out what's going wrong.

I'm happy to help with this for my limited usage. (mostly GLib.Variant at the moment)

Should we just merge this as it is, and then we can all work together on making it better once it's easier to use?

@kaiw
Copy link
Collaborator Author

kaiw commented Feb 22, 2020

I'm happy to leave the merging decision to others.

In my personal opinion, I wouldn't merge this because I couldn't in good conscience suggest that someone use the generated stubs unless they were willing to contribute to fixing them. Also, the stub generation currently has no test coverage. I started writing some and immediately ran into issues that I still haven't had time/motivation to resolve.

One of the "simplest" ways to contribute is to pick a single class with a wide range of corner cases and try to isolate stub generation for that and manually check that the stubs are correct. I've been using classes like Gdk.Color because it has a mix of regular attributes and methods, weird constructor behaviour, class methods and Python overrides. Currently several of these don't work correctly (constructor behaviour and overrides, from memory).

@MrinmoyHaloi
Copy link

There is manually written stubs here. I guess that is enough

@ghost
Copy link

ghost commented Mar 14, 2022

There is manually written stubs here. I guess that is enough

Thanks for the link. Works well, but Gtk3-only. No support for Gtk4 yet

Found a little bit of discussion about Gtk4 here. No movement on it yet though

EDIT: Someone on gnome IRC alerted me to this project, and it seems to be working well for me with Gtk4. https://gitlab.gnome.org/GabMus/gi-stubgen

Run ./gen, cd into out/, and copy the gi/ directory into your site-packages. I had to install gtksourceview5 and webkit2gtk-5.0 prior to running gen.py

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.

5 participants