RFR: 7903788: preparation towards json configuration for jextract tool [v4]

[Athijegannathan Sundararajan]

On Fri, 16 Aug 2024 13:13:49 GMT, Maurizio Cimadamore <mcimadamore at openjdk.org> wrote:

>> Athijegannathan Sundararajan has updated the pull request incrementally with two additional commits since the last revision:
>> 
>>  - removed WildcardType (review suggestion)
>>  - renamed JSONObjects.getRecordProperty to be getRecordComponentValue. (review suggestion)
>
> src/main/java/org/openjdk/jextract/json/JSONObjects.java line 362:
> 
>> 360:         Type[] typeArgs = pt.getActualTypeArguments();
>> 361:         Type nthType = typeArgs.length > n ? typeArgs[n] : Object.class;
>> 362:         if (nthType instanceof WildcardType wt) {
> 
> This logic is not correct. E.g. if something is `List<? super Integer>` it is not correct to assume that the type argument is `String` (and e.g. create a `Integer[]` as a result). Because `List<? extends Number>` can have `Number` elements which won't go in an `Integer[]`.
> 
> In general, I think it might be appropriate to dial down the level of "smartness" and try to provide something that is well-behaved, and good enough for what we need. We don't need to solve the general problem of mapping json to Java, we just need a mapping that is good enough for jextract to use.
> 
> So... I think we'd be ok if wildcards won't be accepted by the mapper.

Okay. I'll remove Wildcard types.

> src/main/java/org/openjdk/jextract/json/JSONObjects.java line 392:
> 
>> 390: 
>> 391:     // get the Record component value as a JSONValue
>> 392:     private static JSONValue getRecordProperty(Record record, RecordComponent rc) {
> 
> getRecordComponentValue seems a better name here?

Will rename.

> src/main/java/org/openjdk/jextract/json/JSONObjects.java line 418:
> 
>> 416:     }
>> 417: 
>> 418:     private static Map<?,?> convertToMap(JSONObject jo, Type keyType, Type valueType) {
> 
> This seems overly general - isn't a JSON object always a map from String to something else?

Keys can be anything convertible from String (like enum for eg).  Yes, there are not that many, but.. 

record MyRecord(Map<SymbolType, <valuetype>> ....)

> src/main/java/org/openjdk/jextract/json/JSONObjects.java line 638:
> 
>> 636:         addNumberConverter(long.class, JSONNumber::asLong);
>> 637:         addNumberConverter(Long.class, JSONNumber::asLong);
>> 638:         addNumberConverter(OptionalInt.class, jn -> OptionalInt.of(jn.asInt()));
> 
> These last four seem odd

You mean the OptionalInt part? Thats to have property that can be dropped. long vs int targets are supported (JSONNumber is always long - is converted to smaller ints by bounds check)

> src/main/java/org/openjdk/jextract/json/JSONObjects.java line 677:
> 
>> 675:         addDecimalConverter(float.class, JSONDecimal::asFloat);
>> 676:         addDecimalConverter(Float.class, JSONDecimal::asFloat);
>> 677:         addDecimalConverter(OptionalDouble.class, jd -> OptionalDouble.of(jd.asDouble()));
> 
> This seems a bit odd

You mean the Optional part? Thats to have property that can be dropped. float exists only for completion (of all primitive types).

> src/main/java/org/openjdk/jextract/json/JSONObjects.java line 678:
> 
>> 676:         addDecimalConverter(Float.class, JSONDecimal::asFloat);
>> 677:         addDecimalConverter(OptionalDouble.class, jd -> OptionalDouble.of(jd.asDouble()));
>> 678:         addDecimalConverter(BigDecimal.class, jd -> BigDecimal.valueOf(jd.asDouble()));
> 
> Not sure we'll ever use this one TBH?

Removed BigDecimal. float exists only for completeness of 'all primitive' types.

> src/main/java/org/openjdk/jextract/json/parser/JSONNumber.java line 83:
> 
>> 81:     @Override
>> 82:     public boolean equals(Object o) {
>> 83:         if (this == o) {
> 
> tests like these can be written more succinctly as:
> 
> return this == o ||
>            (o instanceof JSONNumber n) && value == n.value

As I said, I didn't touch skara code much except for cosmetic changes. Skara's json mostly uses JDK 8 level code.

> test/testng/org/openjdk/jextract/test/json/parser/JSONParserTests.java line 205:
> 
>> 203:     @Test
>> 204:     void testLargerJSONText() {
>> 205:         var text = "{\n" +
> 
> Would it make sense, for this or other big chunks of JSON, to do a roundtrip test where we parse the text, we get a JSONValue, then we obtain the string representation from it, and we compare it against the text?

This test comes from skara repo. Other than package naming nothing is changed. Should I add a round trip test there itself or in another new test?

-------------

PR Review Comment: https://git.openjdk.org/jextract/pull/257#discussion_r1719868150
PR Review Comment: https://git.openjdk.org/jextract/pull/257#discussion_r1719865737
PR Review Comment: https://git.openjdk.org/jextract/pull/257#discussion_r1719887481
PR Review Comment: https://git.openjdk.org/jextract/pull/257#discussion_r1719886068
PR Review Comment: https://git.openjdk.org/jextract/pull/257#discussion_r1719884250
PR Review Comment: https://git.openjdk.org/jextract/pull/257#discussion_r1719876410
PR Review Comment: https://git.openjdk.org/jextract/pull/257#discussion_r1719875702
PR Review Comment: https://git.openjdk.org/jextract/pull/257#discussion_r1719890981