Add Log implementation for &impl Log and Arc<impl Log> #471

2021-11-15T16:59:59-06:00

piegamesde commented

2021-11-15 16:59:59 -06:00

(Migrated from github.com)

Closes #458. Turns out that Rc<Log> isn't possible because Log: Sync. Other candidates for adding:

Cow
Option
() (this would allow us to get rid of the internal NopLogger)
Pin (?)
Probably forgot something

Closes #458. Turns out that `Rc<Log>` isn't possible because `Log: Sync`. Other candidates for adding: - `Cow` - `Option` - `()` (this would allow us to get rid of the internal `NopLogger`) - `Pin` (?) - Probably forgot something

KodrAus (Migrated from github.com) reviewed

2021-11-15 17:24:05 -06:00

src/lib.rs

					
				@ -1250,6 +1250,22 @@ impl Log for NopLogger {

				    fn flush(&self) {}

Normally I'd write something like (**self).enabled(metadata) when forwarding implementations on references. I just thought I'd check to make sure this isn't a possible infinite recursion 🙂

Normally I'd write something like `(**self).enabled(metadata)` when forwarding implementations on references. I just thought I'd check to make sure this isn't a possible infinite recursion 🙂

piegamesde (Migrated from github.com) reviewed

2021-11-15 17:28:52 -06:00

src/lib.rs

					
				@ -1250,6 +1250,22 @@ impl Log for NopLogger {

				    fn flush(&self) {}

I'd forgotten the * and I had a warning, then I added it and the warning went away so we should be good. I could add a test that would otherwise trigger the infinite recursion if you like.

KodrAus commented

2021-11-15 17:36:59 -06:00

(Migrated from github.com)

Thanks @piegamesde! I think this is a great starting point. I think ideally we'd stick to implementations that can unconditionally forward to another implementation, like Box<T>, Arc<T>, &T. Just to avoid any possible confusion over how something like Option<T> or () should actually behave.

Thanks @piegamesde! I think this is a great starting point. I think ideally we'd stick to implementations that can unconditionally forward to another implementation, like `Box<T>`, `Arc<T>`, `&T`. Just to avoid any possible confusion over how something like `Option<T>` or `()` should actually behave.

piegamesde commented

2021-11-15 17:41:39 -06:00

(Migrated from github.com)

I'd argue that the intent for Option<T> is rather clear and for () more or less as well. But I agree that this can be overdone (looking at you, Haskell) and one has to draw the line somewhere.

This still leaves Pin and Cow open. I'd be in favor of Pin because I don't see any reason not to (the first person coming up with a legitimate use case for Pin<Box<Log>> wins a cookie). For Cow, I don't know. Technically the implementation would simply forward along its Borrow so its good?

I'd argue that the intent for `Option<T>` is rather clear and for `()` more or less as well. But I agree that this can be overdone (looking at you, Haskell) and one has to draw the line somewhere. This still leaves `Pin` and `Cow` open. I'd be in favor of `Pin` because I don't see any reason *not* to (the first person coming up with a legitimate use case for `Pin<Box<Log>>` wins a cookie). For `Cow`, I don't know. *Technically* the implementation would simply forward along its `Borrow` so its good?

KodrAus commented

2021-11-15 18:03:34 -06:00

(Migrated from github.com)

Yeh, there are subtleties in the semantics that I don't think the benefit outweighs for Option or (). Pin and Cow both do seem reasonable. Would you like to include them in this PR or follow-up with another?

Yeh, there are subtleties in the semantics that I don't think the benefit outweighs for `Option` or `()`. `Pin` and `Cow` both do seem reasonable. Would you like to include them in this PR or follow-up with another?

KodrAus (Migrated from github.com) reviewed

2021-11-15 18:04:14 -06:00

src/lib.rs

					
				@ -1250,6 +1250,22 @@ impl Log for NopLogger {

				    fn flush(&self) {}

The more tests the merrier!

piegamesde commented

2021-11-16 05:51:00 -06:00

(Migrated from github.com)

Would you like to include them in this PR or follow-up with another?

Let's do all that make sense right now. I added Pin, but I had to back off Cow because it's Clone constraint on T. This is something that does make little sense for loggers. This is not the first time I've wished for some MaybeOwned in Rust, i.e. a Cow without the "clone" and the "write" part.

For the potential recursion issue mentioned in review, I added #![deny(unconditional_recursion)] instead of a test.

> Would you like to include them in this PR or follow-up with another? Let's do all that make sense right now. I added `Pin`, but I had to back off `Cow` because it's `Clone` constraint on `T`. This is something that does make little sense for loggers. This is not the first time I've wished for some `MaybeOwned` in Rust, i.e. a `Cow` without the "clone" and the "write" part. For the potential recursion issue mentioned in review, I added `#![deny(unconditional_recursion)]` instead of a test.

👍 1

KodrAus commented

2021-11-16 06:54:38 -06:00

(Migrated from github.com)

Ah it looks like we might need to back Pin out too 🙂 It was actually still unstable in 1.31.0

Ah it looks like we might need to back `Pin` out too 🙂 It was actually still unstable in 1.31.0

piegamesde commented

2021-11-16 08:34:53 -06:00

(Migrated from github.com)

Yes, I've seen the CI fail too. Is there a way to feature guard this only for newer Rust versions? Edit: found it: https://github.com/dtolnay/rustversion#use-cases

Yes, I've seen the CI fail too. Is there a way to feature guard this only for newer Rust versions? **Edit:** found it: https://github.com/dtolnay/rustversion#use-cases

KodrAus commented

2021-11-17 04:47:28 -06:00

(Migrated from github.com)

Ah unfortunately we can’t really introduce any unconditional dependencies such as rustversion to log. We could do some version detection in the build script, but the best option for now might be to simply roll back Pin.

Ah unfortunately we can’t really introduce any unconditional dependencies such as `rustversion` to `log`. We could do some version detection in the build script, but the best option for now might be to simply roll back `Pin`.

piegamesde commented

2021-11-17 11:22:51 -06:00

(Migrated from github.com)

I removed the Pin implementation in a second commit so that you can simply revert it after the next MSRV bump. Feel free to squash it if you prefer it that way.

I removed the `Pin` implementation in a second commit so that you can simply revert it after the next MSRV bump. Feel free to squash it if you prefer it that way.

KodrAus (Migrated from github.com) approved these changes

2021-11-19 06:46:11 -06:00

KodrAus (Migrated from github.com) left a comment