On Tue, 11 Feb 2025 03:25:28 GMT, Sergey Bylokhov <s...@openjdk.org> wrote:
> I'm concerning why I can't find a check in this PR which builds and tests the > change. Should I trigger it somehow, or is it just invisible for me? Oh, I acidentally switched to "Try the new merge experience" by GitHub. It doesn't show the checks in PRs. I'm able to see them again after I switched back. @mrserb > you can change the code that uses the result of `MethodInfo.get()` in > `PropertyInfo.get()` or `PropertyInfo.initialize()` where we created the > property to pick the correct default/non-default method. >... > Or, most likely, you can skip adding the default method in MethodInfo.get() > if non-default methods are already added. As I commented in JDK-8347826, I think the issue surely can be fixed in different ways. Currently, all methods are put into a list, are sorted, and then methods with the same signature are filtered out in `Introspector.addMethod()` basing on the order. The change I suggested fixes the method order to make `Introspector.addMethod()` working properly. It could be filtered out in `MethodInfo.get()`, however it requires iterating through sub-classes or/and through the entries already added, what doesn't look easy before sorting, and looks redundant as this is already done in `Introspector.addMethod()`. By introducing this kind of processing somewhere else, we duplicate `Introspector.addMethod()` logic. Futhermore, the issue cannot be addressed in `PropertyInfo` because it `Introspector.addMethod()` filters these methods out, so they are already removed from the method list at the time of `PropertyInfo.get()` call. Additionally, you're focusing on property decriptor list only. The issue occurs for both property descriptor list and method descriptor list. My proposal fixes both. > the method you updated was added to make the method list stable >From my point of view, the method list wasn't actually stable (after >JDK-8071693), because it didn't take into account if a method is default or >not, so `default` is located in the aorted list depending on a name of a >returned type, what causes a wrong method list processing in >`Introspector.addMethod()`, and exactly what I'm trying to fix here. > Also please check the next example: > ... > Is the next output expected()? > `public default void Test$A.setFoo(java.lang.Integer)` It looks OK, it was properly chosen by `PropertyInfo` logic, or am I wrong? If it's OK, I will add it to the test cases. > I also would like to discuss the next small example. > Is `public default java.lang.Object Parent3.getValue()` really supposed to be > the writer for the "value" property, that looks suspicious? > Shouldn't the correct output be: > `>> public ????? java.lang.Runnable Parent3.getValue()` > or > `>>null` As far as I see, `Parent3` actually has 2 methods falling into `com.sun.beans.introspect.MethodInfo.get()`: - `java.beans.MethodDescriptor[name=getValue; method=public abstract java.lang.Runnable Parent3.getValueX()] isSynthetic()=false` - `java.beans.MethodDescriptor[name=getValue; method=public default java.lang.Object Parent3.getValueX()] isSynthetic()=true` While processing the `Parent3` class (in case we call `Introspector.getBeanInfo(Parent3.class)`), the default synthetic method is filtered out in `java.beans.Introspector.addMethod()` as synthetic. While processing the derived `Child`, it takes the default method regardless if it's synthetic or not, as JDK-8071693 implemented. However I'm not sure if the synthetic `default java.lang.Object Parent3.getValueX()` method should exist at all in the list. I think this issue is out of scope of this PR, and doesn't relate to the proposed fix, so I'd suggest to process it separately. ------------- PR Comment: https://git.openjdk.org/jdk/pull/23443#issuecomment-2651305697
