Generalize parsing of externalized type forms #428

mcimadamore · 2025-05-15T13:16:53Z

This draft PR contains some preliminary work to generalize parsing of externalized type forms, as well as to making Java type string in code models more readable.

Progress

Change must not contain extraneous whitespace

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 428

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/babylon/pull/428.diff

mcimadamore · 2025-05-15T13:17:57Z

src/jdk.incubator.code/share/classes/jdk/incubator/code/internal/ReflectMethods.java

@@ -2679,7 +2679,7 @@ FunctionType typeToFunctionType(Type t) {
        }

        RecordTypeRef symbolToRecordTypeRef(Symbol.ClassSymbol s) {
-            TypeElement recordType = typeToTypeElement(s.type);
+            TypeElement recordType = typeToTypeElement(s.erasure(types));


The new grammar for record references wants an erased type identifier here, so I tweaked the code accordingly.

bridgekeeper · 2025-05-15T13:18:06Z

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

openjdk · 2025-05-15T13:19:29Z

❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.

mcimadamore · 2025-05-15T13:19:40Z

src/jdk.incubator.code/share/classes/jdk/incubator/code/type/ClassType.java

        String typeArgs = hasTypeArguments() ?
                typeArguments().stream().map(JavaType::toString)
                        .collect(Collectors.joining(", ", "<", ">")) :
                "";
        return String.format("%s%s%s", prefix, name, typeArgs);
    }

+    static String escape(String s) {


This is interesting (see also ExternalizedType::toString): if the type identifier is not a valid Java identifier, we need to wrap it in quotes -- otherwise we might have parsing issues when doing a round-trip. This comes up e.g. in local class names, which start with a number.

mcimadamore · 2025-05-15T13:24:18Z

src/jdk.incubator.code/share/classes/jdk/incubator/code/type/JavaType.java

@@ -167,6 +168,11 @@ public sealed interface JavaType extends TypeElement permits ClassType, ArrayTyp
     */
    ClassDesc toNominalDescriptor();

+    @Override
+    default ExternalizedTypeElement externalize() {
+        return DescParser.parseExTypeElem(toString());


This is perhaps the main issue with this approach (see similar code in JavaRef). That is, while we can parse back textual representation that look like Java types seamlessly, the toString of the parsed ExternalizedTypeElement doesn't look like the original text.

As an example, consider the following type string:

Foo<? extends Bar>

This is parsed correctly. But if we print the toString of the (parsed) externalized type, we get:

Foo<extends<?, Bar>>

In other words, since at the externalized type element level it's all about type application, when we render the external form we no longer know what was an infix operator vs. prefix (and also whether ( or < should be used).

This means that we can't just render the toString of javaType.externalize() -- as that will not look like Java types at all. Instead, we need to render the javaType.toString() -- in other words, the toString of a Java types "happens" to be parseable back as an externalized form.

mcimadamore · 2025-05-15T13:26:05Z

src/jdk.incubator.code/share/classes/jdk/incubator/code/type/JavaTypeFactory.java

+    //        '?' 'extends' JavaType                                // covariant type argument
+    //        '?' 'super' JavaType                                  // contravariant type argument
+    //        JavaType
+    JavaType constructType(JavaType qualifier, ExternalizedTypeElement tree) {


As can be seen, the code here is quite complex. This is due to the fact that there's no general 1-1 mapping between type operators and JavaType forms. This means that when we parse an ExternalizedTypeElement back into a JavaType we have to look hard at its structure, to understand what it is, which leads to the spaghetti code in this class.

mcimadamore · 2025-05-15T13:28:25Z

src/jdk.incubator.code/share/classes/jdk/incubator/code/type/JavaTypeFactory.java

+    //        JavaType '::' ident ':' JavaType                      // field reference
+    //        JavaType '::' ident '(' JavaType* ')' ':' JavaType    // method reference
+    //        JavaType '::' '(' JavaType* ')'                       // constructor reference
+    //        ident  '(' RecordComponent* ')'                       // record reference


I had to change this slightly because the original production was:

'(' JavaType* ')' JavaType

But we cannot express that combination using the generalized grammar we have for externalized types (because the last JavaType occurs in "postfix" position).

mcimadamore · 2025-05-15T13:29:19Z

src/jdk.incubator.code/share/classes/jdk/incubator/code/parser/OpParser.java

@@ -121,8 +122,7 @@
 */
 public final class OpParser {

-    static final TypeElement.ExternalizedTypeElement VOID =
-            new TypeElement.ExternalizedTypeElement("void", List.of());
+    static final TypeElement.ExternalizedTypeElement VOID = JavaType.VOID.externalize();


Same workaround we discussed for the other attempt.

mcimadamore · 2025-05-15T13:56:26Z

src/jdk.incubator.code/share/classes/jdk/incubator/code/type/JavaTypeFactory.java

+    //        JavaType '::' JavaRefParams                           // constructor reference
+    //        ident  '(' RecordComponent* ')'                       // record reference
+    //
+    //    JavaRefParams:


There's an issue here: if there's no parameter (e.g. the method/constructor takes no args), then we have no externalized type arguments. This means that if we print that, we lose the <> and we cannot parse it back again (it will look like a field ref, not a method ref). To address this I've applied the same trick used in C -- where an empty parameter list is denoted as (void).

mcimadamore · 2025-05-15T14:01:45Z

src/jdk.incubator.code/share/classes/jdk/incubator/code/parser/impl/DescParser.java

+    //    ExSymbol:
+    //        '::'
+    //        ':'
+    //        '?'


I initially listed -> here -- but I quickly ran into an issue where parsing a func op would fail, because OpWriter also uses the -> token to separate the return type of the func op from the body (e.g. the ex type parser for the return type of the func op ends up consuming tokens that are not meant for it).

mcimadamore · 2025-05-15T14:03:07Z

src/jdk.incubator.code/share/classes/jdk/incubator/code/type/JavaTypeFactory.java

+    //        ClassType                                             // class type
+    //        PrimitiveType                                         // primitive type
+    //        TypeVar                                               // type variable
+    //        '[' JavaType ']'                                      // array type


The lack of a post-fix form is forcing us down some suboptimal path. I picked this mostly because it's succinct to read/write (although probably quite surprising).

mcimadamore · 2025-05-15T14:16:27Z

src/jdk.incubator.code/share/classes/jdk/incubator/code/type/JavaTypeFactory.java

+    //    ClassTypeNoPackage:
+    //        ident                                                 // simple class type
+    //        ident '<' TypeArg* '>'                                // parameterized class type
+    //        ClassType '::' ClassType                               // nested class type


We can't use $ here -- because $ can be the start of a Java identifier -- so if we have, say Foo$Bar the parser will read that as a single identifier.

mcimadamore added 10 commits May 6, 2025 12:12

Initial push

2d6a222

More principled handling of Java type strings

e81d656

All tests pass

c460965

Improve documentation of grammar in DescParser

cefd50b

Remove tabs from grammar doc in DescParser

141f7ef

Drop unused field in RecordTypeRefImpl

e4dc4d6

Re-enable all the test cases in TestJavaType

e963ea8

Fix ClassType::rawType() to take enclosing type into account

24f2a5b

All tests pass

dc326f7

Make JavaTypeFactory stricter

8da9710

mcimadamore commented May 15, 2025

View reviewed changes

mcimadamore added 2 commits May 15, 2025 14:47

Add more comments in DescParser

e80f183

Improve comments in JavaTypeFactory

d6d99a9

mcimadamore commented May 15, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Generalize parsing of externalized type forms #428

Generalize parsing of externalized type forms #428

Uh oh!

mcimadamore commented May 15, 2025 •

edited by openjdk bot

Loading

Uh oh!

mcimadamore May 15, 2025

Uh oh!

bridgekeeper bot commented May 15, 2025

Uh oh!

openjdk bot commented May 15, 2025

Uh oh!

mcimadamore May 15, 2025

Uh oh!

mcimadamore May 15, 2025

Uh oh!

mcimadamore May 15, 2025 •

edited

Loading

Uh oh!

mcimadamore May 15, 2025

Uh oh!

mcimadamore May 15, 2025

Uh oh!

mcimadamore May 15, 2025

Uh oh!

mcimadamore May 15, 2025

Uh oh!

mcimadamore May 15, 2025

Uh oh!

mcimadamore May 15, 2025

Uh oh!

Uh oh!

Generalize parsing of externalized type forms #428

Are you sure you want to change the base?

Generalize parsing of externalized type forms #428

Uh oh!

Conversation

mcimadamore commented May 15, 2025 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Reviewing

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bridgekeeper bot commented May 15, 2025

Uh oh!

openjdk bot commented May 15, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mcimadamore May 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mcimadamore commented May 15, 2025 •

edited by openjdk bot

Loading

mcimadamore May 15, 2025 •

edited

Loading