Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add inline classes support for scalars #6352

Closed
wants to merge 11 commits into from
Closed

Conversation

BoD
Copy link
Contributor

@BoD BoD commented Jan 15, 2025

This adds 2 things:

  1. Scalar mappings can indicate the target is an inline class, and the name of the wrapped property.
    When doing so, the generated adapter calls the primitive adapter on the wrapped property to avoid boxing/unboxing.
    To do this, new versions of mapScalarToKotlinXyz() are added that take the inline class target name and property name.

  2. Ability to generate inline classes for scalars.
    Done by applying a new @inlineClass directive, e.g. extend scalar Length @inlineClass(coercion: Long)
    This will generate:

package com.example.xyz.type.scalar

@JvmInline
public value class Length(
  public val `value`: Long,
)

When doing so, the mapping is also added (no need to call mapScalarToXyz()).

Related to #6332.

@BoD BoD requested a review from martinbonnin as a code owner January 15, 2025 09:58
@svc-apollo-docs
Copy link
Collaborator

svc-apollo-docs commented Jan 15, 2025

✅ Docs preview has no changes

The preview was not built because there were no changes.

Build ID: d17423f264760e0c34caa9dd

Copy link
Contributor

@martinbonnin martinbonnin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks cool! I'm curious at the generated bytecode. Can you dump the adapters bytecode for a simple scalar/list primitive case ? Make sure there's no boxing?

Comment on lines +495 to +498
interface JavaOperationsCodegenOptions : CommonCodegenOpt, OperationsCodegenOpt, JavaCodegenOpt
interface KotlinOperationsCodegenOptions : CommonCodegenOpt, OperationsCodegenOpt, KotlinCodegenOpt
interface JavaSchemaCodegenOptions : CommonCodegenOpt, SchemaCodegenOpt, JavaCodegenOpt
interface KotlinSchemaCodegenOptions : CommonCodegenOpt, SchemaCodegenOpt, KotlinCodegenOpt
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW, this is expected:

Screenshot 2025-01-15 at 11 59 54

Comment on lines +179 to +191
internal val kotlinLabsDefinitions_0_5 = kotlinLabsDefinitions_0_4 + """
""${'"'}
Possible values for the `coercion` argument of the `@inlineClass` directive.
Since: 4.1.1
""${'"'}
enum InlineClassCoercion { String, Boolean, Int, Long, Float, Double, Any }

""${'"'}
Generate an inline class for the given scalar type. The wrapped type is determined by the `coercion` argument.
Since: 4.1.1
""${'"'}
directive @inlineClass(coercion: InlineClassCoercion!) on SCALAR
"""
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also reminder to add the matching definition on https://github.com/apollographql/specs if we agree on this

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also reminder to add support in the IDE plugin

Screenshot 2025-01-15 at 19 52 03

@BoD
Copy link
Contributor Author

BoD commented Jan 15, 2025

Generated Kotlin

public object InlineClassQuery_ResponseAdapter {
  public object Data : Adapter<InlineClassQuery.Data> {
    public val RESPONSE_NAMES: List<String> = listOf("nonNullableLength")

    override fun fromJson(reader: JsonReader, customScalarAdapters: CustomScalarAdapters): InlineClassQuery.Data {
      var _nonNullableLength: Length? = null

      while (true) {
        when (reader.selectName(RESPONSE_NAMES)) {
          0 -> _nonNullableLength = custom.scalars.type.scalar.Length(com.apollographql.apollo.api.LongAdapter.fromJson(reader, customScalarAdapters))
          else -> break
        }
      }

      return InlineClassQuery.Data(
        nonNullableLength = _nonNullableLength ?: missingField(reader, "nonNullableLength")
      )
    }

    override fun toJson(
      writer: JsonWriter,
      customScalarAdapters: CustomScalarAdapters,
      `value`: InlineClassQuery.Data,
    ) {
      writer.name("nonNullableLength")
      com.apollographql.apollo.api.LongAdapter.toJson(writer, customScalarAdapters, value.nonNullableLength.value)
    }
  }
}

Decompiled Java equivalent

  @NotNull
  public InlineClassQuery.Data fromJson(@NotNull JsonReader reader, @NotNull CustomScalarAdapters customScalarAdapters) {
     Intrinsics.checkNotNullParameter(reader, "reader");
     Intrinsics.checkNotNullParameter(customScalarAdapters, "customScalarAdapters");
     Length _nonNullableLength = null;

     while(true) {
        int var4 = reader.selectName(RESPONSE_NAMES);
        if (var4 != 0) {
           InlineClassQuery.Data var10000 = new InlineClassQuery.Data;
           if (_nonNullableLength != null) {
              var10000.<init>(_nonNullableLength.unbox-impl(), (DefaultConstructorMarker)null);
              return var10000;
           } else {
              Assertions.missingField(reader, "nonNullableLength");
              throw new KotlinNothingValueException();
           }
        }

        _nonNullableLength = Length.box-impl(Length.constructor-impl(((Number)Adapters.LongAdapter.fromJson(reader, customScalarAdapters)).longValue()));
     }
  }

  public void toJson(@NotNull JsonWriter writer, @NotNull CustomScalarAdapters customScalarAdapters, @NotNull InlineClassQuery.Data value) {
     Intrinsics.checkNotNullParameter(writer, "writer");
     Intrinsics.checkNotNullParameter(customScalarAdapters, "customScalarAdapters");
     Intrinsics.checkNotNullParameter(value, "value");
     writer.name("nonNullableLength");
     Adapters.LongAdapter.toJson(writer, customScalarAdapters, value.getNonNullableLength-7cMOgQw());
  }
  • In fromJson, _nonNullableLength is a nullable Length and boxing/unboxing happens :( I wonder if we could avoid it, while keeping the missingField behavior...

  • In toJson, value.getNonNullableLength-7cMOgQw() returns a long: no boxing 👍

@martinbonnin
Copy link
Contributor

In fromJson, _nonNullableLength is a nullable Length and boxing/unboxing happens :( I wonder if we could avoid it, while keeping the missingField behavior...

I guess we could do it by using a separate boolean. No idea if it's worth the added complexity without additional benchmarks though.

@BoD
Copy link
Contributor Author

BoD commented Jan 15, 2025

I guess we could do it by using a separate boolean.

Yeah thought of using booleans... honestly I'm not convinced it's worth it.

I think this is still useful even with this un/boxing?

@martinbonnin martinbonnin mentioned this pull request Mar 5, 2025
@martinbonnin
Copy link
Contributor

Superseded by #6404

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants