Skip to content

Commit 876dc8e

Browse files
fluentfuturefluentfuture
committed
Support {optional_arg? -> ...}
1 parent 21ff9ed commit 876dc8e

File tree

4 files changed

+177
-22
lines changed

4 files changed

+177
-22
lines changed

‎mug-errorprone/src/main/java/com/google/mu/errorprone/Placeholder.java‎

Lines changed: 23 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,13 +1,17 @@
11
package com.google.mu.errorprone;
22

33
import static com.google.common.base.CharMatcher.whitespace;
4+
import static com.google.common.collect.ImmutableSet.toImmutableSet;
45
import static com.google.mu.util.Substring.before;
56
import static com.google.mu.util.Substring.first;
67
import static com.google.mu.util.Substring.firstOccurrence;
8+
import static com.google.mu.util.Substring.word;
9+
import static com.google.mu.util.Substring.BoundStyle.INCLUSIVE;
710

811
import java.util.stream.Stream;
912

1013
import com.google.common.annotations.VisibleForTesting;
14+
import com.google.common.collect.ImmutableSet;
1115
import com.google.mu.util.Substring;
1216
import com.google.errorprone.VisitorState;
1317
import com.google.errorprone.fixes.FixedPosition;
@@ -24,11 +28,15 @@ final class Placeholder {
2428
private static final Substring.Pattern PLACEHOLDER_NAME_END =
2529
Stream.of("=", "->", ":").map(Substring::first).collect(firstOccurrence());
2630

31+
private static final Substring.Pattern OPTIONAL_PARAMETER =
32+
word().immediatelyBetween("", INCLUSIVE, "?", INCLUSIVE);
33+
34+
private final String placeholderText;
2735
private final Substring.Match match;
2836
private final String name;
2937

3038
Placeholder(Substring.Match match) {
31-
String placeholderText = match.skip(1, 1).toString();
39+
this.placeholderText = match.skip(1, 1).toString();
3240
this.match = match;
3341
this.name =
3442
before(PLACEHOLDER_NAME_END)
@@ -45,6 +53,19 @@ Substring.Match match() {
4553
return match;
4654
}
4755

56+
/** Returns true if the placeholder is of the form {foo? -> ...} */
57+
boolean hasOptionalParameter() {
58+
return hasConditionalOperator() && OPTIONAL_PARAMETER.from(name).orElse("").equals(name);
59+
}
60+
61+
/** Returns all the foo?, bar? references from the rhs of the -> operator */
62+
ImmutableSet<String> optionalParametersFromOperatorRhs() {
63+
return PLACEHOLDER_NAME_END
64+
.in(placeholderText)
65+
.map(op -> OPTIONAL_PARAMETER.repeatedly().from(op.after()).collect(toImmutableSet()))
66+
.orElse(ImmutableSet.of());
67+
}
68+
4869
/**
4970
* Returns the source position of this placeholder by looking in {@code node}.
5071
*
@@ -69,7 +90,7 @@ int getStartIndexInSource(String source) {
6990
.orElse(0);
7091
}
7192

72-
boolean requiresBooleanArg() {
93+
boolean hasConditionalOperator() {
7394
return PLACEHOLDER_NAME_END.from(match).orElse("").equals("->");
7495
}
7596

‎mug-errorprone/src/main/java/com/google/mu/errorprone/StringFormatArgsCheck.java‎

Lines changed: 30 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@
99

1010
import java.util.List;
1111
import java.util.Map;
12+
import java.util.Optional;
1213
import java.util.OptionalDouble;
1314
import java.util.OptionalInt;
1415
import java.util.OptionalLong;
@@ -32,6 +33,7 @@
3233
import com.google.common.collect.ImmutableMap;
3334
import com.google.common.collect.ImmutableSet;
3435
import com.google.common.collect.Multimaps;
36+
import com.google.common.collect.Sets;
3537
import com.google.errorprone.BugPattern;
3638
import com.google.errorprone.BugPattern.LinkType;
3739
import com.google.errorprone.VisitorState;
@@ -105,6 +107,7 @@ public final class StringFormatArgsCheck extends AbstractBugChecker
105107
new TypeName("com.google.mu.util.BiStream"),
106108
new TypeName("com.google.mu.util.Both"));
107109
private static final TypeName BOOLEAN_TYPE = TypeName.of(Boolean.class);
110+
private static final TypeName OPTIONAL_TYPE = TypeName.of(Optional.class);
108111
private static final Substring.Pattern ARG_COMMENT = Substring.spanningInOrder("/*", "*/");
109112

110113
@Override
@@ -286,18 +289,41 @@ private void checkFormatArgs(
286289
placeholder.name(),
287290
placeholder.name());
288291
}
289-
if (placeholder.requiresBooleanArg()) {
292+
if (placeholder.hasConditionalOperator()) {
290293
Type argType = ASTHelpers.getType(arg);
291294
checkingOn(() -> placeholder.sourcePosition(formatExpression, state))
292295
.require(
293296
ASTHelpers.isSameType(argType, state.getSymtab().booleanType, state)
294-
|| BOOLEAN_TYPE.isSameType(argType, state),
295-
"String format placeholder {%s} (defined as in \"%s\") is expected to be boolean,"
296-
+ " whereas argument <%s> is of type %s",
297+
|| BOOLEAN_TYPE.isSameType(argType, state)
298+
|| OPTIONAL_TYPE.isSameType(argType, state),
299+
"String format placeholder {%s} (defined as in \"%s\") is expected to be boolean or"
300+
+ " Optional, whereas argument <%s> is of type %s",
297301
placeholder.name(),
298302
placeholder,
299303
arg,
300304
argType);
305+
if (OPTIONAL_TYPE.isSameType(argType, state)) {
306+
checkingOn(() -> placeholder.sourcePosition(formatExpression, state))
307+
.require(
308+
placeholder.hasOptionalParameter(),
309+
"optional parameter {%s->} must be an identifier followed by a '?'",
310+
placeholder.name())
311+
.require(
312+
!placeholder.optionalParametersFromOperatorRhs().isEmpty(),
313+
"optional parameter %s must be referenced at least once to the right of"
314+
+ " {%s->}",
315+
placeholder.name(),
316+
placeholder.name())
317+
.require(
318+
placeholder
319+
.optionalParametersFromOperatorRhs()
320+
.equals(ImmutableSet.of(placeholder.name())),
321+
"unexpected optional parameters to the right of {%s->}: %s",
322+
placeholder.name(),
323+
Sets.difference(
324+
placeholder.optionalParametersFromOperatorRhs(),
325+
ImmutableSet.of(placeholder.name())));
326+
}
301327
}
302328
}
303329
checkingOn(invocation)

‎mug-errorprone/src/test/java/com/google/mu/errorprone/PlaceholderTest.java‎

Lines changed: 42 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -53,37 +53,63 @@ public void toString_notTheFirstLine_notTheLastLine() {
5353
}
5454

5555
@Test
56-
public void toString_multiLinePlaceholder() {
57-
Placeholder placeholder =
58-
placeholder(
59-
Substring.spanningInOrder("{", "}"), "SELECT\n {foo -> \nbar,\nbaz,}\n FROM tbl");
60-
assertThat(placeholder.toString()).isEqualTo("... <{foo -> \nbar,\nbaz,}>...");
56+
public void hasConditionalOperator_regularPlaceholder() {
57+
Placeholder placeholder = placeholder("{foo}", "my {foo}-{bar}");
58+
assertThat(placeholder.hasConditionalOperator()).isFalse();
6159
}
6260

6361
@Test
64-
public void requiresBooleanArg_regularPlaceholder() {
65-
Placeholder placeholder = placeholder("{foo}", "my {foo}-{bar}");
66-
assertThat(placeholder.requiresBooleanArg()).isFalse();
62+
public void hasConditionalOperator_equalOperatorIsNotBooleanType() {
63+
assertThat(placeholder("{foo=bar}").hasConditionalOperator()).isFalse();
64+
}
65+
66+
@Test
67+
public void hasConditionalOperator_arrowOperatorIsBooleanType() {
68+
assertThat(placeholder("{foo->bar}").hasConditionalOperator()).isTrue();
69+
}
70+
71+
@Test
72+
public void hasConditionalOperator_arrowOperatorAfterEqualOperatorIsNotBooleanType() {
73+
assertThat(placeholder("{foo=a->b}").hasConditionalOperator()).isFalse();
74+
}
75+
76+
@Test
77+
public void hasConditionalOperator_arrowOperatorBeforeEqualOperatorIsBooleanType() {
78+
assertThat(placeholder("{foo->a=b}").hasConditionalOperator()).isTrue();
79+
}
80+
81+
@Test
82+
public void hasOptionalParameter_noQuestionMark_not() {
83+
assertThat(placeholder("{foo->a=b}").hasOptionalParameter()).isFalse();
84+
}
85+
86+
@Test
87+
public void hasOptionalParameter_notProperWord_not() {
88+
assertThat(placeholder("{foo-bar?->a=b}").hasOptionalParameter()).isFalse();
6789
}
6890

6991
@Test
70-
public void requiresBooleanArg_equalOperatorIsNotBooleanType() {
71-
assertThat(placeholder("{foo=bar}").requiresBooleanArg()).isFalse();
92+
public void hasOptionalParameter_properWordWithQuestionMark_yes() {
93+
assertThat(placeholder("{optional_bar? -> a=b}").hasOptionalParameter()).isTrue();
7294
}
7395

7496
@Test
75-
public void requiresBooleanArg_arrowOperatorIsBooleanType() {
76-
assertThat(placeholder("{foo->bar}").requiresBooleanArg()).isTrue();
97+
public void optionalParametersFromOperatorRhs_none() {
98+
assertThat(placeholder("{optional_bar? -> a=b}").optionalParametersFromOperatorRhs()).isEmpty();
7799
}
78100

79101
@Test
80-
public void requiresBooleanArg_arrowOperatorAfterEqualOperatorIsNotBooleanType() {
81-
assertThat(placeholder("{foo=a->b}").requiresBooleanArg()).isFalse();
102+
public void optionalParametersFromOperatorRhs_one() {
103+
assertThat(placeholder("{optional_bar? -> optional_bar?}").optionalParametersFromOperatorRhs())
104+
.containsExactly("optional_bar?")
105+
.inOrder();
82106
}
83107

84108
@Test
85-
public void requiresBooleanArg_arrowOperatorBeforeEqualOperatorIsBooleanType() {
86-
assertThat(placeholder("{foo->a=b}").requiresBooleanArg()).isTrue();
109+
public void optionalParametersFromOperatorRhs_two() {
110+
assertThat(placeholder("{optional_bar? -> a?=b?}").optionalParametersFromOperatorRhs())
111+
.containsExactly("a?", "b?")
112+
.inOrder();
87113
}
88114

89115
@Test

‎mug-errorprone/src/test/java/com/google/mu/errorprone/StringFormatArgsCheckTest.java‎

Lines changed: 82 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2975,6 +2975,88 @@ public void templateFormatMethod_wrapperBooleanForArrowOperator_allowed() {
29752975
.doTest();
29762976
}
29772977

2978+
@Test
2979+
public void templateFormatMethod_optionalParameterForArrowOperator_allowed() {
2980+
helper
2981+
.addSourceLines(
2982+
"Test.java",
2983+
"import com.google.mu.annotations.TemplateFormatMethod;",
2984+
"import com.google.mu.annotations.TemplateString;",
2985+
"import java.util.Optional;",
2986+
"class Test {",
2987+
" void test(Optional<String> foo) {",
2988+
" query(\"SELECT {foo? -> , concat(foo?, 'foo?')}\", foo);",
2989+
" }",
2990+
" @TemplateFormatMethod",
2991+
" void query(@TemplateString String template, Object... args) {}",
2992+
"}")
2993+
.doTest();
2994+
}
2995+
2996+
@Test
2997+
public void templateFormatMethod_optionalParameterForArrowOperator_notProperWord_disallowed() {
2998+
helper
2999+
.addSourceLines(
3000+
"Test.java",
3001+
"import com.google.mu.annotations.TemplateFormatMethod;",
3002+
"import com.google.mu.annotations.TemplateString;",
3003+
"import java.util.Optional;",
3004+
3005+
"class Test {",
3006+
" void test(Optional<String> foo) {",
3007+
" // BUG: Diagnostic contains: {foo->} must be an identifier followed by a '?'",
3008+
" query(\"SELECT {foo -> , concat(foo, 'foo')}\", foo);",
3009+
" }",
3010+
3011+
" @TemplateFormatMethod",
3012+
" void query(@TemplateString String template, Object... args) {}",
3013+
"}")
3014+
.doTest();
3015+
}
3016+
3017+
@Test
3018+
public void
3019+
templateFormatMethod_optionalParameterForArrowOperator_missingQuestionMark_disallowed() {
3020+
helper
3021+
.addSourceLines(
3022+
"Test.java",
3023+
"import com.google.mu.annotations.TemplateFormatMethod;",
3024+
"import com.google.mu.annotations.TemplateString;",
3025+
"import java.util.Optional;",
3026+
3027+
"class Test {",
3028+
" void test(Optional<String> foo) {",
3029+
" // BUG: Diagnostic contains: {foo->} must be an identifier followed by a '?'",
3030+
" query(\"SELECT {foo -> , concat(foo, 'foo')}\", foo);",
3031+
" }",
3032+
3033+
" @TemplateFormatMethod",
3034+
" void query(@TemplateString String template, Object... args) {}",
3035+
"}")
3036+
.doTest();
3037+
}
3038+
3039+
@Test
3040+
public void templateFormatMethod_optionalParameterForArrowOperator_typoInReference_disallowed() {
3041+
helper
3042+
.addSourceLines(
3043+
"Test.java",
3044+
"import com.google.mu.annotations.TemplateFormatMethod;",
3045+
"import com.google.mu.annotations.TemplateString;",
3046+
"import java.util.Optional;",
3047+
3048+
"class Test {",
3049+
" void test(Optional<String> foo) {",
3050+
" // BUG: Diagnostic contains: to the right of {foo?->}: [food?]",
3051+
" query(\"SELECT {foo? -> , concat(food?, 'foo')}\", foo);",
3052+
" }",
3053+
3054+
" @TemplateFormatMethod",
3055+
" void query(@TemplateString String template, Object... args) {}",
3056+
"}")
3057+
.doTest();
3058+
}
3059+
29783060
@Test
29793061
public void arrayArgDisallowed() {
29803062
helper

0 commit comments

Comments
 (0)