Fix compiler warnings in :server - part 2 #75792

pugnascotia · 2021-07-28T15:10:25Z

Part of #40366. Fix a number of javac issues when linting is enforced in server/. Due to the number of issues that this surfaces, the fixes will be spread of multiple PRs.

elasticmachine · 2021-07-28T15:10:29Z

Pinging @elastic/es-delivery (Team:Delivery)

pugnascotia · 2021-07-28T15:36:14Z

@elasticmachine run elasticsearch-ci/bwc

mark-vieira

Left a few questions but otherwise LGTM.

server/src/main/java/org/elasticsearch/common/collect/Iterators.java

mark-vieira · 2021-07-28T16:41:46Z

...src/main/java/org/elasticsearch/search/aggregations/bucket/composite/DoubleValuesSource.java

@@ -158,7 +158,7 @@ public void collect(int doc, long bucket) throws IOException {
    }

    @Override
-    LeafBucketCollector getLeafCollector(Comparable value, LeafReaderContext context, LeafBucketCollector next) {
+    LeafBucketCollector getLeafCollector(Comparable<?> value, LeafReaderContext context, LeafBucketCollector next) {


In cases like this, shouldn't use Comparable<Double> here so we can enforce this at compile time instead of adding an explicit runtime check?

Ah, I see, we need to update this method on SingleDimensionValuesSource to use the proper generic type argument instead of a wildcard. We probably have missed this in other areas as well. We should go back and check where we've used wildcards that there isn't already a proper type argument we should be using instead.

.../src/main/java/org/elasticsearch/search/aggregations/bucket/composite/InternalComposite.java

mark-vieira · 2021-07-28T16:52:42Z

...rc/main/java/org/elasticsearch/search/aggregations/bucket/histogram/DateIntervalWrapper.java

@@ -97,7 +97,7 @@ public String getPreferredName() {
    private DateHistogramInterval dateHistogramInterval;
    private IntervalTypeEnum intervalType = IntervalTypeEnum.NONE;

-    public static <T extends DateIntervalConsumer> void declareIntervalFields(ObjectParser<T, String> parser) {
+    public static <T extends DateIntervalConsumer<T>> void declareIntervalFields(ObjectParser<T, String> parser) {


AGain, shouldn't this be T extends DateIntervalConsumer<?>>? Am I not understanding this properly?

It's one of these super-confusing recursive types. I think it's correct.

mark-vieira · 2021-07-28T16:53:51Z

server/src/main/java/org/elasticsearch/search/aggregations/bucket/range/RangeAggregator.java

@@ -434,6 +434,7 @@ public static Aggregator buildWithoutAttemptedToAdaptToFilters(
    private final DocValueFormat format;
    protected final Range[] ranges;
    private final boolean keyed;
+    @SuppressWarnings("rawtypes")


Why is it we don't just use wildcard types in these scenarios?

You get into the situation of ? being different from ?. It's a mess.

mark-vieira

Added a few more comments.

mark-vieira · 2021-07-28T17:25:03Z

...java/org/elasticsearch/search/aggregations/bucket/composite/SingleDimensionValuesSource.java

@@ -100,7 +100,7 @@
    /**
     * Sets the after value for this source. Values that compares smaller are filtered.
     */
-    abstract void setAfter(Comparable value);
+    abstract void setAfter(Comparable<?> value);


This should be Comparable<T> IMO since this type is already generic.

I've done as you suggest, though all the implementations seem to handle arguments other than T, so I'm not 100% sure it's correct.

Hmm, I see now. Looks like we are lenient here in places and will accept string values. Given that everything compiles though that must imply we're doing either casting or reflection to call this with other types somewhere. Should be fine given type erasure but if the implementations expect other types we should allow it at compile time I suppose 🤷

I looked more carefully at how this method is called, and ended up reverting the wildcard replacement. It's "correct" given the implementations 🤷

mark-vieira · 2021-07-28T17:26:13Z

...src/main/java/org/elasticsearch/search/aggregations/bucket/composite/DoubleValuesSource.java

@@ -158,7 +158,7 @@ public void collect(int doc, long bucket) throws IOException {
    }

    @Override
-    LeafBucketCollector getLeafCollector(Comparable value, LeafReaderContext context, LeafBucketCollector next) {
+    LeafBucketCollector getLeafCollector(Comparable<?> value, LeafReaderContext context, LeafBucketCollector next) {


Ah, I see, we need to update this method on SingleDimensionValuesSource to use the proper generic type argument instead of a wildcard. We probably have missed this in other areas as well. We should go back and check where we've used wildcards that there isn't already a proper type argument we should be using instead.

mark-vieira · 2021-07-28T17:29:51Z

...src/main/java/org/elasticsearch/search/aggregations/bucket/composite/BinaryValuesSource.java

@@ -162,7 +162,7 @@ public void collect(int doc, long bucket) throws IOException {
    }

    @Override
-    LeafBucketCollector getLeafCollector(Comparable value, LeafReaderContext context, LeafBucketCollector next) {
+    LeafBucketCollector getLeafCollector(Comparable<?> value, LeafReaderContext context, LeafBucketCollector next) {


Same thing here, assuming we update SingleDimensionValuesSource we should make this Comparable<BytesRef>

pugnascotia · 2021-08-02T13:54:35Z

@elasticmachine update branch

Part of #40366. Fix a number of javac issues when linting is enforced in `server/`.

pugnascotia · 2021-08-02T14:56:57Z

Backported to 7.x in f3161a2.

pugnascotia added 7 commits July 27, 2021 20:30

Fix compiler warnings in aggregations

298a4ad

Fix compiler warnings in aggregations

81436b7

Fix compiler warnings in aggregations

11f31fc

Fix compiler warnings in aggregations

fe2d083

Fix compiler warnings in aggregations

76ab57e

Fix compiler warnings in injector and utils

ab634b5

Fix compiler warnings

20ca427

pugnascotia added :Delivery/Build Build or test infrastructure >refactoring v8.0.0 v7.15.0 labels Jul 28, 2021

pugnascotia requested a review from mark-vieira July 28, 2021 15:10

elasticmachine added the Team:Delivery Meta label for Delivery team label Jul 28, 2021

Checkstyle

6a52d36

mark-vieira approved these changes Jul 28, 2021

View reviewed changes

pugnascotia added 3 commits July 28, 2021 21:08

Address review comments

ede700c

Revert wildcard replacement for SingleDimensionValuesSource

35c0db8

Fixup

2f6f75d

Merge branch 'master' into 40366-lint-warnings-server-pt2

08512a1

pugnascotia merged commit 4bfead0 into elastic:master Aug 2, 2021

pugnascotia deleted the 40366-lint-warnings-server-pt2 branch August 2, 2021 14:45

pugnascotia added a commit that referenced this pull request Aug 2, 2021

Fix compiler warnings in :server - part 2 (#75792)

f3161a2

Part of #40366. Fix a number of javac issues when linting is enforced in `server/`.

mark-vieira added v8.0.0-alpha1 and removed v8.0.0 labels Aug 4, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix compiler warnings in :server - part 2 #75792

Fix compiler warnings in :server - part 2 #75792

pugnascotia commented Jul 28, 2021

elasticmachine commented Jul 28, 2021

pugnascotia commented Jul 28, 2021

mark-vieira left a comment

mark-vieira Jul 28, 2021

mark-vieira Jul 28, 2021

mark-vieira Jul 28, 2021

pugnascotia Jul 28, 2021

mark-vieira Jul 28, 2021

pugnascotia Jul 28, 2021

mark-vieira left a comment

mark-vieira Jul 28, 2021

pugnascotia Jul 28, 2021

mark-vieira Jul 28, 2021

pugnascotia Aug 2, 2021

mark-vieira Jul 28, 2021

mark-vieira Jul 28, 2021

pugnascotia commented Aug 2, 2021

pugnascotia commented Aug 2, 2021

Fix compiler warnings in :server - part 2 #75792

Fix compiler warnings in :server - part 2 #75792

Conversation

pugnascotia commented Jul 28, 2021

elasticmachine commented Jul 28, 2021

pugnascotia commented Jul 28, 2021

mark-vieira left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mark-vieira left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pugnascotia commented Aug 2, 2021

pugnascotia commented Aug 2, 2021