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

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Chronial
Copy link

@Chronial Chronial commented Nov 22, 2023

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:

// See how many _real_ entries - count until end or
// first param marked as Missing.
for (numArgs = 0; numArgs < argc - 5; numArgs++) {
if (PyTuple_GET_ITEM(args, numArgs + 5)->ob_type == &PyOleMissingType) {
break;
}
}

Context

This was already fixed by 6a3be4b 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.

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

The value of pythoncom.Empty is chosen to be consistent with the genpy code.
@mhammond
Copy link
Owner

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
Copy link
Author

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
Copy link
Owner

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
Copy link
Author

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.

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.

2 participants