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

FnMut type should be used with a mutable reference #970

Merged
merged 1 commit into from
Aug 5, 2023

Conversation

EFanZh
Copy link
Contributor

@EFanZh EFanZh commented Jul 24, 2023

No description provided.

@EFanZh EFanZh changed the title Mutable reference should be used with an FnMut type FnMut type should be used with a mutable reference Jul 24, 2023
@ehuss
Copy link
Contributor

ehuss commented Jul 24, 2023

Thanks, can you provide some more context on this change? What is the motivation for it?

@EFanZh
Copy link
Contributor Author

EFanZh commented Jul 25, 2023

Since C is of type FnMut(Oid) -> bool, calling it would require a mutable reference. Currently, with_hide_callback accepts a shared reference (&C), so theoretically, it should not able to call the function. I think this is a soundness issue, since it essentially converts &C to &mut C.

Copy link
Contributor

@ehuss ehuss left a comment

Choose a reason for hiding this comment

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

OK, I think I understand now. The previous API would allow creating multiple immutable references when it shouldn't. I'm not entirely clear how that can manifest as an issue (since there are several lifetime restrictions in place). It would be interesting to see some example that would be a problem.

I'm also curious if there is some more ergonomic API that could handle this situation. Requiring &mut for closures that aren't FnMut seems a bit unfortunate. I tried a few ideas but couldn't come up with anything that would work. If anyone has ideas, it would be appreciated.

I'm going to go ahead and merge since this seems correct to me, and I don't have any ideas on how to make it better.

@ehuss ehuss enabled auto-merge August 5, 2023 19:00
@ehuss ehuss added this pull request to the merge queue Aug 5, 2023
Merged via the queue into rust-lang:master with commit 414dede Aug 5, 2023
7 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.

2 participants