groovy
groovy copied to clipboard
apache
fix GROOVY-9367
Could you add some tests?
I found not any standard extension descriptor file in the test resource directory. But I found a similier file under /META-INF.services.groovy. However, even if I add the test extension module into it, it not work. So I add a standard extension descriptor file (under /META-INF.services), and it works.
This needs a bit more work before we commit. Firstly, a minor thing, but we prefer the explicit imports over the * variant in nearly all cases for resilience against classpath changes. Secondly, the change to handleMatches would be a binary breaking change to the public api. We'd look at tweaking that first.
handleMatches
It seems that handleMatches is only used by doChooseMostSpecificParams, or the souce will not be compiled.
* variant imports is automatically modified by the ide.
But you'd also need to check the hundreds of libraries/frameworks which use Groovy's api classes not just our usage.
But you'd also need to check the hundreds of libraries/frameworks which use Groovy's api classes not just our usage.
handleMatches is protected, the third libraries are only able to use it by reflection
handleMatches is protected, the third libraries are only able to use it by reflection
It can be called from any class extending MetaClassImpl (which is public) without reflection.
I only now did see this PR due to the comment from shils. I can understand the goal, but not exactly the change itself. What I would have done is to change the null-branch in calculateParameterDistance and in there add the check for NullObject. Going with a distance of 0 for NullObject would make it the same as if there is Object. To actually prefere NullObject I would consider returning a distance of for Object 1 and 0 for NullObject instead. Since usually 2 is used for the distance this should be ok, but of course this pends actual checks in form of a test. If that does not work, then we have to look onto why. @danielsun1106 @paulk-asert @vipcxj what do you think?
I only now did see this PR due to the comment from shils. I can understand the goal, but not exactly the change itself. What I would have done is to change the null-branch in calculateParameterDistance and in there add the check for NullObject. Going with a distance of 0 for NullObject would make it the same as if there is Object. To actually prefere NullObject I would consider returning a distance of for Object 1 and 0 for NullObject instead. Since usually 2 is used for the distance this should be ok, but of course this pends actual checks in form of a test. If that does not work, then we have to look onto why. @danielsun1106 @paulk-asert @vipcxj what do you think?
At first, I consider returning a distance 0 for NullObject and 1 for Object, and check NullObject before the Object. but it not work. because a negative distance is still valid. Then I have to change handleMatches to do a extra check to make NullObject win. You can found it in my commit history.
@vipcxj I think you misunderstood me. I did mean a distance of 1 at the beginning of the null branch. At the end this distance is then shifted with OBJECT_SHIFT. Without the shift you get into trouble with interfaces as they are without the shift. Looking at the failure in your second commit that is at least one of the test failures you got. So at the very least your second commit would have to be changed to have no return 1 but return 1 << OBJECT_SHIFT