mhammond / pywin32

Python for Windows (pywin32) Extensions
4.91k stars 783 forks source link

Fix named arguments in win32com.client.dynamic.Dispatch #2150

Open Chronial opened 7 months ago

Chronial commented 7 months ago

Problem

Given an IDispatch interface with a function

HRESULT MyFun([in, optional] VARIANT arg1, [in, optional] VARIANT arg2);

The following code misbehaves:

d = win32com.client.dynamic.Dispatch(...)
d.MyFun(arg2="hi")

Instead of passing the value of arg2, it is silently ignored. So the behavior is identical to d.MyFun().

Caused By

The dynamic client uses pythoncom.Missing as its default value for non-given arguments.

But if any Missing argument value is encountered, argument processing is stopped and any later arguments are not processed. This causes us to silently ignore named arguments that are later in the argument order:

https://github.com/mhammond/pywin32/blob/f7d0a79a7ba3c83b8e6f6c62c617f4eb9770a1e9/com/win32com/src/PyIDispatch.cpp#L369-L375

Context

This was already fixed by 6a3be4b00519fc87e6495964bff450d1acadf8ee for genpy-generated interfaces, but the dynamic interface was seemingly forgotten back then. This somewhat worsens the issue because now the two interfaces have different behavior.

Fix

See PR.

The value of pythoncom.Empty is chosen to be consistent with the genpy code. Using pythoncom.ArgNotFound seems more correct to me, but that should probably then be changed in both implementations and is a separate issue.

mhammond commented 7 months ago

This has the same problem I mentioned in #2149 - back in the day this would cause some objects to break. There was no single answer I could find that worked everywhere so I erred on the side of b/w compat. Sadly I'm not sure how true that is today, but also aren't really in a position to test and find out.

Chronial commented 7 months ago

The current state is unfortunately rather broken and has repeatedly been causing Issues for our customers over the last few years. Last week we again got a support request saying something like: "I use someObj.functionA(doTheThing=true) of your application, but it stopped doing The Thing. It hasn't worked all week. It worked last week, but it just stopped working around noon on Monday".

The fact that only one of the genpy and dynamic implementations suffers from this bug makes this very unpredictable. Emptying out the Windows Temp folder suddenly breaks working scripts.

I would argue that fixing that inconsistency is worth the backwards compatibility risk.

Also, the pythoncom.Empty behavior is what's officially documented:

https://learn.microsoft.com/en-us/previous-versions/windows/desktop/automat/passing-parameters If the [ommited] arguments are positional (unnamed), you would set cArgs to the total number of possible arguments, cNamedArgs to 0, and pass VT_ERROR as the type of the omitted arguments, with the status code DISP_E_PARAMNOTFOUND as the value. also: http://web.archive.org/web/20130518102011/http://support.microsoft.com/kb/154039

And what's described in the literature:

https://thrysoee.dk/InsideCOM+/ch05d.htm When you use position arguments, you must still define optional parameters. However, you should set the type to VT_ERROR and the value to DISP_E_PARAMNOTFOUND, indicating that the parameter is not actually being passed, as shown below

mhammond commented 7 months ago

Emptying out the Windows Temp folder suddenly breaks working scripts.

Sadly this is true in various other cases too, particularly when trying to index items - eg, obj.foo[i] doesn't really work with dynamic objects very reliably.

The general solution/advice there is to use EnsureDispatch which should regenerate the support even when the generated files are removed.

I would argue that fixing that inconsistency is worth the backwards compatibility risk.

I can agree with that in general, but the question is still who exactly breaks, and does that influence our choice? ie, it's one thing for the docs to say what they say, but it's another thing entirely if one choice makes working with (say) Office worse.

I'm not saying it will, but really just saying I'm a little reluctant to make such changes blind to what the actual consequences are to heavily used COM objects.

It might also be the case that the fact we (say) made things worse for Office simply points at other issues we should address at the same time, which would mean there's no need to choose one over the other.

Chronial commented 7 months ago

The general solution/advice there is to use EnsureDispatch which should regenerate the support even when the generated files are removed.

Yes, that obviously solves the issue, but only after somebody has successfully noticed it and identified the cause.

I would argue that fixing that inconsistency is worth the backwards compatibility risk.

I can agree with that in general, but the question is still who exactly breaks, and does that influence our choice? ie, it's one thing for the docs to say what they say, but it's another thing entirely if one choice makes working with (say) Office worse.

I'm not saying it will, but really just saying I'm a little reluctant to make such changes blind to what the actual consequences are to heavily used COM objects.

It might also be the case that the fact we (say) made things worse for Office simply points at other issues we should address at the same time, which would mean there's no need to choose one over the other.

Anything that's implemented in C++ using the usual MS Toolchain shouldn't break, because calling DispInvoke() fills in the missing parameters in exactly the same way.

And given that EnsureDispatch/makepy have been behaving this way for 21 years now, I would be rather hopeful that somebody would have reported any serious breakage with a popular application. Clearly, various people are using e.g. Word with EnsureDispatch: [1] [2] [3].

Is there anything that would make you more confident that this is safe? I fixed and ran testMSOffice.py against a current Office and it is not affected by this change.