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

Improve Python type-stubs #2468

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

Conversation

lojack5
Copy link

@lojack5 lojack5 commented Oct 18, 2023

The wxPython type-stubs are nice in that they expose what classes, methods, etc are available, but there is no type information in them (there's even a comment in makePyArgsString referencing implementing this). This is my 90% of the way there implementation of exposing this type information to the type-stubs.

I also didn't find any open issues regarding type-stubs, so I haven't linked an issue.

For the most part, all of the type-information was already there in the doxygen files, it just had to be pulled into the args strings. There are a few exceptions, mostly revolving around types that are C++ typedefs (ex: WindowID and Coord).

By using from __future__ import annotations, all of the type-hints are stringized, so even in the cases where the name referenced is undefined (ex: richtext's TextAttrDimensionFlags), the type-stub files are still valid. The hints in those cases will just provide no information.

I do have a few discussion points to probably resolve before this is truly ready though:

  • From what I can tell by the buildbot output, we build as low as Python 3.6. So I'm assuming any new code doesn't need to be compatible with earlier versions, is this a good assumption?
  • How to handle typing.ParamSpec: related to the above, ParamSpec was added in Python 3.10. Currently it's only used to type-hint CallAfter and CallLater, how would you like this import handled:
    • If you're OK with a new dependency typing-extensions could be added for Python versions < 3.10, then import from there.
    • I can skip out on typing CallAfter and CallLater, although if other methods come up in the future that would benifit from this sort of typing, we run into the issue again.
  • Code style - I tried to keep with the same code style in surrouding areas but I probably named a few variables differently at some point. I also used f-strings since those are in Python 3.6+. I can go back and use Python 2 style formatting strings if that's what's wanted however.
  • There are still some typedef types that aren't covered. I could dig through and find every instance in the PYI files where a type is referenced that's undefined, but that may end up with a lot of hard-coded conversions (usually to int) in the type-name fixup code. Let me know of any changes there I need to make.
  • I have no knowledge of PI files (WingIDE files according to the header), do these changes break those?
  • My choice of how to handle C++ enums. I went the route of using the stdlib's enum.IntEnum and enum.IntFlag here. My reasoning is - they're still typed as ints this way (so compatible), and I define the enums with a prefixed _ so the type-stubs don't expose them as some sort of real type that is actually accessible in actual wxPython code. I then make a TypeAlias for the name that includes the enum values or an int, so type-checkers won't complain to users that pass raw ints into methods typed with these enums. The enum then basically serves the purpose of grouping various values together and signaling the the callers which of the named values are expected at the call sites. I can go back to having enums just being typed as plain old int, but I feel we lose the information of "what enum value or flags should you be using here".

I also just realized while writing this that subscripting list and unions with | weren't added until Python 3.9 and 3.10, respectively, so some for me:

  • Replace list[...] with List[...] and import List from typing.
  • Replace X | Y with Union[X, Y] and import Union from typing.
  • Replace tuple[...] with Tuple[...] and import Tuple from typing.
  • Replace from collections.abc import Callable with from typing import Callable - subscripting was introduced in Python 3.10

The line-wrapping causes issues once the python signatures become too long,
as textwrap isn't smart enough to split the lines on valid continuation points
for code. I had one instance of splitting a line in the middle of a string ->
SyntaxError on next run of etg due to the generated PYI file having an
unterminated string.

Specificially, disable splitting for lines that start (ignoring spaces) with
a specific string - in this case any line starting with the name of the
function or method this is a docstring for.
This allows for building `FixWxPrefix.cleanType` on top of it, for use
in processing type-hint strings in the future. It also exposes the method
to `FunctionDef.makePyArgString` in the future, which has easier access to
the types of arguments and returns. And possibly further in the future,
other `***Def` classes can make use of it (constant definitions, etc).
Leverages the `writeSection` machinery, with a tweak to specify to add a
new section to the beginning of a file, after the header. This ensures
the required imports gets updated (and also only imported once per file)
if new imports are needed for type-hints. Hint: there's a few more to come.
One unexpected type of '...' required adding a new transformation
that modifies both the name and the type to just '*args', so added
a preferred method `FixWxPrefix.parseNameAndType` which processes
both strings at once.

Also fixes cleanType to recursively call cleanType on sub-types
(was improperly calling cleanName).

With this, method and function signatures now have type annotations
which are mostly correct (100% correct in the "it compiles" sense).
Thankfully, the incorrect type-hints don't cause errors due to using
stringized annotations (by importing annotations from __future__).

Importantly, the overload signatures now have been fully sanitized.
Before this, there was one instance of a variable named `is`, and another
named `/Transfer/` - both invalid identifiers. I stopped looking after
those. Since theses signatures are valid Python code, this opens up the
opportunity to use `typing.overload` to fully expose those.

Edge-cases in type-hints will be addressed in later commits.
These will be changing to annotation statements, so FixWxPrefix needs to
be able to detect this still (proper thing to look for in this case is
`ast.AnnAssign`).
With the work from the previous commits, it's as simple as
no longer lopping off the args string at the '->' marker.
(And one minor fixup to the makePyArgsString code).
By directly referencing their setter and getter methods,
and due to the typing work already done for methods, we now have
type information attached to properties.

There are a few edge cases of setters/getters not having the proper
number of arguments for a getter(0) or setter(1), but most cases are
handled. The incorrect number of arguments may be missing default
arguments from what the extraction code figures out from the C++ code?
Use the more generic type rather than a literal type. Before, a type-checker
would infer an int defined this way as `Literal[0]` vs the more correctly
generic `int` for example.
Named enums with 'Flags' in the name use `enum.IntFlag`, all other enums
use `enum.IntEnum`.  We have to bring the enum members into the containing
scope to match the original behaviour. Further, since these enums are typing
information only, and not actually in wx proper, we prevent them from appearing
to leak the the user by marking as non-exported (prepend '_' to the name), then
make a TypeAlias to the enum or an int.  This way type signatures still claim
to accept integers as appropriate.

I like this solution best, because we preserved the information about which
enum members are expected for method/function parameters, while not leaking
non-existant classes out of the type-stubs, and not complaining about using
integers.

There's still a few undefined but referenced enums (ex:
richtext.TextAttrDimensionFlags). These are most likely a union of some of
the other flags/enum types already defined, but the work to pull that information
from the C++ source is probably too much.
This fixes erroneous wx. prepended to `version` variable in methods'
parameters.
Use `typing.Optional` and `typing.Union` where applicable, as direct
union (`|`) type annotations were added in Python 3.10
Subscribing builtins.list wasn't added until Python 3.9 so use
`typing.List` where applicable.
Subscripting builtins.tuple was added in Python 3.9, so use
`typing.Tuple` where applicable.
`collections.abc.Callable` subscripting was added in Python 3.10,
use `typing.Callable` instead.
@@ -3,3 +3,4 @@ numpy < 1.17 ; python_version <= '2.7'
numpy ; python_version >= '3.0' and python_version < '3.12'
# pillow < 3.0
six
typing-extensions; python_version < '3.10'
Copy link
Collaborator

Choose a reason for hiding this comment

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

It looks like perhaps a space is needed before the semicolon?

Copy link
Author

@lojack5 lojack5 Oct 18, 2023

Choose a reason for hiding this comment

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

Syntactically not needed, but I'm more than happy to change it to match the style used in the rest of the file.

Currently working though issues shown to me through the buildbot, so I'll get to it after I solve all of those issues that come up.

BTW, I was looking for a CONTRIBUTING.md or a style guide somewhere but didn't find anything, am I missing it?

@swt2c
Copy link
Collaborator

swt2c commented Oct 18, 2023 via email

Alternate solution is to remove the callable typing on CallAfter and CallLater
Since this is used to not only generate the type-stubs, but also the actual
wx/core.py, we need to ensure TypeVar and ParamSpec are imported there as
well. So, move the imports to the wx/core.py generator definition, and
remove the imports from the type-stub generator (these imports are only
used by CallAfter and CallLater).
@lojack5
Copy link
Author

lojack5 commented Oct 19, 2023

Ok, I think this is ready for review and whatever fixes you want from me. The CI is failing on all Windows builds - appears to be a MSVC version mismatch?

error STL1001: Unexpected compiler version, expected MSVC 19.36 or newer.

For reference, on my machine it builds fine for Python 3.12, Win 11 with the latest MSVC build tools installed.

Buildbot output (for Python 3.11):

Will build using: "C:\hostedtoolcache\windows\Python\3.11.6\x64\python.exe"
3.11.6 (tags/v3.11.6:8b6ee5b, Oct  2 2023, 14:57:12) [MSC v.1935 64 bit (AMD64)]
Python's architecture is 64bit
cfg.VERSION: 4.2.2a1

Running command: build_py
Checking for D:\a\1\s\bin\waf-2.0.24...
Not found.  Attempting to download...
Connection successful...
Data downloaded...
Checking for D:\a\1\s\bin\waf-2.0.24...
CL.exe: C:\Program Files\Microsoft Visual Studio\2022\Enterprise\VC\Tools\MSVC\14.37.32822\bin\HostX64\x64\cl.exe
"C:\hostedtoolcache\windows\Python\3.11.6\x64\python.exe" D:\a\1\s\bin\waf-2.0.24 --msvc_arch=x64 --jobs=4 --python="C:\hostedtoolcache\windows\Python\3.11.6\x64\python.exe" --out=build/waf/3.11/x64/release configure build 
Setting top to                           : D:\a\1\s 
Setting out to                           : D:\a\1\s\build\waf\3.11\x64\release 
Checking for program 'CL'                : C:\Program Files\Microsoft Visual Studio\2022\Enterprise\VC\Tools\MSVC\14.35.32215\bin\HostX64\x64\CL.exe 
Checking for program 'CL'                : C:\Program Files\Microsoft Visual Studio\2022\Enterprise\VC\Tools\MSVC\14.35.32215\bin\HostX64\x64\CL.exe 
Checking for program 'LINK'              : C:\Program Files\Microsoft Visual Studio\2022\Enterprise\VC\Tools\MSVC\14.35.32215\bin\HostX64\x64\LINK.exe 
Checking for program 'LIB'               : C:\Program Files\Microsoft Visual Studio\2022\Enterprise\VC\Tools\MSVC\14.35.32215\bin\HostX64\x64\LIB.exe 
Checking for program 'MT'                : C:\Program Files (x86)\Windows Kits\10\bin\10.0.22621.0\\x64\MT.exe 
Checking for program 'RC'                : C:\Program Files (x86)\Windows Kits\10\bin\10.0.22621.0\\x64\RC.exe 
Checking for program 'python'            : C:\hostedtoolcache\windows\Python\3.11.6\x64\python.exe 
Checking for python version >= 3.7.0     : 3.11.6 
'configure' finished successfully (3.838s)

vs what I see on my machine:

Will build using: "C:\Users\Lojack\Desktop\code\wxPython\.venv\Scripts\python.exe"
3.12.0 (tags/v3.12.0:0fb18b0, Oct  2 2023, 13:03:39) [MSC v.1935 64 bit (AMD64)]
Python's architecture is 64bit
cfg.VERSION: 4.2.2a1

Running command: build
Running command: build_wx
CL.exe: C:\Program Files\Microsoft Visual Studio\2022\Community\VC\Tools\MSVC\14.37.32822\bin\HostX64\x64\cl.exe
wxWidgets build options: ['--wxpython', '--unicode']
Updating wx/msw/setup.h
setting build options...
nmake.exe -f makefile.vc UNICODE=1 OFFICIAL_BUILD=1 COMPILER_VERSION=140 SHARED=1 MONOLITHIC=0 USE_OPENGL=1 USE_GDIPLUS=1 BUILD=release

Microsoft (R) Program Maintenance Utility Version 14.37.32825.0
Copyright (C) Microsoft Corporation.  All rights reserved.

WARNING: msgfmt and/or make commands not found, message catalogs not 
         rebuilt. Please install gettext and associated tools.
Finished command: build_wx (0m1.6s)
Running command: build_py
Checking for C:\Users\Lojack\Desktop\code\wxPython\bin\waf-2.0.24...
CL.exe: C:\Program Files\Microsoft Visual Studio\2022\Community\VC\Tools\MSVC\14.37.32822\bin\HostX64\x64\cl.exe
"C:\Users\Lojack\Desktop\code\wxPython\.venv\Scripts\python.exe" C:\Users\Lojack\Desktop\code\wxPython\bin\waf-2.0.24 --msvc_arch=x64 --python="C:\Users\Lojack\Desktop\code\wxPython\.venv\Scripts\python.exe" --out=build/waf/3.12/x64/release configure build
Setting top to                           : C:\Users\Lojack\Desktop\code\wxPython 
Setting out to                           : C:\Users\Lojack\Desktop\code\wxPython\build\waf\3.12\x64\release
Checking for program 'CL'                : C:\Program Files\Microsoft Visual Studio\2022\Community\VC\Tools\MSVC\14.37.32822\bin\HostX64\x64\CL.exe 
Checking for program 'CL'                : C:\Program Files\Microsoft Visual Studio\2022\Community\VC\Tools\MSVC\14.37.32822\bin\HostX64\x64\CL.exe 
Checking for program 'LINK'              : C:\Program Files\Microsoft Visual Studio\2022\Community\VC\Tools\MSVC\14.37.32822\bin\HostX64\x64\LINK.exe 
Checking for program 'LIB'               : C:\Program Files\Microsoft Visual Studio\2022\Community\VC\Tools\MSVC\14.37.32822\bin\HostX64\x64\LIB.exe
Checking for program 'MT'                : C:\Program Files (x86)\Windows Kits\10\bin\10.0.22621.0\\x64\MT.exe
Checking for program 'RC'                : C:\Program Files (x86)\Windows Kits\10\bin\10.0.22621.0\\x64\RC.exe
Checking for program 'python'            : C:\Users\Lojack\Desktop\code\wxPython\.venv\Scripts\python.exe
Checking for python version >= 3.7.0     : 3.12.0 
'configure' finished successfully (2.133s)
Waf: Entering directory `C:\Users\Lojack\Desktop\code\wxPython\build\waf\3.12\x64\release'
Waf: Leaving directory `C:\Users\Lojack\Desktop\code\wxPython\build\waf\3.12\x64\release'
'build' finished successfully (0.518s)

The only thing that sticks out to me is the buildbot is calling into executables found in 14.35.32215, whereas on my machine its 14.37.32822.

@lojack5 lojack5 marked this pull request as ready for review October 19, 2023 15:15
@lojack5
Copy link
Author

lojack5 commented Oct 23, 2023

@swt2c sorry to bug. Are you able to review this (or help track down the windows compile error)? Or maybe know who to ping for it?

@swt2c
Copy link
Collaborator

swt2c commented Oct 23, 2023

@swt2c sorry to bug. Are you able to review this (or help track down the windows compile error)? Or maybe know who to ping for it?

Yes, I will try to track down the windows compiler error eventually. Too much to do and too little time.. :(

@Metallicow

This comment was marked as off-topic.

@swt2c swt2c closed this Oct 31, 2023
@swt2c swt2c reopened this Oct 31, 2023
@lojack5
Copy link
Author

lojack5 commented Nov 1, 2023

Not sure what you did on the backend, but now everything except Python 3.7 are succeeding, nice! I looked into that, there wasn't even a build run for Python 3.7, so maybe a mismatch between what the GitHub check wants and what Azure was commanded to build?

@Metallicow

This comment was marked as off-topic.

@Metallicow

This comment was marked as off-topic.

@lojack5
Copy link
Author

lojack5 commented Nov 2, 2023

???

@Metallicow

This comment was marked as off-topic.

@Metallicow

This comment was marked as off-topic.

@Metallicow

This comment was marked as off-topic.

@swt2c
Copy link
Collaborator

swt2c commented Nov 2, 2023

Not sure what you did on the backend, but now everything except Python 3.7 are succeeding, nice! I looked into that, there wasn't even a build run for Python 3.7, so maybe a mismatch between what the GitHub check wants and what Azure was commanded to build?

Yeah, I fixed the CI builds and also removed Python 3.7 as it is EOL. Your old builds with Python 3.7 before that removal are still there. So at least your changes all build. Unfortunately, I think these changes probably need to be reviewed by @RobinD42 before merging.

@lojack5
Copy link
Author

lojack5 commented Nov 2, 2023

Awesome :). Thanks for the work getting that cleared up (and the thread cleanup). Hopefully Robin has some time in the future to look at this

@lojack5
Copy link
Author

lojack5 commented Nov 13, 2023

Just checking in to see if any dev's have time to review this.

@Metallicow
Copy link
Contributor

Metallicow commented Dec 5, 2023 via email

@Metallicow
Copy link
Contributor

Metallicow commented Dec 5, 2023 via email

@lojack5
Copy link
Author

lojack5 commented Feb 25, 2024

Checking back in on this, any chance for a dev to take a look at this?

@Metallicow
Copy link
Contributor

Metallicow commented Feb 26, 2024 via email

@timrid
Copy link
Contributor

timrid commented Nov 12, 2024

Now Python 3.7 and Python 3.8 are no longer available due to EOL.

Hopefully the pipeline will now run without errors.

Since Python 3.9 is now the minimum supported version and supports PEP585, the following points can be changed back:

  • typing.list[...] to list[...]
  • typing.Tuple[...] to tuple[...]

(If we wait another year with this PR, even Python 3.9 will be discontinued and we can do without typing_extension and typing.Union. But I hope that the PR will be merged before then :) )

@swt2c swt2c closed this Nov 12, 2024
@swt2c swt2c reopened this Nov 12, 2024
@lojack5
Copy link
Author

lojack5 commented Nov 16, 2024

I had totally forgotten about this after giving up thinking this would never be looked at again. Do you want me to go through and pull out the pre-3.9 compatibility code? It's not needed to do so, but would make the generated code a little cleaner looking (older typing stuff can certainly get in the way visually).

@swt2c
Copy link
Collaborator

swt2c commented Nov 16, 2024

I had totally forgotten about this after giving up thinking this would never be looked at again. Do you want me to go through and pull out the pre-3.9 compatibility code? It's not needed to do so, but would make the generated code a little cleaner looking (older typing stuff can certainly get in the way visually).

Really sorry about that, Robin has been on an extended leave of absence and I've taken over doing releases and slowly been trying to work through reviewing PRs.

I'd say you can probably leave the pre-3.9 code for now, just in case someone still wants to build for 3.8 in the short term.

@lojack5
Copy link
Author

lojack5 commented Nov 17, 2024

That's ok - don't burn yourself out. Taking over responsibilities when there's not many around on a large project can be draining.

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.

4 participants