Update Static Analysis #1350

RussellSpitzer · 2020-08-17T21:57:38Z

This is an incomplete upgrade but enough to compile the new code and expose the new warnings.

Version Bumps
baseline 0.55 -> 3.36.2
git-version 0.9 -> 0.12.3

Checkstyle Changes Require for Compile

LineLength no longer is a Treewalker so it needs to be moved
MissingJavaDocMethod is now it's own module, allowMissingThrows properties have been removed
MinLineCount was owned by MissingJavaDocMethod but we only set this to a huge number

FindBug Suppression Required for Compile

StrictUnusedVariables must be disabled because we have several hundred cases of method parameters not being used in their bodies

New Checkstyle Violations

RightCurly - We use Same but this triggers on private constructors with same line braces

New Baseline/Findbug Warnings

Final Class - Class not declared final but all constructors are private
Javadoc Missing Summary - No Summary mostly in our case this is where we have methods with only a @return
RawTypes - Using a parameterized class without a parameterized type
ReferenceEquality - We have a few instances of == instead of Equals
LambdaMethodReference - Using s -> s.foo instead of s::foo
LoggerEnclosingClass - using getLogger for super in child
UncessaryAnonymousClass - Extend Function instead of using a lambda
CollectionUndefinedEquality - Using equality or contains on Collection without it's own equals or badly behaving equality like (UTF8 strings)
JDKObsolete : Collection of many things
- Using Date (we use Date internally in some places we don't need to be using it)
- Using linked list ( Warning argues that it is not performed compared to ArrayList and we should us that unless we need nulls or want to benchmark)
- SortedMap is deprecated use NavigableMap instead
Public constructor for Abstract Class
CollectionStreamForeach : Collection.stream.foreach should be Collection.foreach
MissingOverrides

RussellSpitzer · 2020-08-20T16:20:06Z

So the Issue with Right Curly is that in previous versions of check style it was broken.
checkstyle/checkstyle#6345

We use these directive

        <module name="RightCurly"> <!-- Java Style Guide: Nonempty blocks: K & R style -->
            <property name="option" value="alone"/>
            <property name="tokens" value="CLASS_DEF, METHOD_DEF, CTOR_DEF, LITERAL_FOR, LITERAL_WHILE, STATIC_INIT, INSTANCE_INIT"/>
        </module>

On Constructors, which means every usage of private Constructor(){} is illegal. It just wasn't marked previously.

Since we have decided that this is ok (It's present in dozens of files) I'm going to remove the "CTOR_DEF" check

We have a few other instances of methods failing to place their new } on a new line, but only about 4. So I'm going to fix those.

WIP

Remove check for Constructors on a single line, We do this alot Fix several methods without braces on their own lines Fix incorrect indentation in a few files

RussellSpitzer · 2020-08-20T16:47:19Z

@kbendick I know you were interested in this, you can see from this build a large number of new warnings :) Some I think we can just turn off but I suggest that for this PR we probably just leave warnings as is and fix the warnings in subsequent PR's.

cc: @aokolnychyi + @rdblue

OrElse vs OrElseGet

RussellSpitzer · 2020-08-20T21:27:57Z

spark2/src/main/java/org/apache/iceberg/spark/source/Reader.java

      this.localityPreferred = options.get("locality").map(Boolean::parseBoolean)
-          .orElse(LOCALITY_WHITELIST_FS.contains(scheme));
+          .orElse(localityFallback);


Because schema is non-final we can't just switch this to a lambda

I think actually this would actually be fine, but to make the rules happy we would have to reassign Scheme anyway so I figured we may as well just put the result in a local var

We could refactor scheme detection into a method to make scheme effectively final here.

rdblue · 2020-08-20T21:47:51Z

.baseline/checkstyle/checkstyle.xml

            <property name="allowedAnnotations" value="Override, Test"/>
-            <property name="allowThrowsTagsForSubclasses" value="true"/>
+            <property name="validateThrows" value="false"/>


This seems more general than "allowThrowsTagsForSubclasses"? Are we sure it does the same thing?

It's a bit of a complicated story

checkstyle/checkstyle#7329

Long story short, the checks were basically broken in check style so they were removed and are no longer valid configs. We actually don't need to set "validateThrows" to false either since it is false by default. I just wanted to make sure we were aware that we weren't validating any throw related tags.

rdblue · 2020-08-20T21:48:08Z

arrow/src/main/java/org/apache/iceberg/arrow/vectorized/VectorizedArrowReader.java

@@ -347,7 +347,9 @@ public String toString() {
    }

    @Override
-    public void setBatchSize(int batchSize) {}
+    public void setBatchSize(int batchSize) {
+


Do we require the empty line?

Yep, we actually did for the Private Constructors as well but we were ignoring that everywhere so I deleted that check

<module name="RightCurly">  <property name="option" value="alone"/> <property name="tokens" value="CLASS_DEF, **METHOD_DEF**, CTOR_DEF, LITERAL_FOR, LITERAL_WHILE, STATIC_INIT, INSTANCE_INIT"/> </module>

Basically Check-style was just broken before (see my comment on this PR) and didn't work correctly on empty blocks.

I don't suppose it's possible to make it okay to have the right curly on the line just after the method definition?

That is allowed, I'll change it

Does that allow us to turn it back on for private constructors without too much trouble?

You want to have it on for private constructors and I can change all the references to

private Constructor() { }

Because I can do that no problem

I think that's what we do in most places isn't it? I was hoping that with this update it wouldn't be too many places to change. Probably good to update them to all be standard, right?

But we can do that in a separate PR if you want to keep this one small.

rdblue · 2020-08-20T21:48:51Z

orc/src/main/java/org/apache/iceberg/orc/ORCSchemaUtil.java

@@ -267,7 +267,7 @@ private static TypeDescription buildOrcProjection(Integer fieldId, Type type, bo
          // e.g. renaming column c -> d and adding new column d
          String name = Optional.ofNullable(mapping.get(nestedField.fieldId()))
              .map(OrcField::name)
-              .orElse(nestedField.name() + "_r" + nestedField.fieldId());
+              .orElseGet(() -> nestedField.name() + "_r" + nestedField.fieldId());


Nice that it catches these.

rdblue · 2020-08-20T21:50:55Z

Overall, looks good to me.

rdblue · 2020-08-21T17:21:50Z

Is this still WIP? It looks about ready to me. We can always follow up with more.

We decided to enforce no single line constructors

Use todoComment rules as a CommentRegex to find comments which lead with a word character. Flags these instances as a mistake.

RussellSpitzer · 2020-08-21T18:49:54Z

Updated even more rules,
No more single line constructors
Comments must not lead with a "word" character

rdblue · 2020-08-21T20:02:51Z

Thanks for fixing this, @RussellSpitzer! Really good to have checks working automatically.

RussellSpitzer · 2020-08-21T20:21:12Z

Thanks for reviewing!

RussellSpitzer mentioned this pull request Aug 17, 2020

Checkstyle Improvements #1328

Closed

probot-autolabeler bot added arrow core ORC INFRA labels Aug 20, 2020

RussellSpitzer added 2 commits August 20, 2020 11:43

Update Static Analysis WIP

ec364e7

WIP

Fix Style Issues that were Previously False Negatives

3bc9cd6

Remove check for Constructors on a single line, We do this alot Fix several methods without braces on their own lines Fix incorrect indentation in a few files

RussellSpitzer force-pushed the UpdateStaticAnalysis branch from d9441ae to 3bc9cd6 Compare August 20, 2020 16:44

Fix Optional orElse

b78acbc

OrElse vs OrElseGet

probot-autolabeler bot added MR spark labels Aug 20, 2020

RussellSpitzer commented Aug 20, 2020

View reviewed changes

rdblue reviewed Aug 20, 2020

View reviewed changes

RussellSpitzer added 2 commits August 20, 2020 17:36

Switch to Effectively Local Scheme in Reader

1aeaf75

Fix Spacing

0f23296

RussellSpitzer added 2 commits August 21, 2020 12:45

Fix all Single Line Constructors

e452662

We decided to enforce no single line constructors

Fix Comments without Whitespace

2ec9ead

Use todoComment rules as a CommentRegex to find comments which lead with a word character. Flags these instances as a mistake.

probot-autolabeler bot added API common labels Aug 21, 2020

RussellSpitzer changed the title ~~Update Static Analysis WIP~~ Update Static Analysis Aug 21, 2020

rdblue merged commit 44c1d00 into apache:master Aug 21, 2020

RussellSpitzer deleted the UpdateStaticAnalysis branch August 21, 2020 20:20

rdblue linked an issue Aug 21, 2020 that may be closed by this pull request

Checkstyle Improvements #1328

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update Static Analysis #1350

Update Static Analysis #1350

RussellSpitzer commented Aug 17, 2020 •

edited

Loading

RussellSpitzer commented Aug 20, 2020 •

edited

Loading

RussellSpitzer commented Aug 20, 2020

RussellSpitzer Aug 20, 2020

RussellSpitzer Aug 20, 2020

rdblue Aug 20, 2020 •

edited

Loading

rdblue Aug 20, 2020 •

edited

Loading

RussellSpitzer Aug 20, 2020 •

edited

Loading

rdblue Aug 20, 2020

RussellSpitzer Aug 20, 2020 •

edited

Loading

rdblue Aug 20, 2020

RussellSpitzer Aug 21, 2020

rdblue Aug 21, 2020

RussellSpitzer Aug 21, 2020

rdblue Aug 21, 2020

rdblue Aug 21, 2020

rdblue Aug 20, 2020

rdblue commented Aug 20, 2020

rdblue commented Aug 21, 2020

RussellSpitzer commented Aug 21, 2020

rdblue commented Aug 21, 2020

RussellSpitzer commented Aug 21, 2020

Update Static Analysis #1350

Update Static Analysis #1350

Conversation

RussellSpitzer commented Aug 17, 2020 • edited Loading

RussellSpitzer commented Aug 20, 2020 • edited Loading

RussellSpitzer commented Aug 20, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rdblue Aug 20, 2020 • edited Loading

Choose a reason for hiding this comment

rdblue Aug 20, 2020 • edited Loading

Choose a reason for hiding this comment

RussellSpitzer Aug 20, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RussellSpitzer Aug 20, 2020 • edited Loading

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

Choose a reason for hiding this comment

rdblue commented Aug 20, 2020

rdblue commented Aug 21, 2020

RussellSpitzer commented Aug 21, 2020

rdblue commented Aug 21, 2020

RussellSpitzer commented Aug 21, 2020

RussellSpitzer commented Aug 17, 2020 •

edited

Loading

RussellSpitzer commented Aug 20, 2020 •

edited

Loading

rdblue Aug 20, 2020 •

edited

Loading

rdblue Aug 20, 2020 •

edited

Loading

RussellSpitzer Aug 20, 2020 •

edited

Loading

RussellSpitzer Aug 20, 2020 •

edited

Loading