You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@logging.apache.org by GitBox <gi...@apache.org> on 2020/12/27 00:31:24 UTC
[GitHub] [logging-log4j2] Marcono1234 opened a new pull request #451: Remove unbound return type from Logger.getMessageFactory()
Marcono1234 opened a new pull request #451:
URL: https://github.com/apache/logging-log4j2/pull/451
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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] garydgregory commented on pull request #451: Remove unbound return type from Logger.getMessageFactory()
Posted by GitBox <gi...@apache.org>.
garydgregory commented on pull request #451:
URL: https://github.com/apache/logging-log4j2/pull/451#issuecomment-751418152
This will break source compatibility though.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] Marcono1234 edited a comment on pull request #451: Remove unbound return type from Logger.getMessageFactory()
Posted by GitBox <gi...@apache.org>.
Marcono1234 edited a comment on pull request #451:
URL: https://github.com/apache/logging-log4j2/pull/451#issuecomment-751490879
> 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:
```java
interface MyCustomMessageFactory extends MessageFactory {
}
Logger logger = LogManager.getLogger();
// Compiles without any warnings or errors
MyCustomMessageFactory f = logger.getMessageFactory();
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] rgoers commented on pull request #451: Remove unbound return type from Logger.getMessageFactory()
Posted by GitBox <gi...@apache.org>.
rgoers commented on pull request #451:
URL: https://github.com/apache/logging-log4j2/pull/451#issuecomment-751842131
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).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] Marcono1234 commented on pull request #451: Remove unbound return type from Logger.getMessageFactory()
Posted by GitBox <gi...@apache.org>.
Marcono1234 commented on pull request #451:
URL: https://github.com/apache/logging-log4j2/pull/451#issuecomment-751478321
Hm, yes you are right. Do you think this might be acceptable, or is that not worth it?
I have [commented](https://issues.apache.org/jira/browse/LOG4J2-1418?focusedCommentId=17219921&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-17219921) 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?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] Marcono1234 commented on pull request #451: Remove unbound return type from Logger.getMessageFactory()
Posted by GitBox <gi...@apache.org>.
Marcono1234 commented on pull request #451:
URL: https://github.com/apache/logging-log4j2/pull/451#issuecomment-751490879
> 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:
```java
interface MyCustomMessageFactory extends MessageFactory {
}
Logger logger = LogManager.getLogger();
// Compiles without any warnings or errors
MyCustomMessageFactory f = logger.getMessageFactory();
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] garydgregory commented on pull request #451: Remove unbound return type from Logger.getMessageFactory()
Posted by GitBox <gi...@apache.org>.
garydgregory commented on pull request #451:
URL: https://github.com/apache/logging-log4j2/pull/451#issuecomment-751484571
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()`.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org