Use more log::warns in the code

[Manishearth]

We have a bunch of debug asserts that could probably be log::warn for better user experience.

Perhaps also have a strict_assert feature that switches between warning and panicking for testing purposes,.

[robertbastian]

+1 to logging, -1 to panicking based on a feature. We should condition on debug_assertions and say we're panic-free in release mode.

[sffc]

Should the warnings be behind a feature? Should we route all the warnings through an intermediate crate like icu_provider?

Discuss with:

• @sffc
• @Manishearth
• @zbraniecki

[Manishearth]

+1 behind a feature, am ambivalent about routing through icu_provider but I think that makes sense as a global toggle instead of adding features everywhere

[sffc]

• @robertbastian - People don't normally depend on the icu_provider crate. We have cases already where people need to add that dep just to enable the feature.
• @zbraniecki - I think I agree, but separately, is there a reason you choose logging over tracing?
• @Manishearth - I think it makes sense to have a feature on the metacrate, but I think it should go through icu_provider. Even though the log crate is low overhead when you disable it, it's not completely free. I'm okay people depending on icu_provider for patterns like this. Maybe we should make icu_core which is the public API of cross things. But I think icu_provider is fine.
• @robertbastian - I like re-exporting the feature on the metacrate; we could also do it for sync.
• @Manishearth - On the topic of tracing... it's an independent crate in the tokio space that does tracing but better. Tracing is much more structured and path-based and you get a lot more context. The tracing crate implements that. The reason we shouldn't use it, I think, is that as a library it makes sense to use something more low-level. We could perhaps expose it as a toggle.
• @zbraniecki - rust compiler switched to tracing 3 y ago - Use tracing instead of log rust-lang/compiler-team#331
• @Manishearth - Yeah but the Rust compiler is an application.
• @zbraniecki - Tracing provides better behavior in async contexts.
• @sffc - Does tracing let us do the same feature gating as log?
• @Manishearth - Yes
• @sffc - Datagen is one of the main places we use logging so async support seems valuable
• @zbraniecki - Agree
• @sffc - Since we already have global features like serde and std, we could consider sync and tracing as more examples.
• @Manishearth - I don't like polluting our feature space. We could just export a macro from icu_provider that gives you a feature-gated log.
• @zbraniecki - I think it's good to not let people selectively log.
• @Manishearth - Once you have logging enabled globally, then you can start filtering.
• @robertbastian - I agree with a global toggle.

@Manishearth proposal: the feature exists in 2 places: icu and icu_provider (or could be icu_core). When you enable it, everyone in your dependency tree gets it.

• @robertbastian - The serde feature enables additional dependencies, so it's not the same thing.
• @Manishearth - Yeah, here we're exposing additional functionality, not additional dependencies.
• @sffc - If we mediate the feature through icu_provider, then you don't have access to all the functionality from crates. datagen, for example, probably wants the full functionaity.
• @Manishearth - datagen could have the feature.
• @sffc - Can we also get rid of icu_provider/log_error_context and just group it with the main feature?
• @sffc - What should the feature be called?

Conclusion:

• Add features icu/logging, icu_provider/logging, and icu_datagen/logging
• Enable all other logging-related features from the logging feature
• Library crates should use the common logging utilities exported by icu_provider, but binary packages can use the logging crates directly
• For now, continue with the log crate, but we can upgrade it later, still using the same logging feature name

LGTM: @sffc @Manishearth @robertbastian @skius @zbraniecki

[robertbastian]

icu_datagen currently doesn't have a logging feature and logging is a pretty fundamental part of its behaviour. Logging can actually be turned off by not registering a logger, so I'm not sure we need a separate feature...

[sffc]

It would be nice if all ICU4X docs tests and unit tests would have logging enabled. Currently we have logs in the code to catch possibly-errorful conditions, but those logs are suppressed, making it difficult to debug (@atcupps).

Context: The use case is that if we don't recognize a calendar BCP-47 identifier, we fall back to the locale's default calendar, which is correct, expected logic, and we have a warning printed when this fallback occurs, but that warning is not being printed in unit tests.