-
-
Notifications
You must be signed in to change notification settings - Fork 1k
#2677 Use type without bounds when looking for read / presence accessor in a SourceReference #2737
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
#2677 Use type without bounds when looking for read / presence accessor in a SourceReference #2737
Conversation
…ce accessor in a SourceReference
| for ( int i = 0; i < entryNames.length; i++ ) { | ||
| boolean matchFound = false; | ||
| ReadAccessor readAccessor = newType.getReadAccessor( entryNames[i] ); | ||
| Type noBoundsType = newType.withoutBounds(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm trying to grasp what is happening here. So I did setup a breakpoint on when the presenceChecker of the noBoundsType differs from the newType presenceChecker. It's not hit. I'm doing something wrong here. Can you explain why there's a diff?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't add a test case for the presence checker, but the test case fails if we don't use withoutBounds for getting the ReadAccessor
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think that we do this at the right place. You are now testing with a upper bound. Did you try using a lower bound?
So in your test case:
@Mapping(target = "id", source = "value.id")
Output map(Wrapper<? super Parent> in);
@Mapping(target = ".", source = "value")
Output mapImplicitly(Wrapper<? super Parent> in);You probably need to change your inheritance structure for that.
The point is: you change it at the spot where we define the SourceReference. We should probably check it at the spot where we try to make a match to the getter respectively the presence checker.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think that we do this at the right place. You are now testing with a upper bound. Did you try using a lower bound?
We do not allow using lower bounds for source parameters. At least not when there is a single parameter. Which means that for the example above there would be a compile error.
I'll try to come up with some more tests where we do implicit without a target = "."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added some more tests, not sure if it is what you were looking for. They still work without my changes. There can be other corner cases like auto generating from things like Wrapper<? extends SomeType>, but I don't think that this belong to this issue
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok.. then go ahead and merge. I was indeed looking for cases that failed.
And I was thinking how this code ties in to the method selection mechanism (I did not investigate that). But it looked like you were making a kind of pre-selection / pre-filter (so limiting the cases that you "feed" to the method selection prior to calling method selection later on). If that's the case: then this should be fixed there (in method selection) and not in source reference. Hope this clarifies my comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes it does make sense. However, the problem was not about method selection, the problem was that the we were not getting any properties for the wild card type.
Fixes #2677