`IDiaSymbol::get_*()` methods can return `S_FALSE`

[tim-weis]

Most (or maybe all) of the get_*() methods of IDiaSymbol can return S_FALSE (in addition to S_OK). The specific return value is currently inaccessible as the signatures are transformed to return a Result<>.

To make those methods usable, they will need to retain their original signature and return an HRESULT.

[tim-weis]

Unless someone is working on this already I can prepare a PR that addresses this issue.

I assume this is controlled via the options.rsp response file during WinMD generation, using the [CanReturnMultipleSuccessValues] attribute. Let me know if this is correct.

[riverar]

Yup, go for it. Thanks!

[tim-weis]

That's a fair bit of API surface to cover; part of me regrets offering help already ;)

Anyway, while I hit a few issues I'd like to keep notes on building this crate. Do you have a preferred location where a "build instructions" document should go?

On a related note, the get_test_session function is currently hardcoded to reference VS 2022 Enterprise. This works in CI but fails on my local system (and my workaround will fail during CI, presumably).

I'd also like to address this but I don't know exactly how. Potential options are delegating this to the vswhere tool, accessing the Microsoft.VisualStudio.Setup.Configuration API, and probably a few more. Any preference here (and that could include "Don't bother")?

[kennykerr]

Sounds like a fair bit of work - there's obviously no obligation that you take this on.

Let's also avoid a monolithic PR and rather make small incremental improvements wherever possible.

[riverar]

If you can split up the changes needed, happy to assist as well.

Regarding the DLL, I'd say leave that as an exercise to the user for now and we'll fix it next round.

[tim-weis]

Let's also avoid a monolithic PR and rather make small incremental improvements wherever possible.

@kennykerr No worries. I hadn't planned to dump everything into a giant PR. My local signatures branch is focused on adjusting signatures alone. As I was going down in alphabetical order, the first method (findChildren) I updated hit a lot of territory (metadata, bindings, tests, samples). The remaining changes will be much more local.

Regarding the DLL, I'd say leave that as an exercise to the user for now and we'll fix it next round.

@riverar That's reasonable. It's an inconvenience (that I know how to work around), not a roadblock. I guess I shouldn't have abused this issue as a diary of everything that crossed my mind. I can submit individual issues instead to keep things organized.

I welcome any assistance and feedback offered. You can follow and comment on my changes over at my fork. I can also create a draft pull request off of it if that makes things easier for you.

One last thing: The idea to have the tests crate validate code as well as generate its own test input is wild. Kudos to whoever thought of doing this.

[riverar]

I welcome any assistance and feedback offered. You can follow and comment on my changes over at my fork. I can also create a draft pull request off of it if that makes things easier for you.

Don't want to step on your toes; we'll need to coordinate and divvy up the work somehow. Unless you want to keep this focused on just IDiaSymbol::get_ methods.

[tim-weis]

I welcome any assistance and feedback offered. You can follow and comment on my changes over at my fork. I can also create a draft pull request off of it if that makes things easier for you.

Don't want to step on your toes; we'll need to coordinate and divvy up the work somehow. Unless you want to keep this focused on just IDiaSymbol::get_ methods.

That's fair. This issue does explicitly name the get_* methods (that form the lion's share of the IDiaSymbol API) but the pattern of "successfully failing" also applies to the find* methods (with two exceptions). That's where I started to get familiar with the source base.

I can do either of the following:

• Change the topic of this issue to include the entire IDiaSymbol API
• Roll back my local changes and stay with the get_* methods

Let me know whichever you prefer. I'm about to wind down for today and will be back on Thursday.

[riverar]

@tim-weis Might as well continue; I can work with you on your fork/repo. In the meantime, I'm going to unblock some folks and cut another release. We can then release an update after you're done.

[tim-weis]

I went ahead and applied the [CanReturnMultipleSuccessValues] attribute to applicable IDiaSymbol methods (that's 224 out of 236). The bindings generated—consequently—turned very clunky across the board (evidenced by the updated samples and tests).

While these changes faithfully reflect the documented call contracts, I can predict that clients won't be happy with them. Should we consider a different approach that supports mapping S_OK/S_FALSE/E_*-returning methods to familiar language primitives (such as Result<Option<T>> in Rust)?

I also observed several documentation bugs (mis-named and mis-typed method signatures). I could create PRs against the documentation repository but I'm unsure whether that has lasting effect, seeing that the documentation is (or at least at some point was) generated from an opaque source. Any guidance on that?

[riverar]

Dang.

This began as a small proof-of-concept crate but now appears to be evolving beyond that stage. It's likely time to stop exposing all the generated code and design a more idiomatic and stable API surface.

Regarding documentation: I’ve had great success editing Microsoft Docs in the past, but with all the user feedback channels now closed, I suspect you may face some challenges. It's worth a shot!

[kennykerr]

This began as a small proof-of-concept crate but now appears to be evolving beyond that stage. It's likely time to stop exposing all the generated code and design a more idiomatic and stable API surface.

💯

[tim-weis]

It's likely time to stop exposing all the generated code and design a more idiomatic and stable API surface.

I'm not sold on this.

The DIA SDK is ever-evolving in exposing compiler and linker internals. I don't know how sustainable a more manual approach is (if that is what you're suggesting). While the API surface is progressing, its shape remains consistent: Standard COM error reporting with the addition of the S_FALSE success code translating to "not available".

This could be handled by windows-bindgen, in theory, provided sufficient metadata (I understand the implications... don't scold me for suggesting this 😉). The result would be pretty much ideal from a client's perspective.

While I'm not holding my breath, what are the plans for this crate going forward? I'd used it semi-frequently and it always delivered. I'd hate to see it go away.