Remove unbound return type from Logger.getMessageFactory()

[Marcono1234]

Commit 2e9fdc135545eee435cad14ee9d74101c93694be added the unbound return type to allow easier usage with MessageFactory2. However, an unbound return type causes loss of type safety: You could claim that the return type of getMessageFactory() is XYZ even though there is no guarantee for that.

Commit 3bd605d2c4eb24657396d4fe4cee78edc3d2c1b6 deprecated MessageFactory2, therefore the need for this unbound return type is not as urgent anymore.

---

Edit: This change will however break source compatibility.

[garydgregory]

This will break source compatibility though.

[Marcono1234]

Hm, yes you are right. Do you think this might be acceptable, or is that not worth it?

I have commented a few weeks ago on LOG4J2-1418 when I was not aware that MessageFactory2 had been deprecated. However, I have not got any response there.

Should the method instead at least get a big warning text that it is not type-safe?

[garydgregory]

IMO I'm not sure it's worth mentioning because this is actually a Java feature and can you can say that any method is 'not type-safe' if it is declared in the patten <T> T foo().

[Marcono1234]

and you can say that any method is 'not type-safe' if it is declared in the patten <T> T foo().

And I will say this 😄 Though to clarify, the issue I am having with this is that the return type is not bound, i.e. none of the parameters of method (in this case there are no parameters) refer to this type parameter. Using type parameters, which are bound, as return type is perfectly fine.

Personally I think such constructs should be used very sparingly and only where it is absolutely necessary. There is a reason why the compiler complains about the implementation of this method in AbstractLogger and you have to use @SuppressWarnings("unchecked") to suppress that warning.

The problem with these unbound return types is that the user of the API might not even notice it because to them it just looks like any other method. But is allows them to write something like the following which won't cause any errors during compile time, not even any warnings, but will break during runtime:

interface MyCustomMessageFactory extends MessageFactory {
}

Logger logger = LogManager.getLogger();
// Compiles without any warnings or errors
MyCustomMessageFactory f = logger.getMessageFactory();

[rgoers]

This is a patch against master (what will be Log4j 2 3.0.0). MessageFactory2 really should be removed from master since it is no longer needed. I don't have a problem with custom MessageFactories having to be modified to implement MessageFactory instead of MessageFactory2 when they upgrade from 2.x to 3.0 as long as that is captured in upgrade notes (which no one has created yet).

[ppkarwasz]

This was closed automatically by Github because we renamed the master branch to main. Feel free to resubmit to the main branch.

[Marcono1234]

@ppkarwasz, was the renaming performed by hand? According to the GitHub documentation, renaming on GitHub should have automatically updated the base branch for pull requests. Though now it is too late anyways, maybe that is useful for you next time though.

[ppkarwasz]

I don't know the details since @vy performed the renaming. Anyway something did not work as expected.

[vy]

@Marcono1234, ASF projects don't have access to project settings view in GitHub. We can indirectly change settings via .asf.yaml files. Hence, yes, I needed to roll out the branch renaming myself. I think we can and will do better next time.