Skip to content
This repository was archived by the owner on Oct 16, 2024. It is now read-only.

Commit d125615

Browse files
alicederynalicederyn
authored
Merge pull request #262 from google/issues/261.optional.property.named.result
Fix variable naming conflicts in toString code
2 parents 5ea5a77 + b5ea61f commit d125615

File tree

2 files changed

+99
-36
lines changed

2 files changed

+99
-36
lines changed

‎src/main/java/org/inferred/freebuilder/processor/CodeGenerator.java

+44-36
Original file line numberDiff line numberDiff line change
@@ -201,7 +201,7 @@ private static void addMergeFromBuilderMethod(SourceBuilder code, Metadata metad
201201
.addLine(" * Does not affect any properties not set on the input.")
202202
.addLine(" */")
203203
.addLine("public %1$s mergeFrom(%1$s template) {", metadata.getBuilder());
204-
Block body = Block.methodBody(code, "template");
204+
Block body = methodBody(code, "template");
205205
for (Property property : metadata.getProperties()) {
206206
property.getCodeGenerator().addMergeFromBuilder(body, "template");
207207
}
@@ -425,66 +425,68 @@ private static void addValueTypeToString(SourceBuilder code, Metadata metadata)
425425
code.addLine("")
426426
.addLine(" @%s", Override.class)
427427
.addLine(" public %s toString() {", String.class);
428+
Block body = methodBody(code);
428429
switch (metadata.getProperties().size()) {
429430
case 0: {
430-
code.addLine(" return \"%s{}\";", metadata.getType().getSimpleName());
431+
body.addLine(" return \"%s{}\";", metadata.getType().getSimpleName());
431432
break;
432433
}
433434

434435
case 1: {
435-
code.add(" return \"%s{", metadata.getType().getSimpleName());
436+
body.add(" return \"%s{", metadata.getType().getSimpleName());
436437
Property property = getOnlyElement(metadata.getProperties());
437438
if (property.getCodeGenerator().getType() == Type.OPTIONAL) {
438-
code.add("\" + (%1$s != null ? \"%2$s=\" + %1$s : \"\") + \"}\";\n",
439+
body.add("\" + (%1$s != null ? \"%2$s=\" + %1$s : \"\") + \"}\";\n",
439440
property.getField(), property.getName());
440441
} else {
441-
code.add("%s=\" + %s + \"}\";\n", property.getName(), property.getField());
442+
body.add("%s=\" + %s + \"}\";\n", property.getName(), property.getField());
442443
}
443444
break;
444445
}
445446

446447
default: {
447448
if (!any(metadata.getProperties(), IS_OPTIONAL)) {
448449
// If none of the properties are optional, use string concatenation for performance.
449-
code.addLine(" return \"%s{\"", metadata.getType().getSimpleName());
450+
body.addLine(" return \"%s{\"", metadata.getType().getSimpleName());
450451
Property lastProperty = getLast(metadata.getProperties());
451452
for (Property property : metadata.getProperties()) {
452-
code.add(" + \"%s=\" + %s", property.getName(), property.getField());
453+
body.add(" + \"%s=\" + %s", property.getName(), property.getField());
453454
if (property != lastProperty) {
454-
code.add(" + \", \"\n");
455+
body.add(" + \", \"\n");
455456
} else {
456-
code.add(" + \"}\";\n");
457+
body.add(" + \"}\";\n");
457458
}
458459
}
459-
} else if (code.feature(GUAVA).isAvailable()) {
460+
} else if (body.feature(GUAVA).isAvailable()) {
460461
// If Guava is available, use COMMA_JOINER for readability.
461-
code.addLine(" return \"%s{\"", metadata.getType().getSimpleName())
462+
body.addLine(" return \"%s{\"", metadata.getType().getSimpleName())
462463
.addLine(" + COMMA_JOINER.join(");
463464
Property lastProperty = getLast(metadata.getProperties());
464465
for (Property property : metadata.getProperties()) {
465-
code.add(" ");
466+
body.add(" ");
466467
if (property.getCodeGenerator().getType() == Type.OPTIONAL) {
467-
code.add("(%s != null ? ", property.getField());
468+
body.add("(%s != null ? ", property.getField());
468469
}
469-
code.add("\"%s=\" + %s", property.getName(), property.getField());
470+
body.add("\"%s=\" + %s", property.getName(), property.getField());
470471
if (property.getCodeGenerator().getType() == Type.OPTIONAL) {
471-
code.add(" : null)");
472+
body.add(" : null)");
472473
}
473474
if (property != lastProperty) {
474-
code.add(",\n");
475+
body.add(",\n");
475476
} else {
476-
code.add(")\n");
477+
body.add(")\n");
477478
}
478479
}
479-
code.addLine(" + \"}\";");
480+
body.addLine(" + \"}\";");
480481
} else {
481482
// Use StringBuilder if no better choice is available.
482-
writeToStringWithBuilder(code, metadata, false);
483+
writeToStringWithBuilder(body, metadata, false);
483484
}
484485
break;
485486
}
486487
}
487-
code.addLine(" }");
488+
code.add(body)
489+
.addLine(" }");
488490
}
489491

490492
private static void addPartialType(SourceBuilder code, Metadata metadata) {
@@ -626,12 +628,14 @@ private static void addPartialType(SourceBuilder code, Metadata metadata) {
626628
code.addLine("")
627629
.addLine(" @%s", Override.class)
628630
.addLine(" public %s toString() {", String.class);
629-
if (metadata.getProperties().size() > 1 && !code.feature(GUAVA).isAvailable()) {
630-
writeToStringWithBuilder(code, metadata, true);
631+
Block body = methodBody(code);
632+
if (metadata.getProperties().size() > 1 && !body.feature(GUAVA).isAvailable()) {
633+
writeToStringWithBuilder(body, metadata, true);
631634
} else {
632-
writePartialToStringWithConcatenation(code, metadata);
635+
writePartialToStringWithConcatenation(body, metadata);
633636
}
634-
code.addLine(" }");
637+
code.add(body)
638+
.addLine(" }");
635639
}
636640
code.addLine("}");
637641
}
@@ -668,14 +672,17 @@ private static void addPartialToBuilderMethod(SourceBuilder code, Metadata metad
668672
code.addLine(" }");
669673
}
670674

671-
private static void writeToStringWithBuilder(
672-
SourceBuilder code, Metadata metadata, boolean isPartial) {
673-
code.addLine("%1$s result = new %1$s(\"%2$s%3$s{\");",
674-
StringBuilder.class, isPartial ? "partial " : "", metadata.getType().getSimpleName());
675+
private static void writeToStringWithBuilder(Block code, Metadata metadata, boolean isPartial) {
676+
Excerpt result = code.declare(
677+
Excerpts.add("%s", StringBuilder.class),
678+
"result",
679+
Excerpts.add("new %s(\"%s%s{\")",
680+
StringBuilder.class, isPartial ? "partial " : "", metadata.getType().getSimpleName()));
675681
boolean noDefaults = !any(metadata.getProperties(), HAS_DEFAULT);
682+
Excerpt separator = null;
676683
if (noDefaults) {
677684
// We need to keep track of whether to output a separator
678-
code.addLine("String separator = \"\";");
685+
separator = code.declare(Excerpts.add("String"), "separator", Excerpts.add("\"\""));
679686
}
680687
boolean seenDefault = false;
681688
Property first = metadata.getProperties().get(0);
@@ -699,15 +706,16 @@ private static void writeToStringWithBuilder(
699706
break;
700707
}
701708
if (noDefaults && property != first) {
702-
code.addLine("result.append(separator);");
709+
code.addLine("%s.append(%s);", result, separator);
703710
} else if (!noDefaults && hadSeenDefault) {
704-
code.addLine("result.append(\", \");");
711+
code.addLine("%s.append(\", \");", result);
705712
}
706-
code.addLine("result.append(\"%s=\").append(%s);", property.getName(), property.getField());
713+
code.addLine("%s.append(\"%s=\").append(%s);",
714+
result, property.getName(), property.getField());
707715
if (!noDefaults && !seenDefault) {
708-
code.addLine("result.append(\", \");");
716+
code.addLine("%s.append(\", \");", result);
709717
} else if (noDefaults && property != last) {
710-
code.addLine("separator = \", \";");
718+
code.addLine("%s = \", \";", separator);
711719
}
712720
switch (property.getCodeGenerator().getType()) {
713721
case HAS_DEFAULT:
@@ -724,8 +732,8 @@ private static void writeToStringWithBuilder(
724732
break;
725733
}
726734
}
727-
code.addLine("result.append(\"}\");")
728-
.addLine("return result.toString();");
735+
code.addLine("%s.append(\"}\");", result)
736+
.addLine("return %s.toString();", result);
729737
}
730738

731739
private static void writePartialToStringWithConcatenation(SourceBuilder code, Metadata metadata) {

‎src/test/java/org/inferred/freebuilder/processor/ProcessorTest.java

+55
Original file line numberDiff line numberDiff line change
@@ -49,6 +49,7 @@
4949
import java.lang.reflect.Method;
5050
import java.util.List;
5151

52+
import javax.annotation.Nullable;
5253
import javax.tools.JavaFileObject;
5354

5455
@RunWith(Parameterized.class)
@@ -593,6 +594,60 @@ public void testToString_twoPrimitiveProperties() {
593594
.runTest();
594595
}
595596

597+
@Test
598+
public void testToString_nullablePropertyNamedResult() {
599+
// See https://github.com/google/FreeBuilder/issues/261
600+
behaviorTester
601+
.with(new Processor(features))
602+
.with(new SourceBuilder()
603+
.addLine("package com.example;")
604+
.addLine("@%s", FreeBuilder.class)
605+
.addLine("public abstract class DataType {")
606+
.addLine(" public abstract @%s String getValue();", Nullable.class)
607+
.addLine(" public abstract @%s String getResult();", Nullable.class)
608+
.addLine("")
609+
.addLine(" public static class Builder extends DataType_Builder {}")
610+
.addLine(" public static Builder builder() {")
611+
.addLine(" return new Builder();")
612+
.addLine(" }")
613+
.addLine("}")
614+
.build())
615+
.with(new TestBuilder()
616+
.addLine("com.example.DataType value = com.example.DataType.builder()")
617+
.addLine(" .setResult(\"fred\")")
618+
.addLine(" .build();")
619+
.addLine("assertEquals(\"DataType{result=fred}\", value.toString());")
620+
.build())
621+
.runTest();
622+
}
623+
624+
@Test
625+
public void testPartialToString_propertyNamedResult() {
626+
// See https://github.com/google/FreeBuilder/issues/261
627+
behaviorTester
628+
.with(new Processor(features))
629+
.with(new SourceBuilder()
630+
.addLine("package com.example;")
631+
.addLine("@%s", FreeBuilder.class)
632+
.addLine("public abstract class DataType {")
633+
.addLine(" public abstract String getValue();")
634+
.addLine(" public abstract String getResult();")
635+
.addLine("")
636+
.addLine(" public static class Builder extends DataType_Builder {}")
637+
.addLine(" public static Builder builder() {")
638+
.addLine(" return new Builder();")
639+
.addLine(" }")
640+
.addLine("}")
641+
.build())
642+
.with(new TestBuilder()
643+
.addLine("com.example.DataType value = com.example.DataType.builder()")
644+
.addLine(" .setResult(\"fred\")")
645+
.addLine(" .buildPartial();")
646+
.addLine("assertEquals(\"partial DataType{result=fred}\", value.toString());")
647+
.build())
648+
.runTest();
649+
}
650+
596651
@Test
597652
public void testGwtSerialize_twoStringProperties() {
598653
behaviorTester

0 commit comments

Comments
 (0)