Add errorprone verifications #5873

vlsi · 2023-05-01T13:10:06Z

The code changes are performed using two approaches:

IDEA's "run inspection by name". "field may be static", "field may be final", "method may be static", "replace iterator with foreach"
Manual edits driven by Errorprone failures: remove default cases in enum switch, and some of "method may be static"

In other words, there are no changes that were made "just in case", and the code changes are accompanied with the corresponding Errorprone verifications, so we won't accidentally re-introduce the same issues.

build-logic/verification/src/main/kotlin/build-logic.errorprone.gradle.kts

FSchumacher · 2023-05-01T14:34:56Z

src/components/src/main/java/org/apache/jmeter/assertions/CompareAssertion.java

@@ -126,8 +126,8 @@ private void compareContent(CompareAssertionResult result) {
        }
    }

-    private void markContentFailure(CompareAssertionResult result, String prevContent, SampleResult prevResult,
-            SampleResult currentResult, String currentContent) {
+    private static void markContentFailure(CompareAssertionResult result, String prevContent, SampleResult prevResult,


In my current IDE setup, the IDE suggests this kind of change quite often and I wonder why. Is it better performing, more stylish, something else?

The key feature is that static keyword makes it clear that the method does not access instance fields, so all the variables are local

src/components/src/main/java/org/apache/jmeter/visualizers/SearchTextExtension.java

FSchumacher · 2023-05-01T14:42:13Z

src/core/src/main/java/org/apache/jmeter/gui/action/AbstractAction.java

@@ -102,7 +102,6 @@ protected boolean popupCheckExistingFileListener(HashTree tree) {
                            return false;
                        }
                    case ASK:
-                    default:


While it might not be nice to have a fall-through from ASK to default. Removing default seems wrong (haven't looked deeply into this).

The removal of default fo enums is indeed the most controversial changes here.
However, in this case, the switch goes over private enum:

private enum ActionOnFile { APPEND, DELETE, ASK }

So it is clear there are only three possibilities, so the default can be omitted.

At the same time, if some of enum cases are missing, the compilation would fail.

The questionable part is whether we trust enums from third-party classes. In other words, even if we cover all the current cases, it might be the library would add a new case, so if users upgrade the library without recompiling jmeter, then it won't detect "unexpected enum value".

WDYT? Is it really that important?
Frankly speaking, I thin that an exhaustive switch over enum values looks better than arbitrary default that might unexpectedly catch newly added enum values. In other words, if we omit default for enums, then the newly added value would fail the compilation and ask the developer to add an appropriate case rather than falling into an unknown default.

src/core/src/main/java/org/apache/jmeter/report/processor/ExternalSampleSorter.java

FSchumacher · 2023-05-01T14:47:37Z

src/functions/src/main/java/org/apache/jmeter/functions/LogFunction.java

@@ -175,8 +175,6 @@ static synchronized void logDetails(Logger logger, String stringToLog, String pr
                case TRACE:
                    logger.trace("{} {} {}", threadName, separator, stringToLog, throwable);
                    break;
-                default:


Seems wrong to be removed.

Do you really think we should protect from the case when org.slf4j.event.Level enum will have extra enum value in runtime?
I think it would be unlikely, and if we drop the default, then we'll know when slf4j adds new value in compile-time.

FSchumacher · 2023-05-01T14:49:09Z

src/jorphan/src/main/java/org/apache/jorphan/gui/MenuScroller.java

-    @Override
-    @SuppressWarnings("deprecation")
-    public void finalize() throws Throwable {
-        dispose();


Can we remove this already? Do we have to take care of dspose() somewhere else?

Finalize runs whenever the object becomes unreachable. Currently, MenuScroller is discarded as soon as it is created. I believe this finalize has never been useful.

FSchumacher · 2023-05-01T14:52:00Z

...otocol/http/src/main/java/org/apache/jmeter/protocol/http/parser/LagartoBasedHtmlParser.java

@@ -181,8 +181,6 @@ public void tag(Tag tag) {
                break;
            case END:
                break;
-            default:


Seems wrong to remove

This is a switch over jodd.lagarto.TagType enum. The current switch covers all enum cases.
I think it would be better to ensure enum switches are exhaustive rather than fail in runtime.

src/protocol/http/src/main/java/org/apache/jmeter/protocol/http/proxy/ProxyControl.java

src/protocol/http/src/main/java/org/apache/jmeter/protocol/http/sampler/HTTPAbstractImpl.java

…vated

… in the source code See https://errorprone.info/bugpattern/LongDoubleConversion

…mpiler only ErrorProne requires Java 11, so we can't execute it with Java 8 compiler.

The redundant overrides for clone are left intact as they are needed for Sonar rule S2157 - "Cloneables" should implement "clone"

See https://errorprone.info/bugpattern/UnnecessaryDefaultInEnumSwitch

See https://errorprone.info/bugpattern/MethodCanBeStatic

See https://errorprone.info/bugpattern/FieldCanBeFinal

See https://errorprone.info/bugpattern/FieldCanBeStatic # Conflicts: # build-logic/verification/src/main/kotlin/build-logic.errorprone.gradle.kts

There's no much sense in alignling the parameters just like with most the other cases for horizontal alignment. See https://google.github.io/styleguide/javaguide.html#s4.6.3-horizontal-alignment

vlsi force-pushed the errorprone branch 2 times, most recently from 48ead5b to 4ca844f Compare May 1, 2023 13:48

FSchumacher reviewed May 1, 2023

View reviewed changes

vlsi force-pushed the errorprone branch from 4ca844f to 86551cd Compare May 1, 2023 15:51

vlsi added 7 commits May 4, 2023 13:37

chore: fix condition for enableErrorprone so the verification is acti…

f1d4844

…vated

chore: add explicit casts from long to double so data loss is visible…

df7c8bd

… in the source code See https://errorprone.info/bugpattern/LongDoubleConversion

chore: remove use of deprecated Object#finalize

5a993bf

chore: execute ErrorProne verifications when running with Java 11+ co…

0b371e0

…mpiler only ErrorProne requires Java 11, so we can't execute it with Java 8 compiler.

style: remove redundant method overrides

4a7057c

The redundant overrides for clone are left intact as they are needed for Sonar rule S2157 - "Cloneables" should implement "clone"

style: remove unnecessary default cases in enum switch

8b413c0

See https://errorprone.info/bugpattern/UnnecessaryDefaultInEnumSwitch

style: replace iterator with enhanced for loop

11466ea

vlsi force-pushed the errorprone branch 2 times, most recently from d5bc20f to 453b470 Compare May 5, 2023 08:33

vlsi added 4 commits May 5, 2023 12:25

style: apply "private method can be static" inspection

12aefb0

See https://errorprone.info/bugpattern/MethodCanBeStatic

style: run "field may be final" inspection

049cc3a

See https://errorprone.info/bugpattern/FieldCanBeFinal

style: run "field can be static"

6a0de9b

See https://errorprone.info/bugpattern/FieldCanBeStatic # Conflicts: # build-logic/verification/src/main/kotlin/build-logic.errorprone.gradle.kts

style: prevent alignment of method parameters via .editorconfig

af92f07

There's no much sense in alignling the parameters just like with most the other cases for horizontal alignment. See https://google.github.io/styleguide/javaguide.html#s4.6.3-horizontal-alignment

vlsi force-pushed the errorprone branch from 453b470 to af92f07 Compare May 5, 2023 09:26

vlsi merged commit af92f07 into apache:master May 5, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add errorprone verifications #5873

Add errorprone verifications #5873

vlsi commented May 1, 2023 •

edited

Loading

FSchumacher May 1, 2023

vlsi May 1, 2023

FSchumacher May 1, 2023

vlsi May 1, 2023

vlsi May 1, 2023 •

edited

Loading

FSchumacher May 1, 2023

vlsi May 1, 2023

FSchumacher May 1, 2023

vlsi May 1, 2023

FSchumacher May 1, 2023

vlsi May 1, 2023

Add errorprone verifications #5873

Add errorprone verifications #5873

Conversation

vlsi commented May 1, 2023 • 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

vlsi May 1, 2023 • 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

vlsi commented May 1, 2023 •

edited

Loading

vlsi May 1, 2023 •

edited

Loading