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

Remove cast from implement macro #3506

Merged

Conversation

sivadeilra
Copy link
Collaborator

This is a cleanup item for my PR #3055. That PR (and many others that I submitted around the same time) greatly improved support for creating COM objects. This PR cleans up a few minor things from that work.

First, this PR deletes the generated cast method on types marked with #[implement]. The cast method was an older mechanism for casting from an object to one of its interfaces. It is extremely dangerous because it relies on memory constraints that are not verifiable (which is why it is marked unsafe). My previous PRs provided much better ways to handle the need for this, with the to_interface, as_interface etc. methods. So cast is dangerous and unneeded; let's just remove it. This is a breaking change, but it's needed.

Second, the PR removes an incorrect type bound on a method in ComObject. This is harmless.

@sivadeilra sivadeilra force-pushed the user/ardavis/remove-obsolete-com branch from 6980878 to 26e93d5 Compare February 21, 2025 21:27
@riverar
Copy link
Collaborator

riverar commented Feb 21, 2025

This method is currently in the documentation; what's the proposed new API surface here? Are these methods drop-in replacements or?

@sivadeilra sivadeilra requested a review from Copilot February 21, 2025 21:38

Choose a reason for hiding this comment

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

Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.

@sivadeilra
Copy link
Collaborator Author

sivadeilra commented Feb 21, 2025

This method is currently in the documentation; what's the proposed new API surface here? Are these methods drop-in replacements or?

There are already better replacements: ComObject::as_interface, ComObject::to_interface. You can see in the existing code where I changed uses of cast() to use these functions. The replacements are safe and do not require runtime type checks (QueryInterface calls).

Which docs reference cast? I can update the docs.

Also, note that this PR is about the T::cast method, not the ComObject::cast method, which is unaffected (and is safe).

@riverar
Copy link
Collaborator

riverar commented Feb 21, 2025

Ah, thanks. I also took it for a spin and realized we weren't talking about Interface::cast.

@kennykerr kennykerr changed the title Remove T::cast (unsafe and obsolete) Remove cast from implement macro Feb 21, 2025
Copy link
Collaborator

@kennykerr kennykerr left a comment

Choose a reason for hiding this comment

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

Nice, less scary code is great.

@kennykerr kennykerr merged commit 7417e58 into microsoft:master Feb 21, 2025
34 checks passed
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.

3 participants