Java Codegen: better disambiguation between fields and packages (#18418)

We can currently have a problem where we can generate code that the java compiler chokes on.

This occurs when we have a generated class with a field, say `public final Integer foo;`, and it needs to call the `jsonDecoder` static method of a class under a package which begins with "foo", e.g. `foo.bar.Baz.jsonDecoder()`

Java sees the `foo.bar` and interprets that as an attempt to access the `bar` field on the local `foo` variable, and fails with `symbol not found`, not understanding that `foo.bar` is simply the package qualifier of the `Bar` class. When hand-writing code, we can avoid this by importing `foo.bar.Baz`, and then calling the method with `Baz.jsonDecoder()`, but when generating code, the JavaPoet library seems to opt for using fully qualified references to these static methods, which is when we can hit the problematic expressions.

This workaround generates a class nested within `Baz` a la

    public static class JsonDecoder$ { public JsonLfDecoder<Baz> get() { return jsonDecoder(); } }

which simply forwards the call on. However it means that we can avoid the ambiguious parsing context at the call side by invoking the method via `new foo.bar.Baz.JsonDecoder$().get()`. Using the `new` keyword clarifies that the next thing is a type, and javac correctly resolves `foo.bar` as a package name.

I benchmarked the changes with `bazel run language-support/java/codegen:from-json-bench` and there was no appreciable impact on performance.

We had previously avoided this issue by using `.simpleName` on the class used for `jsonDecoder()` to explicitly drop the package name, but that won't work once we start supporting decoding of choice arguments and results, as the relevant types typically won't have already been imported.

This workaround would be unnecessary if JavaPoet would reliably import all referenced types, and then refer to the class name unqualified. It would also be unnecessary if javac were a bit smarter about its parsing. However as of this PR, that does not seem to be the behaviour.
This commit is contained in:
Raphael Speyer 2024-02-14 14:40:39 +11:00 committed by GitHub
parent 1459f084c9
commit b9fe360ae6
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
5 changed files with 47 additions and 8 deletions

View File

@ -37,6 +37,7 @@ private[inner] object EnumClass extends StrictLogging {
.addMethod(generateToValue)
.addMethods(FromJsonGenerator.forEnum(className, "__enums$").asJava)
.addMethods(ToJsonGenerator.forEnum(className).asJava)
.addType(FromJsonGenerator.decoderAccessorClass(className, Vector()))
logger.debug("End")
enumType.build()
}

View File

@ -15,12 +15,14 @@ import com.squareup.javapoet.{
ParameterSpec,
ParameterizedTypeName,
TypeName,
TypeSpec,
TypeVariableName,
}
import scala.jdk.CollectionConverters._
private[inner] object FromJsonGenerator extends StrictLogging {
private def decodeClass = ClassName.get(classOf[JsonLfDecoders])
private val decodeClass = ClassName.get(classOf[JsonLfDecoders])
private val decoderAccessorClassName = "JsonDecoder$"
// JsonLfDecoder<T>
private def decoderTypeName(t: TypeName) =
@ -38,6 +40,10 @@ private[inner] object FromJsonGenerator extends StrictLogging {
.build()
}.asJava
private def decodeTypeParamArgList(typeParams: IndexedSeq[String]): CodeBlock =
CodeBlock
.join(typeParams.map(t => CodeBlock.of("$L", decodeTypeParamName(t))).asJava, ", ")
def forRecordLike(fields: Fields, className: ClassName, typeParams: IndexedSeq[String])(implicit
packagePrefixes: PackagePrefixes
): Seq[MethodSpec] = {
@ -124,8 +130,7 @@ private[inner] object FromJsonGenerator extends StrictLogging {
.addException(classOf[JsonLfDecoder.Error])
.addStatement(
"return jsonDecoder($L).decode(new $T(json))",
CodeBlock
.join(typeParams.map(t => CodeBlock.of("$L", decodeTypeParamName(t))).asJava, ", "),
decodeTypeParamArgList(typeParams),
classOf[JsonLfReader],
)
.build()
@ -152,10 +157,7 @@ private[inner] object FromJsonGenerator extends StrictLogging {
"case $S: return $L($L)",
f.damlName,
decoderForTagName(f.damlName),
CodeBlock.join(
typeParams.map(t => CodeBlock.of("$L", decodeTypeParamName(t))).asJava,
", ",
),
decodeTypeParamArgList(typeParams),
)
}
block
@ -242,6 +244,38 @@ private[inner] object FromJsonGenerator extends StrictLogging {
Seq(jsonDecoder, fromJsonString(className, IndexedSeq.empty[String]))
}
// Generates a class a la
//
// public class JsonDecoder$ { public JsonLfDecoder<Baz> get() { return jsonDecoder(); } }
//
// This is a workaround to avoid specific cases where the java compliler gets confused.
// See https://github.com/digital-asset/daml/pull/18418/files for more details.
def decoderAccessorClass(
className: ClassName,
typeParams: IndexedSeq[String],
) = {
val typeVars = typeParams.map(TypeVariableName.get)
val typeName =
if (typeParams.isEmpty) className else ParameterizedTypeName.get(className, typeVars: _*)
TypeSpec
.classBuilder(decoderAccessorClassName)
.addModifiers(Modifier.PUBLIC, Modifier.STATIC)
.addJavadoc(
"Proxies the jsonDecoder(...) static method, to provide an alternative calling synatx, which avoids some cases in generated code where javac gets confused"
)
.addMethod(
MethodSpec
.methodBuilder("get")
.addModifiers(Modifier.PUBLIC)
.addTypeVariables(typeVars.asJava)
.returns(decoderTypeName(typeName))
.addParameters(jsonDecoderParamsForTypeParams(typeParams))
.addStatement("return jsonDecoder($L)", decodeTypeParamArgList(typeParams))
.build()
)
.build()
}
private def jsonDecoderForType(
damlType: Type
)(implicit packagePrefixes: PackagePrefixes): CodeBlock = {
@ -254,7 +288,8 @@ private[inner] object FromJsonGenerator extends StrictLogging {
damlType match {
case TypeCon(TypeConName(ident), typeParams) =>
CodeBlock.of("$L.jsonDecoder($L)", guessClass(ident).simpleName, typeReaders(typeParams))
val decoderAccessorClass = guessClass(ident).nestedClass(decoderAccessorClassName)
CodeBlock.of("new $T().get($L)", decoderAccessorClass, typeReaders(typeParams))
case TypePrim(PrimTypeBool, _) => CodeBlock.of("$T.bool", decodeClass)
case TypePrim(PrimTypeInt64, _) => CodeBlock.of("$T.int64", decodeClass)
case TypeNumeric(scale) => CodeBlock.of("$T.numeric($L)", decodeClass, scale)

View File

@ -41,6 +41,7 @@ private[inner] object RecordClass extends StrictLogging {
.addFields(RecordFields(fields).asJava)
.addField(createPackageIdField(packageId))
.addMethods(recordMethods.asJava)
.addType(FromJsonGenerator.decoderAccessorClass(className, typeParameters))
.build()
logger.debug("End")

View File

@ -99,6 +99,7 @@ private[inner] object TemplateClass extends StrictLogging {
.addMethod(companion.generateGetter())
.addFields(RecordFields(fields).asJava)
.addMethods(templateMethods.asJava)
.addType(FromJsonGenerator.decoderAccessorClass(className, Vector()))
generateByKeyMethod(className, template.key) foreach { byKeyMethod =>
templateType
.addMethod(byKeyMethod)

View File

@ -63,6 +63,7 @@ private[inner] object VariantClass extends StrictLogging {
).asJava
)
.addField(createPackageIdField(typeWithContext.interface.packageId))
.addType(FromJsonGenerator.decoderAccessorClass(variantClassName, typeArguments))
.build()
val (constructors, constructorStaticImports) = generateConstructorClasses(
typeArguments,