rust-lang / log

Logging implementation for Rust
https://docs.rs/log
Apache License 2.0
2.12k stars 248 forks source link

Allow constructing LevelFilter via incrementation #624

Open neithernut opened 3 months ago

neithernut commented 3 months ago

When implementing a command line application where logging is somewhat important (e.g. a longer-running service), I sometimes like to make the verbosity configurable via multiple sources such as configuration files, environment variables and command line options. However, for the latter I usually want a somewhat different behavior. Rather than the number of -v (or --verbose) flags setting the verbosity directly, I want each -v to increase the level from whatever the baseline is, which may come from a config file or some default level.

Currently, this requires some awkward code in applications. If we had the possibility to create a LevelFilter from an integer, this would be trivial. This was proposed in the past but clearly rejected (#318, #460). However, I didn't yet find anyone proposing the possibility of creating a level by incrementing some LevelFilter.

That could be done by either:

The latter clearly needed to be feature gated (which would be awkward) and/or increase the MSRV, but would allow using LevelFilter with std::ops::Ranges as an Iterator in the future. And of course, those options are not mutually exclusive. The std::iter::Step impl can follow at some later point if/when it gets stabilized.

Thomasdezeeuw commented 3 months ago

You can use min((level_filter as usize) + 1, Trace::Trace as usize) as LevelFilter as LevelFilter has #[repr(usize)] to do this already.

I'm not sure this a common enough case that we need the crate, though.

neithernut commented 3 months ago

Are you sure? All versions of Rust which I tried this on say that the cast from usize to LevelFilter is illegal[^1]. Given that the representation is known you can still hack the equivalent of C++'s reinterpret_cast, but...

[^1]: I would provide a link to https://play.rust-lang.org, but it appears to be broken right now.

Thomasdezeeuw commented 3 months ago

This works: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=1f583a914a1b153a80213f3cd78c8676 and it's safe only because of #[repr(usize)]. However you have to make sure the value is valid, i.e. not bigger than 5/LevelFilter::Trace, but the transmute itself is not UB.

neithernut commented 3 months ago

Agreed, but it would still be nice not having to resort to unsafe, even if it is factually safe.

Thomasdezeeuw commented 3 months ago

Agreed, but it would still be nice not having to resort to unsafe, even if it is factually safe.

Not really, the compiler can't ensure you do 6 as LevelFilter, which would be UB (5 is max valid value).

neithernut commented 3 months ago

Agreed, but it would still be nice not having to resort to unsafe, even if it is factually safe.

Not really, the compiler can't ensure you do 6 as LevelFilter, which would be UB (5 is max valid value).

Yes, I know. I meant that with an API like TryFrom<usize> or the ones I suggested in my initial post would remove the need for library users to use unsafe code like the one you posted earlier.

Thomasdezeeuw commented 3 months ago

Which brings us back to

I'm not sure this a common enough case that we need the crate, though.

Zoybean commented 1 month ago

I agree that some kind of increment/decrement on log levels and/or filters would be useful. It would also circumvent the stated reasons the other issues were rejected (#318, #460). Namely, it doesn't increase exposure of the underlying integer representation to users. It still does, however, expose the exact sequence of elements, e.g. adding a new level between two existing levels might break someone's code. I don't know if that is an issue.

neithernut commented 1 month ago

It still does, however, expose the exact sequence of elements, e.g. adding a new level between two existing levels might break someone's code. I don't know if that is an issue.

The sequence is already exposed via the Iterator returned by LevelFilter::iter. A new feature for iterating over levels will thus at least not introduce this issue.