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

Make cast method produced by implement macro unsafe #1753

Merged
merged 5 commits into from
May 11, 2022

Conversation

rylev
Copy link
Contributor

@rylev rylev commented May 11, 2022

Fixes #1183

note: this PR originally removed the cast method all together, but I then discovered that's not really desirable, so I added it back in.*

The implement macro creates a cast method for the types using implement macro that allowed users to query for arbitrary interfaces directly from the implementing type. This method, however, is very hard to use properly as it required that not only self be heap allocated and pinned but that it be allocated inside the type's corresponding _Impl struct. While this is often the case, the compiler does not enforce this being the case. When there are safety invariants the compiler can't enforce, unsafe is the right tool for informing users to be careful.

This PR makes the cast method unsafe which should at least draw the user's attention to the requirements described above.

Additionally, this PR adds an alloc method. This method does the dance of placing the type inside the _Impl struct, heap allocating it, and then calling QueryInterface to query for the supplied interface. This is essentially the dynamic equivalent of the various From impls the implement macro already creates (which can skip calling QueryInterface since we know those interfaces are implemented).

alloc still has the issue of consuming self meaning that once alloc is called the user can no longer use the original implementation type. However, if the user needs access to the original type, the user can query for one of the interfaces used in the implement macro which will implement the AsImpl trait allowing the user to retrieve an & reference to the original type. Unfortunately, if the call to alloc fails (presumably because the type does not implement the interface being queried for), the user will no longer have any access to the underlying type (and it will be freed).

Some questions and thoughts:

  • Making cast unsafe is quite unfortunate, but currently the best thing we can do.
    • We could try to enforce the heap/pinned requirement, but this would require all the traits take a self: &Pin<Box<Self>> which is certainly not pretty.
    • There's no way to really enforce the invariant that the implementation type be embedded in the _Impl type. I think, unfortunately, the right thing to do is to make all trait methods for a COM interface unsafe. Then it would be up to the caller to ensure this invariant.
  • Is the new alloc function actually useful? Would it perhaps be a better idea to remove it?
  • If we keep alloc, should we consider returning self on failure so the user can keep using it?
  • Would this have been better as an issue or an RFC? The change seemed to be mostly in service of a bug fix and (at least originally) rather small in scope, so a PR felt appropriate.

@rylev rylev requested a review from kennykerr May 11, 2022 10:21
@rylev rylev changed the title Remove cast method produces by implement macro Make cast method produced by implement macro unsafe May 11, 2022
@rylev rylev force-pushed the remove-implement-cast branch from ff2300d to efdbbc8 Compare May 11, 2022 12:43
@kennykerr
Copy link
Collaborator

Is the new alloc function actually useful? Would it perhaps be a better idea to remove it?

I'm not aware of any need that the cast function doesn't serve. I would err on the side of avoiding adding a new function until there's clearly a need for it.

@@ -5,12 +5,12 @@ use windows::UI::Xaml::*;
// TODO: This is a compile-only test for now until #81 is further along and can provide composable test classes.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI: @kennykerr this refers to issue #81 which has been closed. Is there a more appropriate issue to link to here?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, we should create one. I've been reluctant to delve into this as it would take a few hundred lines of code to test and only applies to Xaml which I've been contemplating dropping support for entirely as it is largely unusable in Rust anyway.

@rylev rylev merged commit c178210 into microsoft:master May 11, 2022
@rylev rylev deleted the remove-implement-cast branch May 11, 2022 15:38
@mominul
Copy link
Contributor

mominul commented May 12, 2022

@rylev An FAQ/wiki entry describing the usage of cast method produced by implement macro would be beneficial for those who don't know much about COM or holding the safety invariants.

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.

Wrong cast() call causes access violation at vtable access
3 participants