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

relax lifetime constraints on AtomicRef #25

Merged
merged 1 commit into from
Oct 10, 2023

Conversation

SrTobi
Copy link
Contributor

@SrTobi SrTobi commented Oct 9, 2023

fixes and tests the problem described in #24.

I had to set editon to 2018, to make the async test work. If this is undesirable, I can move the test to a different crate.

I'm reasonable confident, that the changes are correct, because the compiler would generate even less restrictive Sync/Send implementation in a previous version.

impl<'b, T: ?Sized> Send for AtomicRefMut<'b, T> where
    T: Send

On the other hand, I have to admit, that I'm not really understanding why this change fixes #24...
pub struct AtomicRef<'b, T: ?Sized + 'b> has T: 'b after all.

Maybe @Imberflur can weigh in?

@Imberflur
Copy link
Contributor

My best guess is that #24 is caused by rust-lang/rust#110338 and the explicit T: 'b on the Send impl adds some constraint that the compiler currently can't solve in this context.

I used the for<'a>: &... hrtb bounds here since it is less error prone than manually writing out the required bounds on T, they should be equivalent, and I couldn't find any cases where they caused issues. However, given that there are cases where they are confusing the compiler, I think it would be best to write the required bounds for T directly.

I think these are the correct bounds, but you can double check them against the previously generated bounds:
T: Sync + Send -> AtomicRef: Send
T: Sync -> AtomicRef: Send
T: Sync -> AtomicRefMut: Sync
T: Send -> AtomicRefMut: Send

@Imberflur
Copy link
Contributor

It's interesting that T: 'b doesn't need to be written here when it is included on the struct definition. I think this is because lifetime bounds can be implied.

@SrTobi
Copy link
Contributor Author

SrTobi commented Oct 10, 2023

Yeah, I was surprised as well... and also that it leads to this inference problem... I could have bet that it's because of the hrtb. but no... those are fine

@Imberflur
Copy link
Contributor

Yeah, I was surprised as well... and also that it leads to this inference problem... I could have bet that it's because of the hrtb. but no... those are fine

I would be surprised if the hrtb aren't involved though, i.e. I would guess that it is this combined with the hrtb

@Imberflur
Copy link
Contributor

e.g. I'm guessing this would not cause issues even though it keeps the unnecessary T: 'b bound, although I'm not sure 🙃

unsafe impl<'b, T: ?Sized + 'b> Send for AtomicRefMut<'b, T> where T: Send {}

@SrTobi
Copy link
Contributor Author

SrTobi commented Oct 10, 2023

No, I removed them first, because I thought they were the reason... but the error stayed... only removing 'b from T fixed it...

I also think we can leave the for<'a> &'a mut T: * ... that's a good trick to let the compiler do the reasoning, as you are right, AtomicRefMut<T> is equivalent to &mut T

@Imberflur
Copy link
Contributor

No, I removed them first, because I thought they were the reason... but the error stayed... only removing 'b from T fixed it...

Oh!

@Imberflur
Copy link
Contributor

I also think we can leave the for<'a> &'a mut T: * ... that's a good trick to let the compiler do the reasoning, as you are right, AtomicRefMut is equivalent to &mut T

That sounds reasonable to me then.

@bholley bholley merged commit 77d1519 into bholley:master Oct 10, 2023
@bholley
Copy link
Owner

bholley commented Oct 10, 2023

Thanks!

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.

AtomicRefMut is not Sync nor Send in async block
3 participants