Skip to content

Commit

Permalink
Fix the logic for computing the number of int default-value bitmask…
Browse files Browse the repository at this point in the history
…s in a Kotlin constructor.

There are ⌈_n_/32⌉ `int` bitmasks when the total number of parameters is _n_. Previously the logic was based on _n_ being the number of _optional_ parameters (the ones with default values).

RELNOTES=A bug with AutoBuilder and Kotlin data classes has been fixed. If there was a mix of required and optional parameters in a data class with a large number of properties, sometimes the generated code would not compile.
PiperOrigin-RevId: 651226198
  • Loading branch information
eamonnmcmanus authored and Google Java Core Libraries committed Jul 11, 2024
1 parent bc7278e commit ad284f6
Show file tree
Hide file tree
Showing 5 changed files with 140 additions and 9 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -251,6 +251,91 @@ public void kotlinSomeDefaults_missingRequired() {
assertThat(e).hasMessageThat().contains("requiredString");
}

@AutoBuilder(ofClass = KotlinDataSomeDefaultsBig.class)
interface KotlinDataSomeDefaultsBigBuilder {
static KotlinDataSomeDefaultsBigBuilder builder() {
return new AutoBuilder_AutoBuilderKotlinTest_KotlinDataSomeDefaultsBigBuilder();
}

KotlinDataSomeDefaultsBigBuilder requiredInt(int x);

KotlinDataSomeDefaultsBigBuilder requiredString(String x);

KotlinDataSomeDefaultsBigBuilder a1(int x);

KotlinDataSomeDefaultsBigBuilder a2(int x);

KotlinDataSomeDefaultsBigBuilder a3(int x);

KotlinDataSomeDefaultsBigBuilder a4(int x);

KotlinDataSomeDefaultsBigBuilder a5(int x);

KotlinDataSomeDefaultsBigBuilder a6(int x);

KotlinDataSomeDefaultsBigBuilder a7(int x);

KotlinDataSomeDefaultsBigBuilder a8(int x);

KotlinDataSomeDefaultsBigBuilder a9(int x);

KotlinDataSomeDefaultsBigBuilder a10(int x);

KotlinDataSomeDefaultsBigBuilder a11(int x);

KotlinDataSomeDefaultsBigBuilder a12(int x);

KotlinDataSomeDefaultsBigBuilder a13(int x);

KotlinDataSomeDefaultsBigBuilder a14(int x);

KotlinDataSomeDefaultsBigBuilder a15(int x);

KotlinDataSomeDefaultsBigBuilder a16(int x);

KotlinDataSomeDefaultsBigBuilder a17(int x);

KotlinDataSomeDefaultsBigBuilder a18(int x);

KotlinDataSomeDefaultsBigBuilder a19(int x);

KotlinDataSomeDefaultsBigBuilder a20(int x);

KotlinDataSomeDefaultsBigBuilder a21(int x);

KotlinDataSomeDefaultsBigBuilder a22(int x);

KotlinDataSomeDefaultsBigBuilder a23(int x);

KotlinDataSomeDefaultsBigBuilder a24(int x);

KotlinDataSomeDefaultsBigBuilder a25(int x);

KotlinDataSomeDefaultsBigBuilder a26(int x);

KotlinDataSomeDefaultsBigBuilder a27(int x);

KotlinDataSomeDefaultsBigBuilder a28(int x);

KotlinDataSomeDefaultsBigBuilder a29(int x);

KotlinDataSomeDefaultsBigBuilder a30(int x);

KotlinDataSomeDefaultsBigBuilder a31(int x);

KotlinDataSomeDefaultsBig build();
}

@Test
public void kotlinSomeDefaultsBig() {
KotlinDataSomeDefaultsBig allDefaulted =
KotlinDataSomeDefaultsBigBuilder.builder().requiredInt(23).requiredString("skidoo").build();
assertThat(allDefaulted.getRequiredInt()).isEqualTo(23);
assertThat(allDefaulted.getRequiredString()).isEqualTo("skidoo");
assertThat(allDefaulted.getA1()).isEqualTo(1);
assertThat(allDefaulted.getA31()).isEqualTo(31);
}

@AutoBuilder(ofClass = KotlinDataWithList.class)
interface KotlinDataWithListBuilder {
static KotlinDataWithListBuilder builder() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ data class KotlinDataWithDefaults(
val anInt: Int = 23,
val anImmutableList: ImmutableList<String> = ImmutableList.of("foo"),
val notDefaulted: Long,
val aString: String = "skidoo"
val aString: String = "skidoo",
)

// Exactly 8 defaulted properties, in case we have a problem with sign-extending byte bitmasks.
Expand All @@ -44,7 +44,49 @@ data class KotlinDataSomeDefaults(
val requiredInt: Int,
val requiredString: String,
val optionalInt: Int = 23,
val optionalString: String = "Skidoo"
val optionalString: String = "Skidoo",
)

/**
* Class with 2 required properties and 31 optional ones. This validates that we use the total count
* of properties to compute how many default-value bitmasks the Kotlin constructor has. Using just
* the number of optional properties would be wrong, and would show up as passing only one `int`
* bitmask instead of two.
*/
data class KotlinDataSomeDefaultsBig(
val requiredInt: Int,
val requiredString: String,
val a1: Int = 1,
val a2: Int = 2,
val a3: Int = 3,
val a4: Int = 4,
val a5: Int = 5,
val a6: Int = 6,
val a7: Int = 7,
val a8: Int = 8,
val a9: Int = 9,
val a10: Int = 10,
val a11: Int = 11,
val a12: Int = 12,
val a13: Int = 13,
val a14: Int = 14,
val a15: Int = 15,
val a16: Int = 16,
val a17: Int = 17,
val a18: Int = 18,
val a19: Int = 19,
val a20: Int = 20,
val a21: Int = 21,
val a22: Int = 22,
val a23: Int = 23,
val a24: Int = 24,
val a25: Int = 25,
val a26: Int = 26,
val a27: Int = 27,
val a28: Int = 28,
val a29: Int = 29,
val a30: Int = 30,
val a31: Int = 31,
)

// CharSequence is an interface so the parameter appears from Java as List<? extends CharSequence>,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
import static com.google.auto.value.processor.AutoValueProcessor.OMIT_IDENTIFIERS_OPTION;
import static com.google.auto.value.processor.ClassNames.AUTO_ANNOTATION_NAME;
import static com.google.auto.value.processor.ClassNames.AUTO_BUILDER_NAME;
import static java.math.RoundingMode.CEILING;
import static java.util.stream.Collectors.joining;
import static java.util.stream.Collectors.toMap;
import static javax.lang.model.util.ElementFilter.constructorsIn;
Expand All @@ -40,6 +41,7 @@
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableSet;
import com.google.common.math.IntMath;
import java.io.IOException;
import java.io.OutputStream;
import java.io.UncheckedIOException;
Expand Down Expand Up @@ -250,7 +252,7 @@ private void generateForwardingClass(
.map(Element::asType)
.map(typeUtils()::erasure)
.forEach(constructorParameters::add);
int bitmaskCount = (executable.optionalParameterCount() + 31) / 32;
int bitmaskCount = IntMath.divide(executable.parameters().size(), 32, CEILING);
constructorParameters.addAll(
Collections.nCopies(bitmaskCount, typeUtils().getPrimitiveType(TypeKind.INT)));
String marker = "kot".concat("lin.jvm.internal.DefaultConstructorMarker"); // defeat shading
Expand All @@ -271,9 +273,9 @@ private void generateForwardingClass(

private Optional<String> maybeForwardingClass(
TypeElement autoBuilderType, Executable executable) {
return executable.optionalParameterCount() == 0
? Optional.empty()
: Optional.of(generatedClassName(autoBuilderType, "AutoBuilderBridge_"));
return executable.hasOptionalParameters()
? Optional.of(generatedClassName(autoBuilderType, "AutoBuilderBridge_"))
: Optional.empty();
}

private ImmutableSet<Property> propertySet(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,13 +18,15 @@
import static com.google.auto.common.MoreStreams.toImmutableList;
import static com.google.auto.common.MoreStreams.toImmutableMap;
import static java.lang.Math.min;
import static java.math.RoundingMode.CEILING;
import static java.util.stream.Collectors.joining;

import com.google.auto.value.processor.AutoValueishProcessor.Property;
import com.google.common.annotations.VisibleForTesting;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableSet;
import com.google.common.math.IntMath;
import java.util.stream.IntStream;
import java.util.stream.Stream;

Expand Down Expand Up @@ -142,7 +144,7 @@ private BuilderRequiredProperties(
.collect(toImmutableMap(trackedProperties::get, i -> i));

this.bitmaskFields =
IntStream.range(0, (trackedCount + 31) / 32)
IntStream.range(0, IntMath.divide(trackedCount, 32, CEILING))
.mapToObj(
i -> {
int bitBase = i * 32;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -91,8 +91,8 @@ boolean isOptional(String parameterName) {
return optionalParameters.contains(parameterName);
}

int optionalParameterCount() {
return optionalParameters.size();
boolean hasOptionalParameters() {
return !optionalParameters.isEmpty();
}

ImmutableList<TypeParameterElement> typeParameters() {
Expand Down

0 comments on commit ad284f6

Please sign in to comment.