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

The constructor of _FuncPointer can accept None to paramflags and Structure to iid in runtime. #8968

Closed
junkmd opened this issue Oct 23, 2022 · 20 comments

Comments

@junkmd
Copy link
Contributor

junkmd commented Oct 23, 2022

def WINFUNCTYPE(
restype: type[_CData] | None, *argtypes: type[_CData], use_errno: bool = ..., use_last_error: bool = ...
) -> type[_FuncPointer]: ...

class _FuncPointer(_PointerLike, _CData):
restype: type[_CData] | Callable[[int], Any] | None
argtypes: Sequence[type[_CData]]
errcheck: _ECT
@overload
def __init__(self, address: int) -> None: ...
@overload
def __init__(self, callable: Callable[..., Any]) -> None: ...
@overload
def __init__(self, func_spec: tuple[str | int, CDLL], paramflags: tuple[_PF, ...] = ...) -> None: ...
@overload
def __init__(self, vtlb_index: int, name: str, paramflags: tuple[_PF, ...] = ..., iid: _Pointer[c_int] = ...) -> None: ...
def __call__(self, *args: Any, **kwargs: Any) -> Any: ...

This is simplified my project code.
And it can run in Py2.7 and Py3.3-3.10.

            # argyypes: tuple[type[_CData], ...]
            # vtbl_offset: int
            # i: int
            # _iid_: subclass of `Structure` instance, not a pointer of that.
            # paramflags: tuple[...] | None
            prototype = WINFUNCTYPE(restype, *argtypes)
            if restype == HRESULT:
                ...
                raw_func = prototype(i + vtbl_offset, name, None, _iid_)   # <- pylance error
                func = prototype(i + vtbl_offset, name, paramflags, _iid_)   # <- pylance error
                ...

Probably this is a false-positive.

I think that line 102 of ctypes/__init__.pyi will be fixed as below.

    def __init__(self, vtlb_index: int, name: str, paramflags: tuple[_PF, ...] | None = ..., iid: _CData = ...) -> None: ... 

Any opinions would be appreciated.

@AmberAnsari89
Copy link
Contributor

hi,
i want to work on it.please assign to me if nobody is working on it.

@junkmd
Copy link
Contributor Author

junkmd commented Nov 3, 2022

@AmberAnsari89
Hi

Ok, I'll leave it for you.

I also noticed that iid can take None, so please take that into consideration as well.

@AmberAnsari89
Copy link
Contributor

i analysed the issue and have few questions to ask.

  1. _pointer[_ct] is generally a optional interface identifier to extend the error tracking.. why _cdata in place of it. _cdata is generally a base class for ctype data types.
  2. structure, itself is a subclass of __StructUnionBase and it is subclass of _cdata. then what is the iid in this case?

I am new a new contributor to this repository and need your help to understand the functional thought behind the changes.
your help definitely guide me to get the better understanding..

thanks
Amber

@junkmd
Copy link
Contributor Author

junkmd commented Nov 13, 2022

@AmberAnsari89

see this code snippet. This is available in runtime.

So I think that _FuncPointer should be changed as follows.

_PF: TypeAlias = _UnionT[tuple[int], tuple[int, str | None], tuple[int, str | None, Any]]

class _FuncPointer(_PointerLike, _CData):
    ...
    @overload
    def __init__(self, vtlb_index: int, name: str, paramflags: tuple[_PF, ...] | None = ..., iid: _CData | None = ...) -> None: ...

@junkmd
Copy link
Contributor Author

junkmd commented Nov 13, 2022

@AmberAnsari89

This part is closely tied to the C implementation.

I posted this issue because it doesn't make sense to me that the code that runs at runtime has an error in the type checker.

I haven't read that much of the C implementation code and wanted to get the views of anyone familiar with the ctypes implementation.

@AmberAnsari89
Copy link
Contributor

@junkmd
you are right iid should be structure type as it is taking GUID interface value. And GUID is the subclass of structure. we can update the iid to structure class.

however we have look for documentation also.please advise

@junkmd
Copy link
Contributor Author

junkmd commented Nov 15, 2022

@AmberAnsari89

I have read the C implementation.

I am not familiar with C, but from the conditional branching of the overloading process, I can interpret it to accept None for paramflags or its elements, or _CData or None for iid.

https://github.com/python/cpython/blob/50b0415127009119882e32377d25a4d191088a76/Modules/_ctypes/_ctypes.c#L3377-L3440

https://github.com/python/cpython/blob/50b0415127009119882e32377d25a4d191088a76/Modules/_ctypes/_ctypes.c#L3588-L3612

@AmberAnsari89
Copy link
Contributor

@junkmd
i went through the above shared link..i am ok with paramflags as tuple or None..regarding iid -
self = (PyCFuncPtrObject *)GenericPyCData_new(type, args, kwds);
self->index = index + 0x1000;
self->paramflags = Py_XNewRef(paramflags);
if (iid_len == sizeof(GUID))
self->iid = iid;
return (PyObject *)self;

here, PyCFuncPtrObject* is converting the returrning pyobject to ctype and storing to self.then setting the each value, including iid in ctype. and finally. converting the self to pyObject and return..

from these inference, you are deriving that iid should be cData or None. am i right?

thanks

@junkmd
Copy link
Contributor Author

junkmd commented Nov 16, 2022

@AmberAnsari89

Yes, I think you're right.

@AmberAnsari89
Copy link
Contributor

thank you..
i will do the changes.

@AmberAnsari89
Copy link
Contributor

i did the changes and below issue i got while performing pulling request..
comtypes (https://github.com/enthought/comtypes)

  • comtypes/_memberspec.py:358: error: Argument 3 to "_FuncPointer" has incompatible type "Optional[Tuple[Union[Tuple[int, Optional[str]], Tuple[int, Optional[str], Any]], ...]]"; expected "Optional[Tuple[Union[Tuple[int], Tuple[int, str], Tuple[int, str, Any]], ...]]" [arg-type]
  • comtypes/_memberspec.py:357: error: No overload variant of "_FuncPointer" matches argument types "int", "str", "None", "Optional[GUID]" [call-overload]
  • comtypes/_memberspec.py:357: note: Possible overload variants:
  • comtypes/_memberspec.py:357: note: def init(self, address: int) -> _FuncPointer
  • comtypes/_memberspec.py:357: note: def init(self, callable: Callable[..., Any]) -> _FuncPointer
  • comtypes/_memberspec.py:357: note: def init(self, func_spec: Tuple[Union[str, int], CDLL], paramflags: Tuple[Union[Tuple[int], Tuple[int, str], Tuple[int, str, Any]], ...] = ...) -> _FuncPointer
  • comtypes/_memberspec.py:357: note: def init(self, vtlb_index: int, name: str, paramflags: Tuple[Union[Tuple[int], Tuple[int, str], Tuple[int, str, Any]], ...] = ..., iid: _Pointer[c_int] = ...) -> _FuncPointer
  • comtypes/_memberspec.py:358: error: No overload variant of "_FuncPointer" matches argument types "int", "str", "Optional[Tuple[Union[Tuple[int, Optional[str]], Tuple[int, Optional[str], Any]], ...]]", "Optional[GUID]" [call-overload]
  • comtypes/_memberspec.py:358: note: Possible overload variants:
  • comtypes/_memberspec.py:358: note: def init(self, address: int) -> _FuncPointer
  • comtypes/_memberspec.py:358: note: def init(self, callable: Callable[..., Any]) -> _FuncPointer
  • comtypes/_memberspec.py:358: note: def init(self, func_spec: Tuple[Union[str, int], CDLL], paramflags: Tuple[Union[Tuple[int], Tuple[int, str], Tuple[int, str, Any]], ...] = ...) -> _FuncPointer
  • comtypes/_memberspec.py:358: note: def init(self, vtlb_index: int, name: str, paramflags: Tuple[Union[Tuple[int], Tuple[int, str], Tuple[int, str, Any]], ...] = ..., iid: _Pointer[c_int] = ...) -> _FuncPointer

@AmberAnsari89
Copy link
Contributor

the type hints of paramflag is different from _functPointer overload in comtypes

@junkmd
Copy link
Contributor Author

junkmd commented Nov 20, 2022

@AmberAnsari89

I think the following _PF needs to be changed as well.

_PF: TypeAlias = _UnionT[tuple[int], tuple[int, str], tuple[int, str, Any]]

_PF: TypeAlias = _UnionT[tuple[int], tuple[int, str | None], tuple[int, str | None, Any]]

@AmberAnsari89
Copy link
Contributor

Optional[Tuple[Union[Tuple[int, Optional[str]], Tuple[int, Optional[str], Any]], ...]]

here 2 arguments are given however in _PF 3 arguments..though union wise it looks same. Will it not create any problem?
will such changes not impact other packages? otherwise we have to looks where it is being used.

Thanks

@junkmd
Copy link
Contributor Author

junkmd commented Nov 22, 2022

@AmberAnsari89

The Unions of comtypes are written in a legacy way for backward compatibility and is non-intuitive compared to the A | B in typeshed.

https://github.com/enthought/comtypes/blob/30523b53fbd5560d5bde2e722607c860cdf743dd/comtypes/_memberspec.py#L11-L13

The ParamFlagType of comtypes in the typeshed writing style would be the following.

ParamFlagType: TypeAlias = _UnionT[tuple[int, str | None], tuple[int, str | None, Any]]

This is contained in _UnionT[tuple[int], tuple[int, str | None], tuple[int, str | None, Any]].

The purpose of this change is to ensure that type checking does not cause an error if it is safe to pass it as an argument at runtime.
This change "allows more types than previously allowed". It does not "disallow types that were previously allowed”.
So the existing code should not cause any errors by the type checker.

@junkmd
Copy link
Contributor Author

junkmd commented May 3, 2023

I think this issue should be addressed after the class definitions have been moved to _ctypes.pyi and names have been changed to CFuncPtr.
Please see #10132.

@junkmd
Copy link
Contributor Author

junkmd commented May 7, 2023

@AmberAnsari89

The definitions of these constructors have been moved to _ctypes.pyi and the code has changed a lot since I first posted this issue.

This issue has also not moved for about 6 months.
I will write a PR about this.

Thank you for your cooperation.

@junkmd
Copy link
Contributor Author

junkmd commented May 7, 2023

I close this because #10155 is merged.

Thank you.

@junkmd junkmd closed this as completed May 7, 2023
@AlexWaygood
Copy link
Member

Thank you for all your work on ctypes stubs, @junkmd!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants