#3054: Allow abstract return type when all directly sealed subtypes are covered by subclass mappings. #3056
Conversation
…ubtypes are covered by subclass mappings.
| permitted -> ctx | ||
| .getTypeUtils() | ||
| .isSameType( | ||
| permitted, | ||
| subClassOption.getSource() ) ) |
There was a problem hiding this comment.
I haven't played around with this, but would changing this check to be either the "same type" check or a recursive "isCorrectlySealed" check be enough to fully cover a sealed subtype hierarchy, rather than just the case where it's only one-level deep and only the direct subtypes are covered?
There was a problem hiding this comment.
Indeed I missed the hierarchy bit here.
This should be covered in the next commit where I've added a Motor that is extended by a Harley and Davidson using the sealed structure for the test. And in code it now checks recursively, but only if the sealed subtype is abstract.
In order to achieve this we are going to need to use java 17 for our processor test suite. We used to have something like this back when we were supporting java 7. The pattern would be to use btw @Zegveld, I've been really busy with other things and haven't had the time to properly review things. I hope I can catch up soon. |
|
@filiphr, no worries. I've been busy myself (and still am atm) so it will take a while before I have more time then a few minutes to look at things here myself. |
| Type mappingSourceType = method.getMappingSourceType(); | ||
| return isCorrectlySealed( mappingSourceType ); | ||
| } | ||
| catch ( Exception e ) { |
There was a problem hiding this comment.
What kind of an exception can happen here?
There was a problem hiding this comment.
good question, reading the code there should be no chance of an exception happening here when not java 17.
There was a problem hiding this comment.
In that case let's remove the try catch. We should handle it properly if something throws an exception
| method.getOptions().getSubclassMappings().forEach( subClassOption -> { | ||
| unusedPermittedSubclasses | ||
| .stream() | ||
| .filter( | ||
| permitted -> ctx | ||
| .getTypeUtils() | ||
| .isSameType( | ||
| permitted, | ||
| subClassOption.getSource() ) ) | ||
| .findFirst() | ||
| .ifPresent( unusedPermittedSubclasses::remove ); | ||
| } ); |
There was a problem hiding this comment.
I think that this would be more readable if we write in without streams. Perhaps use an iteratorfor removing from theunusedPermittedSubclasses`
|
I've made the wanted changes, and they were definitely an improvement.
|
We don't need to do it now. Perhaps we can create an issue for when we change our LTS version. I am not sure how easy / difficult is to have the test suite with a different java version in Maven |
3 participants
@filiphr do you know how to add a failure test for java 17+ situations?
Currently I've only added the happy flow situation to the integration test.
Fixes #3054