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

Rework of struct definition #209

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

Conversation

zwergziege
Copy link

This PR is a rework of how structs are defined and is an improvement in three points:

This PR supersedes / is based on #185, which already contains some discussion of the changes introduced in this PR. It should mitigate the concerns raised by @mara004 related to #185 but has a slightly larger change set.

@zwergziege
Copy link
Author

If I'm not mistaken the definition of anonymous fields in _anonymous_ could (and probably should) be moved to the class definition, leaving only the construction of fields to be done after class definition.

@mara004
Copy link
Contributor

mara004 commented Jan 25, 2024

If I'm not mistaken the definition of anonymous fields in _anonymous_ could (and probably should) be moved to the class definition, leaving only the construction of fields to be done after class definition.

Yes, I think so.

It should mitigate the concerns raised by @mara004

It may address some of the points I mentioned, but I still don't like the idea of complicating ctypesgen on behalf of type hints. If this PR were addressed at my fork I'd most likely reject it.
In my eyes, type hints here are basically a workaround for the dumbness of static analyzers. The information is in the ._fields_ and .argtypes already.
As an alternative to changing ctypesgen, could you maybe write a separate script to create a .pyi type interface file from the existing info?

That said, could you wait for #205 and then rebase ?
That would result in a cleaner step-by-step progress: first just the fix and code move, then the possible new feature.

@zwergziege
Copy link
Author

I still don't like the idea of complicating ctypesgen on behalf of type hints.

I would consider calling a seven line function in a place that is not time critical a bearable complication for having type hints.

In my eyes, type hints here are basically a workaround for the dumbness of static analyzers. The information is in the ._fields_ and .argtypes already.

Maybe I missed something, but it seems to me that you're saying that static analyzers should look for packages that include type information in some non-standard way instead of sticking to well established conventions about type hinting. In this case I strongly disagree. Also in the general case, the assignment of _fields_ needs not be static. Even in our setting _fields_ is set well after the creation of the class and for a good reason (enabling cyclic dependencies). Static analyzers are not stupid for not executing your code, they are just static. So while I do agree that type hints should come out of the box, I see it in the responsibility of ctypes to adjust to modern python. There is an issue related to that (python/cpython#104533 (comment)), but it's not particularly fast moving and supporting cyclic dependencies would probably require some effort in ctypes or a change in how incomplete types in annotations are handled (for which I think there exists a PEP). I might be dead until all that gets through. In the meantime this PR seems to be a workaround that works well enough for now.

As an alternative to changing ctypesgen, could you maybe write a separate script to create a .pyi type interface file from the existing info?

Thank you for the suggestion, but tbh I won't look into it. I'm already happy. I have my own fork that works for me. This PR is just to give the option to the maintainers to pick stuff that I found helpful. The amount I use ctypesgen does not justify reimplementing my stuff in another way.

That said, could you wait for #205 and then rebase ?
That would result in a cleaner step-by-step progress: first just the fix and code move, then the possible new feature.

I don't think that I'm the right person to address concerning that regard ;) Not that my opinion would matter but I think that the change set is small enough for a single PR. However, I of course agree that a clean separation of individual changes would be marginally better 🤷

@mara004
Copy link
Contributor

mara004 commented Jan 25, 2024

Maybe I missed something, but it seems to me that you're saying that static analyzers should look for packages that include type information in some non-standard way instead of sticking to well established conventions about type hinting

No. I just mean it's silly from a human POV -- all the info is in there already, and we aren't crafting it dynamically, just declaring it in a slightly different fashion.

Static analyzers are not stupid for not executing your code, they are just static.

Sure, the dumbness lies not in the implementation, but in the nature of the concept.
The problem is, it's only a benefit to IDEs, not to the actual functionality.

Thank you for the suggestion, but tbh I won't look into it. I'm already happy. I have my own fork that works for me. This PR is just to give the option to the maintainers to pick stuff that I found helpful. The amount I use ctypesgen does not justify reimplementing my stuff in another way.

I understand, but IMO we need another way for optional type hints.
Perspectively, I just don't want this merged in a codebase I'm working with.

@mara004
Copy link
Contributor

mara004 commented Jan 25, 2024

Historically, the problem has been that ctypesgen got so cluttered with templates over time that the codebase and output became a real mess.

My design goal is to revert that and stick to lean ctypes, and keep things simple in the future.
It took me months to check and step-by-step remove all the indirection and leanify the bloated template.
I don't want any templates or workaroundish code again.

I would consider calling a seven line function in a place that is not time critical a bearable complication for having type hints.

Having to do non-standard things like splitting up fields info, resolving annotations, and fusing it back together through a template decorator at import time (or alternatively duplicating all the content) just doesn't align with my idea of clean code.
Also considering that the type hints cycles/self-references and annotations resolution story has been under evolution, making it feel like a chaos to users who are not much into type hints.

@mara004
Copy link
Contributor

mara004 commented Jan 25, 2024

@zwergziege For the record, can you clarify which python version the generated output would now require?
Is it 3.7 ?

@mara004
Copy link
Contributor

mara004 commented Jan 26, 2024

As an alternative to changing ctypesgen, could you maybe write a separate script to create a .pyi type interface file from the existing info?

Or probably easier, declare the type hints in ctypesgen, but behind an opt-in guard, and in that case just accept the duplication with fields. That at least should be a really simple and template-free change. – While this patch results in a smaller output file, it's hard to make optional.

PYI would also mean duplication and probably take more code to do (yet that could be done without touching ctypesgen).

(Edit: That's considering structs only. I don't know how the situation is for functions. It might be that this couldn't be done without wrappers, so maybe PYI would be better anyway...)

@zwergziege
Copy link
Author

@zwergziege For the record, can you clarify which python version the generated output would now require?
Is it 3.7 ?

Using from __future__ import annotations requires 3.7, but that could be replaced by quoted types. Beside this I think it should work with 3.5+.

Also considering that the type hints cycles/self-references and annotations resolution story has been under evolution, making it feel like a chaos to users who are not much into type hints.

typing.get_type_hints has been around since 3.5.

Thank you for sharing your thoughts on PYI. While I don't particularly like the idea of creating stubs when we also create the actual source, I think it could be a good middle ground solution that can be made optional and not overly ugly to implement. Especially for covering type annotations for primitives (where it would be nice to have union types like int | ctypes.int) and functions (where it would be nice to avoid wrappers).

@caffeinepills
Copy link

caffeinepills commented Jun 15, 2024

@zwergziege

You can accomplish something similar with a metaclass:

from typing import get_type_hints

class TypedStruct(type(ctypes.Structure)):
    def __new__(cls, name, bases, namespace):
        class _TempClass:
            __annotations__ = namespace.get('__annotations__', {})
        annotations = get_type_hints(_TempClass)
        namespace['_fields_'] = tuple(annotations.items())
        namespace['__slots__'] = tuple(annotations.keys())
        return type(ctypes.Structure).__new__(cls, name, bases, namespace)

Becomes:

class struct_MyStruct(Structure, metaclass=TypedStruct):
    firstValue: c_float

It's a little cleaner.

@mara004
Copy link
Contributor

mara004 commented Jun 16, 2024

@caffeinepills I don't see how this handles the bitfields? Also, will the slots actually work? (i.e. prevent assignment of nonexistent fields?)

@zwergziege
Copy link
Author

I gave this only a cursory glance but slots should work and adapting for bitfields should be easy.

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.

None yet

3 participants