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

A way to avoid the need for AccessConvert? #77

Closed
steffahn opened this issue Jul 5, 2022 · 4 comments
Closed

A way to avoid the need for AccessConvert? #77

steffahn opened this issue Jul 5, 2022 · 4 comments

Comments

@steffahn
Copy link
Contributor

steffahn commented Jul 5, 2022

Looks like something like

diff --git a/src/access.rs b/src/access.rs
index be4ddfd..5ffcdaa 100644
--- a/src/access.rs
+++ b/src/access.rs
@@ -112,13 +112,52 @@ pub trait Access<T> {
     fn load(&self) -> Self::Guard;
 }
 
-impl<T, A: Access<T>, P: Deref<Target = A>> Access<T> for P {
+impl<T, A: Access<T> + ?Sized, P: Deref<Target = A>> Access<T> for P {
     type Guard = A::Guard;
     fn load(&self) -> Self::Guard {
         self.deref().load()
     }
 }
 
+impl<T> Access<T> for dyn DynAccess<T> + '_ {
+    type Guard = DynGuard<T>;
+
+    fn load(&self) -> Self::Guard {
+        self.load()
+    }
+}
+
+impl<T> Access<T> for dyn DynAccess<T> + '_ + Send {
+    type Guard = DynGuard<T>;
+
+    fn load(&self) -> Self::Guard {
+        self.load()
+    }
+}
+
+impl<T> Access<T> for dyn DynAccess<T> + '_ + Sync + Send {
+    type Guard = DynGuard<T>;
+
+    fn load(&self) -> Self::Guard {
+        self.load()
+    }
+}
+
+#[cfg(test)]
+mod test {
+    use super::*;
+    fn _expect_access<T>(_: impl Access<T>) {}
+    fn _test1<T>(x: Box<dyn DynAccess<T> + '_>) {
+        _expect_access(x)
+    }
+    fn _test2<T>(x: Box<dyn DynAccess<T> + '_ + Send>) {
+        _expect_access(x)
+    }
+    fn _test3<T>(x: Box<dyn DynAccess<T> + '_ + Send + Sync>) {
+        _expect_access(x)
+    }
+}
+
 impl<T: RefCnt, S: Strategy<T>> Access<T> for ArcSwapAny<T, S> {
     type Guard = Guard<T, S>;

could avoid the need for the AccessConvert wrapper, right?

Edit: Ah, the actual AccessConvert is somewhat more general, as it allows – say – Box<dyn Foo> for a trait Foo: DynAccess, too. Still, this change could make many use cases of AccessConvert unnecessary, I suppose; and it’s not even a breaking change. (It should e.g. be documented though, if this becomes a PR.)

@steffahn steffahn changed the title A way to avoid the need for AccessConvert A way to avoid the need for AccessConvert? Jul 5, 2022
@vorner
Copy link
Owner

vorner commented Jul 5, 2022

I think I was trying to come up with something like that back then. I don't know why it failed for me, maybe there were some kind of conflicting implementations. Would you try making it into a PR (maybe without the docs first) to see if it passes all the tests, including on the old but still supported platforms?

@steffahn
Copy link
Contributor Author

steffahn commented Jul 5, 2022

@vorner here you go: #78

@steffahn
Copy link
Contributor Author

steffahn commented Jul 5, 2022

and it’s not even a breaking change

I mean, one might think it is because the blanket impl gets more general with the added ?Sized; but since rustc doesn’t even appear to like straightforward user-code such as

use arc_swap::access::Access;

trait Foo {}

impl Access<()> for Box<dyn Foo> {
    type Guard = &'static ();
    fn load(&self) -> &'static () {
        &()
    }
}

without complaining about alleged “overlap”, despite the fact that the <Box<dyn Foo> as Deref>::Target is not Sized, makes me think that there might actually be no breakage happening here.

@vorner
Copy link
Owner

vorner commented Dec 25, 2022

The #78 PR got cherry-picked and merged as part of #85, will be part of the next release.

Thank you

@vorner vorner closed this as completed Dec 25, 2022
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

No branches or pull requests

2 participants