RFR: 8347826: Introspector shows wrong method list after 8071693 [v4]

[Roman Marchenko]

On Tue, 11 Feb 2025 03:25:28 GMT, Sergey Bylokhov <serb at 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