initial draft for downcastable Log

[djugei]

fixes #666.
i do not expect this to be merged as is, this is more of a discussion helper. Names are up for bikeshedding.
technically this is not a breaking change i think, as anything that implements log automatically implements LogAny though i agree that it looks weird.

can't just have a method on Log sadly as that would introduce a Sized requirement (for some reason). Can't add the Any bound to Log itself as it would need to be 'static

i have tested this with indicatif-log-bridge and env_logger and it works

[KodrAus]

Thanks for the PR @djugei! I think we can get away without the as_any method, since recent Rust releases support dyn trait upcasting. Maybe something like:

pub trait GlobalLog: Log + Any {}

impl<T: Log + Any> GlobalLog for T {}

I did look at a few alternative designs that try to fully hide the downcasting support, but they all had unsatisfying trade-offs since we accept a Box<dyn Trait> in our public API.

[djugei]

just makes user code a bit uglier
log::logger().as_any().downcast_ref()
(log::logger() as &'static dyn std::any::Any).downcast_ref()
but yes, no additional capabilities so can be removed.

is this feature in general something the project would like to add?

[KodrAus]

I'd be in favour of this approach with the as_any method removed. I think we should call the trait GlobalLog, and use it as our marker for a Log that can be plugged into the shared slot.

[Thomasdezeeuw]

I think this a breaking change. We didn't the std::any::Any trait bound on these functions before, now we effectively do. So we might as well do pub trait Log: std::any::Any { ... }.

[KodrAus]

@Thomasdezeeuw Yeh I checked this as well, and couldn’t actually trigger a compile error by flipping from Box<dyn Log> to Box<dyn GlobalLog>, I believe because of the default 'static object bound. If there’s any chance this is a breaking change though we should avoid it.

Adding an Any bound to Log itself is almost certainly a breaking change.

[Thomasdezeeuw]

Not 100% accurate, but I think https://play.rust-lang.org/?version=stable&mode=debug&edition=2024&gist=7e1712bc6dbdeb86780c85de4b0a7e6c shows it's a problem.

[djugei]

i tried doing Log: Any but that lead to lifetime issues with impl Log for &'_ T: Log so i did a separate trait, though i guess those could be tightened to &'static and it would be fine.

i think any concrete type that implements Log will also implement Any, but if you have a function that bounds only on Log then the compiler is ignorant of that fact, so yeah i guess this is a breaking change after all, though one that can be fixed with minimal changes/no structural changes.

[KodrAus]

Ah yes I see. If you explicitly already have a Box<dyn Log> you can’t pass it to the set_boxed_logger function when we introduce GlobalLog.

That would mean we’d have to introduce a new set of APIs for this if we wanted to do it and deprecate the old ones. I’m not sure that level of churn is worth it.

[djugei]

That would mean we’d have to introduce a new set of APIs for this if we wanted to do it and deprecate the old ones. I’m not sure that level of churn is worth it.

it is even worse than that: since we need to store the logger we have to decide on a concrete type, so those would be 2 completely separate apis, which does not make a lot of sense to me, as it would essentially be keeping two versions. in that case i would just keep one version in the library and let cargo do its job for versioning.

alternatively we can try to think about &'static Log always being Any a bit harder and maybe do an unsafe cast if we can convince ourselves that it is always safe? i sadly do not know a ton about trait objects.

then again this functionality is achievable without using Any at all (see issue) but uses unsafe to do so in the first place. i was hoping for an elegant solution with Any.

[KodrAus]

then again this functionality is achievable without using Any at all (see issue) but uses unsafe to do so in the first place. i was hoping for an elegant solution with Any.

This approach of comparing vtables is unfortunately generally unsound.

Probably the cleanest solution would be to introduce the GlobalLog trait, and add a new set of set_global_logger, global_logger etc. functions using it, and deprecate the old ones. That would be a significant breaking change though to every application using log, so I don't think is one we could consider.

If we could smuggle a type id in the Log trait with a 'static bound, and then use the same trick that the type_id crate does for casting it, relying on the fact that you can't set a global logger unless it's 'static, then we may be able to make it work. I haven't spent a lot of time in that direction so don't know how feasible or sound it is.

[djugei]

Probably the cleanest solution would be to introduce the GlobalLog trait, and add a new set of set_global_logger, global_logger etc. functions using it, and deprecate the old ones. That would be a significant breaking change though to every application using log, so I don't think is one we could consider.

i think the cleanest solution would be to just introduce the GlobalLog trait, accepting and returning it with the existing functions. that way the only breakage would be libraries handelling Log-trait objects in a generic manner. i expect not too many (if any?) of those to exist and the fix would probably always be simple, adding either Any or GlobalLog to the type parameter. No code changes would be required for the vast majority of codebases that simply set a logger, nor the codebases the provide a logger (except when they return a type-errased Log object, but i have not seen that yet, have not looked to hard either though).

[Thomasdezeeuw]

I don't think have another trait to do logging is a great idea, especially if it's not compatible with the existing logger.

[KodrAus]

log is a widely used library so unfortunately we can’t treat breakage in terms of probabilities. If something is technically breaking, chances are someone will be broken by it.

I think we’ve found ourselves a bit limited in options here, so until/unless we consider a breaking change to the library, this is probably not going to be possible to support directly in log.