Improve textual representation of code models #423
Conversation
|
👋 Welcome back mcimadamore! A progress list of the required criteria for merging this PR into |
|
❗ This change is not yet ready to be integrated. |
| @@ -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(); | |||
There was a problem hiding this comment.
This is a workaround: the parser uses a private void type -- but such a type is not handled by either the core type factory or the Java type factory. So injecting such type into the parsed model results into issues.
| for (Method m : clazz.getDeclaredMethods()) { | ||
| check(m); | ||
| } | ||
| for (Field f : clazz.getDeclaredFields()) { | ||
| check(f); | ||
| } | ||
| for (Class<?> c : clazz.getDeclaredClasses()) { |
There was a problem hiding this comment.
Interestingly, I discovered that the harness was ignoring CodeReflection annotations in nested classes -- this is fixed now.
| @@ -61,6 +69,28 @@ static void error(String msg, Object... args) { | |||
| System.err.println("error: " + String.format(msg, args)); | |||
| } | |||
|
|
|||
| static void checkModel(Member member, String found, IR ir) { | |||
| String expected = null; | |||
There was a problem hiding this comment.
This new method allows the harness to print the model we found on the compiled class even in the face of an error when parsing the @IR annotation. This is very handy when bulk-updating the tests, as it means that parser errors will cause the harness to just dump the "expected" model for all the methods in the class (instead of throwing on the first parser error).
| JavaType javaType = JavaType.type(type); | ||
| Assert.assertEquals(type.getTypeName(), javaType.toString()); | ||
| } | ||
| // @Test(dataProvider = "types") |
There was a problem hiding this comment.
I've decided to comment this test -- since it relies on the assumption that toString of a JavaType is exactly the same as getTypeName of j.l.r.Type. While this is generally true, it doesn't quite hold for type-variables.
| // | ||
| // RecordComponent: | ||
| // JavaType ident | ||
| static JavaRef parseJavaRef(Lexer l) { |
There was a problem hiding this comment.
Note: there's a slight ambiguity here (but a harmless one, I think). When we parse a type variable that is owned by a method, we start with a (, followed by a JavaRef. This means parseJavaType will consume the (, and then call JavaRef, expecting either a method, or constructor to be parsed. Now, both production for constructor and method references starts with a JavaType (the "ref" type). But what happens if that "ref" type is also a type variable owned by some method? Again, that type starts with ( -- in other words, the type string starts with ((. But this is a problem -- because if parseJavaRef sees a ( prefix, it will think it's a record reference.
Now, this is harmless, because a method/constructor reference can never have a type-variable as its ref type.
If we wanted to completely remove the ambiguity we could:
- tweak record refs, so that we start with the record type, and then we follow with the component list
- use some other way to delimit the
JavaRefin a type-variable owner (other than()).
|
@mcimadamore this pull request can not be integrated into git checkout better_type_strings
git fetch https://git.openjdk.org/babylon.git code-reflection
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge code-reflection"
git push |
openjdk
bot
added
the
merge-conflict
This draft PR contains some preliminary work for making handling of Java type strings in code models more uniform.
Progress
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/babylon.git pull/423/head:pull/423$ git checkout pull/423Update a local copy of the PR:
$ git checkout pull/423$ git pull https://git.openjdk.org/babylon.git pull/423/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 423View PR using the GUI difftool:
$ git pr show -t 423Using diff file
Download this PR as a diff file:
https://git.openjdk.org/babylon/pull/423.diff