Support record inside JandexUtil

[loicmathieu]

Improve Java records support.

The only place where I tested it is with MongoDB with Panache, thanks to this PR MongoDB entity can now be declared as records. Other places should require more work.

There is no test as there is no facility in Quarkus yet to test functionalities provided by java version different than the one we build with.

[Ladicek]

This change LGTM, but I've got 2 notes:

1. The isSubclassOf method didn't work correctly when parentName was java.lang.Object. Now, it won't work correclty if parentName is also java.lang.Record. I guess that's not too big of an issue by now?
2. RESTEasy Reactive carries a copy of JandexUtil that has already drifted a little. Maybe we should do this change there as well? CC @geoand

I'm rescinding my approval here, because I don't really understand how Quarkus uses Jandex internally. I also think that using the "computing" index (which indexes classes on demand) is probably a better choice.

[geoand]

Maybe we should do this change there as well

Sounds like a good idea to me

[loicmathieu]

@Ladicek I added your commit to the branch so we have now an almost fully functionning JandexUtil for records.

[Ladicek]

I would agree with the original form of this PR, as an optimization. (Also done in the RESTEasy Reactive copy of JandexUtil.) I don't agree with exposing an isClassTerminal operation, because it introduces but not properly defines a new concept, one that I doubt is useful.

[loicmathieu]

Ok I'll change it tomorrow. I did it thus way because there is a bunch of places in Panache code where the same if is made.

[Ladicek]

Can you please try using the computed index and see if those ifs can disappear from Panache code?

[loicmathieu]

@Ladicek using the computing index didn't change anything, I can do it if you prefere (I prepare a PR for some changes that must be done at MongoDB with Panache side but I need to do more test).

I propose to revert to my previous changes without the new method and also impact the resteasy reactive JandexUtil, by the way, some methods are not used (at least it's what IntelliJ said), can I remove them?

[Ladicek]

I think I'd prefer to keep the 2 JandexUtil classes as much in sync as possible. There's a drift already, but let's not make it bigger.

[Ladicek]

BTW if there's a need to special-case for records, you can always check ClassInfo.isRecord(). But the computing index should provide a ClassInfo for java.lang.Record and hence no special-casing should be necessary, at least not the kind we do in this PR. If not, I suspect my lack of understanding is bigger than I thought :-)

[loicmathieu]

Sooooo, @Ladicek your undertanding of records is OK, I was hit by a testing issue by the CLI not recompiling ...

Yes, using the computing index inside the processor fix the issue. Should I dismiss this PR ?
Can you quickly explain to me what is the difference between the two index ? All Panache code using the index not the computing one since 3 years now so I prefer understanding the impact of changing it.

[Ladicek]

You can see the difference here: https://github.com/quarkusio/quarkus/blob/main/core/deployment/src/main/java/io/quarkus/deployment/steps/CombinedIndexBuildStep.java In short, the computing index is a composite of the original index (that you currently use) and a mutable index that produces ClassInfo objects on-demand, if they were not indexed yet. This is used on multiple places for this exact case -- classes from the JDK referred to by previously indexed classes (because the original index contains no JDK classes).

[Ladicek]

And we don't have to dismiss the PR. Your commit works as an optimization, and my commit actually fixes a method that previously wasn't aware of records.

[loicmathieu]

OK, great.

I'll prepare a PR that switch to the computing index for Panache and make some test (this will be the hard part). I wonder if I should create an integration test running Java 17 with some records in it. I'll open a discussion in Zulip about this.

[quarkus-bot]

Failing Jobs - Building 2453904

Status	Name	Step	Failures	Logs	Raw logs
✔️	JVM Tests - JDK 11
✔️	JVM Tests - JDK 17
✖	JVM Tests - JDK 17 MacOS M1	Set up runner	⚠️ Check →	Logs	Raw logs
✔️	JVM Tests - JDK 18

[loicmathieu]

@holly-cummins this PR fails on the MacOS build very ealry, you may want to have a look.

As CI failres seems unrelated, I merge this one.