make logging work with trimming

[JeffBezanson]

Note this is against #55047.
@topolarity I moved this to a separate branch since it has test failures and can be finished after merging #55047.

[timholy]

Thanks for doing this! Constraining the types seems to be the only hope for making the logging system play nice with invalidations and static compilation. It might be possible to remove some of the @invokelatests now?

I wanted to do a version of this during the invalidation hunt during 1.6 development, but I was concerned that it would be a breaking change. Is the idea that the new normalization methods give us a way to impose constraints in a way that allows packages to be fixed?

This seems like a change that could be submitted independently, and a PkgEval run might shake out any issues early.

[timholy]

Suggested change

	Applied to all kwargs seen by handle message, except `maxlog` which is not
	Applied to all kwargs seen by `handle_message`, except `maxlog` which is not

[timholy]

Suggested change

	normalize_message(logger::AbstractLogger, context::Any, message::Any) = message
	normalize_message(logger::AbstractLogger, context::Any, message::Any) = convert(Union{String,Base.AnnotatedString{String}}, message)::Union{String,Base.AnnotatedString{String}}

Otherwise aren't you at risk for an unclear MethodError or that people will address this by adding new handle_message methods that accept something besides Union{String,Base.AnnotatedString{String}}? Whereas this version at least makes the "contract" clearer. (One could of course use string instead, it's the type-assert that seems most important.)

Likewise with normalize_kwarg below.

[fredrikekre]

See also #40820. I have also seen "non-conforming" types in the wild.

[topolarity]

Thanks for doing this! Constraining the types seems to be the only hope for making the logging system play nice with invalidations and static compilation. It might be possible to remove some of the @invokelatests now?

The @invokelatest's should be provided by #55111 once that lands

Is the idea that the new normalization methods give us a way to impose constraints in a way that allows packages to be fixed?

Unfortunately, no. As-is, this PR expects to have the logger type inferrable (that's what makes invalidating shouldlog(::AbstractLogger, ...) not catastrophic, even without the TypedCallable/invokelatest pieces)

That means this PR is cheating though, since it can resolve the multiple dispatch of handle_message(logger::AbstractLogger, message::Any) statically by fixing the first argument type. The problem with properly supporting a dynamic logger type are:

1. TypedCallable is fundamentally single dispatch (or at least, it requires all of your type information to be available simultaneously, at the creation site of the callable) so we need another way to thread the logger type L and object type T to the call-site for the compiler to know it needs to compile, e.g., handle_message(::MyLogger, ::YourFoo)
2. Even if we find a way to thread the type information through the compiler appropriately, that still leads to a quadratic blow-up in the amount of logging/serialization code.

Part of that problem is that loggers in general can serialize objects in "any" way.

One way to tackle that problem might be to split the "serialization type" from the "logger type", so that the combinatorics improve? If we can do that without breaking the ecosystem, that would be my vote for a path forward I think.

This seems like a change that could be submitted independently, and a PkgEval run might shake out any issues early.

Totally agreed - Seems like the way to go

There was already an accidental breakage in 1.6 for custom log levels, which has lead to some type piracy in the ecosystem: https://github.com/chmerdon/GradientRobustMultiPhysics.jl/blob/ef6a7c81ee395a1a33a5bd90bd28e10d4bba4ca0/src/logging.jl#L2-L19, so that will need a bit of clean up too

[timholy]

Oof, sorry about that 1.6 breakage. I wonder why it wasn't picked up by PkgEval. I guess it's not technically piracy because they own CustomLevel, but certainly the intent is not to have to specialize that method.

[topolarity]

I guess it's not technically piracy because they own CustomLevel, but certainly the intent is not to have to specialize that method.

Hah, that is true - but it does create a composability problem since now the CustomLevel doesn't work with a new custom logger

Thankfully in this case, I think the interface for custom log levels is pretty easy-to-define (and also "single dispatch", since the behavior within that interface shouldn't really depend on the logger)

[vtjnash]

Looks like this is significantly conflicted and outdated