8071693: Introspector ignores default interface methods #13544

archiecobbs · 2023-04-19T21:29:05Z

The Introspector class was never updated to include default methods inherited from interfaces.

This patch attempts to fix that omission.

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issues

JDK-8071693: Introspector ignores default interface methods
JDK-8306477: Introspector ignores default interface methods (CSR) (Withdrawn)

Reviewers

Phil Race (@prrace - Reviewer) ⚠️ Review applies to ac90a10e
Alexey Ivanov (@aivanov-jdk - Reviewer)
Sergey Bylokhov (@mrserb - Reviewer) ⚠️ Review applies to e6a2ecba

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/13544/head:pull/13544
$ git checkout pull/13544

Update a local copy of the PR:
$ git checkout pull/13544
$ git pull https://git.openjdk.org/jdk.git pull/13544/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 13544

View PR using the GUI difftool:
$ git pr show -t 13544

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/13544.diff

Webrev

Link to Webrev Comment

bridgekeeper · 2023-04-19T21:30:44Z

👋 Welcome back archiecobbs! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2023-04-19T21:33:32Z

@archiecobbs The following label will be automatically applied to this pull request:

client

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

mlbridge · 2023-04-19T21:37:44Z

Webrevs

liach · 2023-04-19T21:40:14Z

src/java.desktop/share/classes/com/sun/beans/introspect/MethodInfo.java

 import java.util.List;

 import com.sun.beans.TypeResolver;
 import com.sun.beans.finder.MethodFinder;

 final class MethodInfo {
+
+    static final HashSet<Class<?>> IGNORABLE_INTERFACES = new HashSet<>(6);


Suggested change

static final HashSet<Class<?>> IGNORABLE_INTERFACES = new HashSet<>(6);

static final HashSet<Class<?>> IGNORABLE_INTERFACES = HashSet.newHashSet(6);

Or even better, use Set.of instead.

Thanks, much cleaner. Fixed in e37a146.

archiecobbs · 2023-04-19T21:55:55Z

This one probably needs a CSR.

/csr

openjdk · 2023-04-19T21:57:35Z

@archiecobbs has indicated that a compatibility and specification (CSR) request is needed for this pull request.

@archiecobbs please create a CSR request for issue JDK-8071693 with the correct fix version. This pull request cannot be integrated until the CSR request is approved.

turbanoff · 2023-04-20T08:47:08Z

src/java.desktop/share/classes/com/sun/beans/introspect/MethodInfo.java


 import com.sun.beans.TypeResolver;
 import com.sun.beans.finder.MethodFinder;

 final class MethodInfo {
+
+    static final Set<Class<?>> IGNORABLE_INTERFACES = Set.of(


Hm. Why only this specific interfaces?

This a list of commonly implemented interfaces that don't need to be inspected because they are "known empty". This list is inspired by Spring's ClassUtils. Happy to add any others that deserve to be in there.

given that serializable is commonly implemented, this may be a worthwhile optimisation.

I think it should have some explanation comment, about why this interfaces are ignored and how they were chosen.

Fixed in ac90a10.

aivanov-jdk · 2023-04-20T11:11:08Z

src/java.desktop/share/classes/com/sun/beans/introspect/MethodInfo.java

+                if (IGNORABLE_INTERFACES.contains(iface))
+                    continue;
+                for (Method method : iface.getMethods()) {
+                    if ((method.getModifiers() & Modifier.ABSTRACT) == 0)
+                        (list = createIfNeeded(list)).add(method);
                }


Please always use braces even if the body has only one statement.

OK, I'll fix. I've seen examples of both styles in the JDK so am never really sure.

In client libs, we tend to use the braces all the time.

By this comment, I meant adding braces to all the statements where you omitted them, including the test.

If you look through MethodInfo.java class, you see that braces are used consistently even if the body has only one statement.

Fixed in ac90a10.

…est.

prrace

The fix overall looks fine, but I'm not seeing why you think it needs a CSR.
It is just a bug fix. And if there were a CSR, it would be about the visible change, not the internals.

archiecobbs · 2023-04-20T22:21:39Z

The fix overall looks fine, but I'm not seeing why you think it needs a CSR. It is just a bug fix.

I'm not sure if it does or not. According to the CSR FAQ, Behavioral compatibility involves operational equivalence; with "the same" inputs, does a program behave "the same way" before and after a change.

I've been dinged before for mere "bug fixes" without a CSR (see for example #10856 which made the compiler behave more closely to the spec and was eventually reverted because of it :) So, just being conservative I guess.

I'd be happy to be wrong. @jddarcy, your opinion?

jddarcy · 2023-04-21T05:24:30Z

The fix overall looks fine, but I'm not seeing why you think it needs a CSR. It is just a bug fix.

I'm not sure if it does or not. According to the CSR FAQ, Behavioral compatibility involves operational equivalence; with "the same" inputs, does a program behave "the same way" before and after a change.

I've been dinged before for mere "bug fixes" without a CSR (see for example #10856 which made the compiler behave more closely to the spec and was eventually reverted because of it :) So, just being conservative I guess.

I'd be happy to be wrong. @jddarcy, your opinion?

Hmmm. I'm not an expert on Introspectors, but my impression is the behavior change is CSR-worthy. HTH

prrace · 2023-04-21T17:02:19Z

The fix overall looks fine, but I'm not seeing why you think it needs a CSR. It is just a bug fix.

I'm not sure if it does or not. According to the CSR FAQ, Behavioral compatibility involves operational equivalence; with "the same" inputs, does a program behave "the same way" before and after a change.

By that criterion every change needs a CSR.
Gosh, it used to SEGV, now it doesn't - I'd better file a CSR !

I've been dinged before for mere "bug fixes" without a CSR (see for example #10856 which made the compiler behave more closely to the spec and was eventually reverted because of it :) So, just being conservative I guess.
I'd be happy to be wrong. @jddarcy, your opinion?

Hmmm. I'm not an expert on Introspectors, but my impression is the behavior change is CSR-worthy. HTH

Maybe but regardless the CSR that has been written is very wrong, as I already commented there.
If you could write something up that sticks to specification or describing behaviour that rises to the level of implied
specification, using words, not code, then we can consider it.

All it needs to say is
"Previously, the introspector would ignore interfaces, since before default methods were added to the language, there
could be no implementation. Now interfaces are considered like other classes"

That's it, isn't it ?

aivanov-jdk · 2023-04-21T18:58:00Z

test/jdk/java/beans/Introspector/DefaultMethodBeanPropertyTest.java

Is it expected that the test file doesn't compile with simple javac DefaultMethodBeanPropertyTest.java? It complains about BeanUtils not found. When run with jtreg, the test compiles successfully.

Is it expected that the test file doesn't compile with simple javac DefaultMethodBeanPropertyTest.java?

Definitely not. For example, many regression tests use classes that are not part of the standard JDK such as toolbox.ToolBox.

archiecobbs · 2023-04-21T20:21:03Z

Let's make this CSR question easy. I'll remove the CSR and see if anybody actually complains :)

/csr unneeded

openjdk · 2023-04-21T20:21:24Z

@archiecobbs only Reviewers can determine that a CSR is not needed.

archiecobbs · 2023-04-21T20:35:23Z

Let's make this CSR question easy. I'll remove the CSR and see if anybody actually complains :)

Ugh, it looks like the bot won't let me do that. A reviewer will have to remove the CSR....

prrace · 2023-04-21T22:03:35Z

/csr unneeded

mrserb · 2023-04-24T18:14:05Z

test/jdk/java/beans/Introspector/DefaultMethodBeanPropertyTest.java

+import java.util.Arrays;
+import java.util.HashSet;
+
+public class DefaultMethodBeanPropertyTest {


Can we please add two additional tests to verify the "diamond" case:

getFoo is in the top interfaceA, two empty subinterfaces B anc C , and one class D of B and C, will the D have one correct prop Foo?

getFoo is in the top interfaceA, two non-empty subinterfaces B and C and each override getFoo by the different return types, and then one class D of B and C which override getFoo again by compatible type from B and C, will the D have one correct prop Foo?

We also can test the case if the D from the cases above is interface and implemented by the class E.

Sure thing... see 6b43627.

mrserb · 2023-04-24T18:15:24Z

test/jdk/java/beans/Introspector/DefaultMethodBeanPropertyTest.java

+        findProperty(ClassB.class, "foo");
+
+        // Expected properties
+        final HashSet<PropertyDescriptor> expected = new HashSet<>();


please split the long lines to use 80 chars per line.

Fixed in e6a2ecb.

mrserb

Looks fine.

archiecobbs · 2023-04-24T21:24:42Z

/integrate

openjdk · 2023-04-24T21:27:02Z

@archiecobbs
Your change (at version e6a2ecb) is now ready to be sponsored by a Committer.

test/jdk/java/beans/Introspector/DefaultMethodBeanPropertyTest.java

archiecobbs · 2023-04-25T18:52:15Z

/integrate

openjdk · 2023-04-25T18:54:00Z

@archiecobbs
Your change (at version b92726a) is now ready to be sponsored by a Committer.

mrserb · 2023-04-26T22:43:20Z

/sponsor

openjdk · 2023-04-26T22:45:22Z

Going to push as commit 1e4eafb.
Since your change was applied there have been 40 commits pushed to the master branch:

750bece: 8305771: SA ClassWriter.java fails to skip overpass methods
b81c9c8: 8306951: [BACKOUT] JDK-8305252 make_method_handle_intrinsic may call java code under a lock
732179c: 8306409: Open source AWT KeyBoardFocusManger, LightWeightComponent related tests
38cc039: 8306705: com/sun/jdi/PopAndInvokeTest.java fails with NativeMethodException
01b8512: 8302182: Update Public Suffix List to 88467c9
8e36c05: 8305853: java/text/Format/DateFormat/DateFormatRegression.java fails with "Uncaught exception thrown in test method Test4089106"
d0e8aec: 8306374: (bf) Improve performance of DirectCharBuffer::append(CharSequence[,int,int])
a18191f: 8302328: [s390x] Simplify asm_assert definition
9bc6a21: 8306033: Resolve multiple definition of 'throwIOException' and friends when statically linking with JDK native libraries
35e8023: 8306872: Rename Node_Array::Size()
... and 30 more: https://git.openjdk.org/jdk/compare/bad6aa68e4d491e819ab22e91dd5d65bb094120e...master

Your commit was automatically rebased without conflicts.

openjdk · 2023-04-26T22:45:38Z

@mrserb @archiecobbs Pushed as commit 1e4eafb.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

jddarcy · 2023-04-26T23:02:40Z

As a retroactive post-integration comment, there is judgement required when gauging whether or not the behavioral changes associated with a PR merit a CSR, as discussed in the CSR FAQ:

"Q: Under what conditions does a CSR need to be filed for a purely behavioral change?
A: Using qualitative terms, a CSR for a behavioral change should be filed if it is estimated enough developers or users would be sufficiently impacted by the change that it should get additional consideration or documentation. A judgment call is involved. If assistance is needed in determining whether or not a CSR needs to be filed, ask the CSR representative for that area or the CSR chair."
https://wiki.openjdk.org/display/csr/CSR+FAQs

Given my general understanding of the nature of the change, based on nature of the change, my answer to the question of whether or not this should have a CSR would be "yes", but I'm happy to defer to @prrace 's subject matter expertise here that the answer should be "no."

archiecobbs · 2023-04-26T23:04:49Z

Maybe we should add a release note as a sort of consolation prize...?

exabrial · 2023-05-09T20:01:34Z

Hello, If I backported this to JDK17 LTS, would there be any chance of acceptance?

Open Source work often goes unthanked, so I do wish to express my gratitude for fixing this. Unfortunately it does create headaches in JDK17 as an entire class of methods are getting missed.

Include default methods inherited from interfaces in bean introspection.

b343ba1

openjdk bot added the rfr Pull request is ready for review label Apr 19, 2023

openjdk bot added the client client-libs-dev@openjdk.org label Apr 19, 2023

liach reviewed Apr 19, 2023

View reviewed changes

archiecobbs changed the title ~~8182025: PropertyDescriptor ignores default methods from interfaces implemented by superclasses~~ 8071693: Introspector ignores default interface methods Apr 19, 2023

openjdk bot added the csr Pull request needs approved CSR before integration label Apr 19, 2023

Use Set.of() to initialize IGNORABLE_INTERFACES set.

e37a146

turbanoff reviewed Apr 20, 2023

View reviewed changes

aivanov-jdk reviewed Apr 20, 2023

View reviewed changes

archiecobbs added 4 commits April 20, 2023 08:38

Put braces around single-line "if" statement clauses.

0326aa1

Use Modifier.isAbstract() convenience method instead of bit masking t…

1d2ae2c

…est.

Verify static method is not mistakenly identified as property getter.

106edcd

Merge branch 'master' into JDK-8071693

60301d4

prrace reviewed Apr 20, 2023

View reviewed changes

aivanov-jdk reviewed Apr 21, 2023

View reviewed changes

archiecobbs added 2 commits April 21, 2023 15:15

Put braces around single-line "if" statement clauses in test.

1ecc839

Add comment describing what IGNORABLE_INTERFACES is for.

ac90a10

openjdk bot removed the csr Pull request needs approved CSR before integration label Apr 21, 2023

Add braces around if clause.

8c12996

openjdk bot removed the sponsor Pull request is ready to be sponsored label Apr 24, 2023

mrserb reviewed Apr 24, 2023

View reviewed changes

aivanov-jdk approved these changes Apr 24, 2023

View reviewed changes

archiecobbs added 2 commits April 24, 2023 14:17

Add more scenarios to the regression test.

6b43627

Jam lines into 80 columns.

e6a2ecb

mrserb approved these changes Apr 24, 2023

View reviewed changes

openjdk bot added the sponsor Pull request is ready to be sponsored label Apr 24, 2023

aivanov-jdk approved these changes Apr 25, 2023

View reviewed changes

archiecobbs added 2 commits April 25, 2023 13:39

Merge branch 'master' into JDK-8071693

823c4f1

Whitespace cleanups suggested by review.

b92726a

openjdk bot removed the sponsor Pull request is ready to be sponsored label Apr 25, 2023

aivanov-jdk approved these changes Apr 25, 2023

View reviewed changes

openjdk bot added the sponsor Pull request is ready to be sponsored label Apr 25, 2023

openjdk bot added the integrated Pull request has been integrated label Apr 26, 2023

openjdk bot closed this Apr 26, 2023

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review sponsor Pull request is ready to be sponsored labels Apr 26, 2023

	static final HashSet<Class<?>> IGNORABLE_INTERFACES = new HashSet<>(6);
	static final HashSet<Class<?>> IGNORABLE_INTERFACES = HashSet.newHashSet(6);

8071693: Introspector ignores default interface methods #13544

8071693: Introspector ignores default interface methods #13544

Conversation

archiecobbs commented Apr 19, 2023 • edited by openjdk bot Loading

Progress

Issues

Reviewers

Reviewing

Webrev

bridgekeeper bot commented Apr 19, 2023

openjdk bot commented Apr 19, 2023

mlbridge bot commented Apr 19, 2023 • edited Loading

Webrevs

Choose a reason for hiding this comment

Choose a reason for hiding this comment

archiecobbs commented Apr 19, 2023

openjdk bot commented Apr 19, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prrace left a comment

Choose a reason for hiding this comment

archiecobbs commented Apr 20, 2023

jddarcy commented Apr 21, 2023

prrace commented Apr 21, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

archiecobbs commented Apr 21, 2023

openjdk bot commented Apr 21, 2023

archiecobbs commented Apr 21, 2023

prrace commented Apr 21, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrserb left a comment

Choose a reason for hiding this comment

archiecobbs commented Apr 24, 2023

openjdk bot commented Apr 24, 2023

archiecobbs commented Apr 25, 2023

openjdk bot commented Apr 25, 2023

mrserb commented Apr 26, 2023

openjdk bot commented Apr 26, 2023

openjdk bot commented Apr 26, 2023

jddarcy commented Apr 26, 2023

archiecobbs commented Apr 26, 2023

exabrial commented May 9, 2023 • edited by bridgekeeper bot Loading

archiecobbs commented Apr 19, 2023 •

edited by openjdk bot

Loading

mlbridge bot commented Apr 19, 2023 •

edited

Loading

exabrial commented May 9, 2023 •

edited by bridgekeeper bot

Loading