Skip to content

Commit 97f5bea

Browse files
Christoph Büscherkcm
Christoph Büscher
authored and
kcm
committed
Add OneStatementPerLineCheck to Checkstyle rules (#33682)
This change adds the OneStatementPerLineCheck to our checkstyle precommit checks. This rule restricts the number of statements per line to one. The resoning behind this is that it is very difficult to read multiple statements on one line. People seem to mostly use it in short lambdas and switch statements in our code base, but just going through the changes already uncovered some actual problems in randomization in test code, so I think its worth it.
1 parent 77db997 commit 97f5bea

File tree

19 files changed

+249
-105
lines changed

19 files changed

+249
-105
lines changed

‎buildSrc/src/main/resources/checkstyle.xml‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,8 @@
3535
<module name="OuterTypeFilename" />
3636
<!-- No line wraps inside of import and package statements. -->
3737
<module name="NoLineWrap" />
38+
<!-- only one statement per line should be allowed -->
39+
<module name="OneStatementPerLine"/>
3840
<!-- Each java file has only one outer class -->
3941
<module name="OneTopLevelClass" />
4042
<!-- The suffix L is preferred, because the letter l (ell) is often

‎modules/lang-painless/src/main/java/org/elasticsearch/painless/MethodWriter.java‎

Lines changed: 33 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -347,17 +347,39 @@ public void writeBinaryInstruction(Location location, Class<?> clazz, Operation
347347
}
348348

349349
switch (operation) {
350-
case MUL: math(GeneratorAdapter.MUL, getType(clazz)); break;
351-
case DIV: math(GeneratorAdapter.DIV, getType(clazz)); break;
352-
case REM: math(GeneratorAdapter.REM, getType(clazz)); break;
353-
case ADD: math(GeneratorAdapter.ADD, getType(clazz)); break;
354-
case SUB: math(GeneratorAdapter.SUB, getType(clazz)); break;
355-
case LSH: math(GeneratorAdapter.SHL, getType(clazz)); break;
356-
case USH: math(GeneratorAdapter.USHR, getType(clazz)); break;
357-
case RSH: math(GeneratorAdapter.SHR, getType(clazz)); break;
358-
case BWAND: math(GeneratorAdapter.AND, getType(clazz)); break;
359-
case XOR: math(GeneratorAdapter.XOR, getType(clazz)); break;
360-
case BWOR: math(GeneratorAdapter.OR, getType(clazz)); break;
350+
case MUL:
351+
math(GeneratorAdapter.MUL, getType(clazz));
352+
break;
353+
case DIV:
354+
math(GeneratorAdapter.DIV, getType(clazz));
355+
break;
356+
case REM:
357+
math(GeneratorAdapter.REM, getType(clazz));
358+
break;
359+
case ADD:
360+
math(GeneratorAdapter.ADD, getType(clazz));
361+
break;
362+
case SUB:
363+
math(GeneratorAdapter.SUB, getType(clazz));
364+
break;
365+
case LSH:
366+
math(GeneratorAdapter.SHL, getType(clazz));
367+
break;
368+
case USH:
369+
math(GeneratorAdapter.USHR, getType(clazz));
370+
break;
371+
case RSH:
372+
math(GeneratorAdapter.SHR, getType(clazz));
373+
break;
374+
case BWAND:
375+
math(GeneratorAdapter.AND, getType(clazz));
376+
break;
377+
case XOR:
378+
math(GeneratorAdapter.XOR, getType(clazz));
379+
break;
380+
case BWOR:
381+
math(GeneratorAdapter.OR, getType(clazz));
382+
break;
361383
default:
362384
throw location.createError(new IllegalStateException("Illegal tree structure."));
363385
}

‎modules/lang-painless/src/main/java/org/elasticsearch/painless/node/SSource.java‎

Lines changed: 25 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -417,18 +417,33 @@ void write(MethodWriter writer, Globals globals) {
417417
for (AStatement statement : statements) {
418418
statement.write(writer, globals);
419419
}
420-
421420
if (!methodEscape) {
422421
switch (scriptClassInfo.getExecuteMethod().getReturnType().getSort()) {
423-
case org.objectweb.asm.Type.VOID: break;
424-
case org.objectweb.asm.Type.BOOLEAN: writer.push(false); break;
425-
case org.objectweb.asm.Type.BYTE: writer.push(0); break;
426-
case org.objectweb.asm.Type.SHORT: writer.push(0); break;
427-
case org.objectweb.asm.Type.INT: writer.push(0); break;
428-
case org.objectweb.asm.Type.LONG: writer.push(0L); break;
429-
case org.objectweb.asm.Type.FLOAT: writer.push(0f); break;
430-
case org.objectweb.asm.Type.DOUBLE: writer.push(0d); break;
431-
default: writer.visitInsn(Opcodes.ACONST_NULL);
422+
case org.objectweb.asm.Type.VOID:
423+
break;
424+
case org.objectweb.asm.Type.BOOLEAN:
425+
writer.push(false);
426+
break;
427+
case org.objectweb.asm.Type.BYTE:
428+
writer.push(0);
429+
break;
430+
case org.objectweb.asm.Type.SHORT:
431+
writer.push(0);
432+
break;
433+
case org.objectweb.asm.Type.INT:
434+
writer.push(0);
435+
break;
436+
case org.objectweb.asm.Type.LONG:
437+
writer.push(0L);
438+
break;
439+
case org.objectweb.asm.Type.FLOAT:
440+
writer.push(0f);
441+
break;
442+
case org.objectweb.asm.Type.DOUBLE:
443+
writer.push(0d);
444+
break;
445+
default:
446+
writer.visitInsn(Opcodes.ACONST_NULL);
432447
}
433448
writer.returnValue();
434449
}

‎modules/percolator/src/main/java/org/elasticsearch/percolator/QueryAnalyzer.java‎

Lines changed: 46 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@
2424
import org.apache.lucene.queries.BlendedTermQuery;
2525
import org.apache.lucene.queries.CommonTermsQuery;
2626
import org.apache.lucene.search.BooleanClause;
27+
import org.apache.lucene.search.BooleanClause.Occur;
2728
import org.apache.lucene.search.BooleanQuery;
2829
import org.apache.lucene.search.BoostQuery;
2930
import org.apache.lucene.search.ConstantScoreQuery;
@@ -38,7 +39,6 @@
3839
import org.apache.lucene.search.SynonymQuery;
3940
import org.apache.lucene.search.TermInSetQuery;
4041
import org.apache.lucene.search.TermQuery;
41-
import org.apache.lucene.search.BooleanClause.Occur;
4242
import org.apache.lucene.search.spans.SpanFirstQuery;
4343
import org.apache.lucene.search.spans.SpanNearQuery;
4444
import org.apache.lucene.search.spans.SpanNotQuery;
@@ -489,43 +489,51 @@ private static Result handleConjunction(List<Result> conjunctions, Version versi
489489
return subResult;
490490
}
491491
}
492-
int msm = 0;
493-
boolean verified = true;
494-
boolean matchAllDocs = true;
495-
boolean hasDuplicateTerms = false;Set<QueryExtraction> extractions = new HashSet<>();
496-
Set<String> seenRangeFields = new HashSet<>();
497-
for (Result result : conjunctions) {
498-
// In case that there are duplicate query extractions we need to be careful with incrementing msm,
499-
// because that could lead to valid matches not becoming candidate matches:
500-
// query: (field:val1 AND field:val2) AND (field:val2 AND field:val3)
501-
// doc: field: val1 val2 val3
502-
// So lets be protective and decrease the msm:
503-
int resultMsm = result.minimumShouldMatch;
504-
for (QueryExtraction queryExtraction : result.extractions) {
505-
if (queryExtraction.range != null) {
506-
// In case of range queries each extraction does not simply increment the minimum_should_match
507-
// for that percolator query like for a term based extraction, so that can lead to more false
508-
// positives for percolator queries with range queries than term based queries.
509-
// The is because the way number fields are extracted from the document to be percolated.
510-
// Per field a single range is extracted and if a percolator query has two or more range queries
511-
// on the same field, then the minimum should match can be higher than clauses in the CoveringQuery.
512-
// Therefore right now the minimum should match is incremented once per number field when processing
513-
// the percolator query at index time.
514-
if (seenRangeFields.add(queryExtraction.range.fieldName)) {
515-
resultMsm = 1;
516-
} else {
517-
resultMsm = 0;
518-
}
519-
}
520-
521-
if (extractions.contains(queryExtraction)) {
522-
523-
resultMsm = 0;
524-
verified = false;
525-
break;
526-
}
527-
}
528-
msm += resultMsm;
492+
int msm = 0;
493+
boolean verified = true;
494+
boolean matchAllDocs = true;
495+
boolean hasDuplicateTerms = false;
496+
Set<QueryExtraction> extractions = new HashSet<>();
497+
Set<String> seenRangeFields = new HashSet<>();
498+
for (Result result : conjunctions) {
499+
// In case that there are duplicate query extractions we need to be careful with
500+
// incrementing msm,
501+
// because that could lead to valid matches not becoming candidate matches:
502+
// query: (field:val1 AND field:val2) AND (field:val2 AND field:val3)
503+
// doc: field: val1 val2 val3
504+
// So lets be protective and decrease the msm:
505+
int resultMsm = result.minimumShouldMatch;
506+
for (QueryExtraction queryExtraction : result.extractions) {
507+
if (queryExtraction.range != null) {
508+
// In case of range queries each extraction does not simply increment the
509+
// minimum_should_match
510+
// for that percolator query like for a term based extraction, so that can lead
511+
// to more false
512+
// positives for percolator queries with range queries than term based queries.
513+
// The is because the way number fields are extracted from the document to be
514+
// percolated.
515+
// Per field a single range is extracted and if a percolator query has two or
516+
// more range queries
517+
// on the same field, then the minimum should match can be higher than clauses
518+
// in the CoveringQuery.
519+
// Therefore right now the minimum should match is incremented once per number
520+
// field when processing
521+
// the percolator query at index time.
522+
if (seenRangeFields.add(queryExtraction.range.fieldName)) {
523+
resultMsm = 1;
524+
} else {
525+
resultMsm = 0;
526+
}
527+
}
528+
529+
if (extractions.contains(queryExtraction)) {
530+
531+
resultMsm = 0;
532+
verified = false;
533+
break;
534+
}
535+
}
536+
msm += resultMsm;
529537

530538
if (result.verified == false
531539
// If some inner extractions are optional, the result can't be verified

‎plugins/mapper-annotated-text/src/main/java/org/elasticsearch/index/mapper/annotatedtext/AnnotatedTextFieldMapper.java‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -295,7 +295,10 @@ public String toString() {
295295
StringBuilder sb = new StringBuilder();
296296
sb.append(textMinusMarkup);
297297
sb.append("\n");
298-
annotations.forEach(a -> {sb.append(a); sb.append("\n");});
298+
annotations.forEach(a -> {
299+
sb.append(a);
300+
sb.append("\n");
301+
});
299302
return sb.toString();
300303
}
301304

‎server/src/main/java/org/elasticsearch/common/rounding/Rounding.java‎

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -410,9 +410,14 @@ public static Rounding read(StreamInput in) throws IOException {
410410
Rounding rounding;
411411
byte id = in.readByte();
412412
switch (id) {
413-
case TimeUnitRounding.ID: rounding = new TimeUnitRounding(in); break;
414-
case TimeIntervalRounding.ID: rounding = new TimeIntervalRounding(in); break;
415-
default: throw new ElasticsearchException("unknown rounding id [" + id + "]");
413+
case TimeUnitRounding.ID:
414+
rounding = new TimeUnitRounding(in);
415+
break;
416+
case TimeIntervalRounding.ID:
417+
rounding = new TimeIntervalRounding(in);
418+
break;
419+
default:
420+
throw new ElasticsearchException("unknown rounding id [" + id + "]");
416421
}
417422
return rounding;
418423
}

‎server/src/main/java/org/elasticsearch/index/mapper/DocumentParser.java‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -882,7 +882,8 @@ private static Tuple<Integer, ObjectMapper> getDynamicParentMapper(ParseContext
882882
builder = new ObjectMapper.Builder(paths[i]).enabled(true);
883883
}
884884
Mapper.BuilderContext builderContext = new Mapper.BuilderContext(context.indexSettings().getSettings(),
885-
context.path()); mapper = (ObjectMapper) builder.build(builderContext);
885+
context.path());
886+
mapper = (ObjectMapper) builder.build(builderContext);
886887
if (mapper.nested() != ObjectMapper.Nested.NO) {
887888
throw new MapperParsingException("It is forbidden to create dynamic nested objects ([" + context.path().pathAsText(paths[i])
888889
+ "]) through `copy_to` or dots in field names");

‎server/src/main/java/org/elasticsearch/search/aggregations/InternalOrder.java‎

Lines changed: 12 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -502,10 +502,18 @@ public static void writeHistogramOrder(BucketOrder order, StreamOutput out, bool
502502
// convert the new order IDs to the old histogram order IDs.
503503
byte id;
504504
switch (order.id()) {
505-
case COUNT_DESC_ID: id = 4; break;
506-
case COUNT_ASC_ID: id = 3; break;
507-
case KEY_DESC_ID: id = 2; break;
508-
case KEY_ASC_ID: id = 1; break;
505+
case COUNT_DESC_ID:
506+
id = 4;
507+
break;
508+
case COUNT_ASC_ID:
509+
id = 3;
510+
break;
511+
case KEY_DESC_ID:
512+
id = 2;
513+
break;
514+
case KEY_ASC_ID:
515+
id = 1;
516+
break;
509517
default: throw new RuntimeException("unknown order id [" + order.id() + "]");
510518
}
511519
out.writeByte(id);

‎server/src/main/java/org/elasticsearch/search/query/QueryPhase.java‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -232,7 +232,10 @@ static boolean execute(SearchContext searchContext,
232232

233233
final Runnable checkCancelled;
234234
if (timeoutRunnable != null && cancellationRunnable != null) {
235-
checkCancelled = () -> { timeoutRunnable.run(); cancellationRunnable.run(); };
235+
checkCancelled = () -> {
236+
timeoutRunnable.run();
237+
cancellationRunnable.run();
238+
};
236239
} else if (timeoutRunnable != null) {
237240
checkCancelled = timeoutRunnable;
238241
} else if (cancellationRunnable != null) {

‎server/src/test/java/org/elasticsearch/common/settings/ScopedSettingsTests.java‎

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -152,7 +152,10 @@ public void testAddConsumer() {
152152
}
153153

154154
try {
155-
service.addSettingsUpdateConsumer(testSetting, testSetting2, (a, b) -> {consumer.set(a); consumer2.set(b);});
155+
service.addSettingsUpdateConsumer(testSetting, testSetting2, (a, b) -> {
156+
consumer.set(a);
157+
consumer2.set(b);
158+
});
156159
fail("setting not registered");
157160
} catch (IllegalArgumentException ex) {
158161
assertEquals("Setting is not registered for key [foo.bar.baz]", ex.getMessage());
@@ -467,7 +470,10 @@ public void testApply() {
467470

468471
AtomicInteger aC = new AtomicInteger();
469472
AtomicInteger bC = new AtomicInteger();
470-
service.addSettingsUpdateConsumer(testSetting, testSetting2, (a, b) -> {aC.set(a); bC.set(b);});
473+
service.addSettingsUpdateConsumer(testSetting, testSetting2, (a, b) -> {
474+
aC.set(a);
475+
bC.set(b);
476+
});
471477

472478
assertEquals(0, consumer.get());
473479
assertEquals(0, consumer2.get());

0 commit comments

Comments
 (0)