Modify

Opened 7 years ago

Closed 7 years ago

Last modified 7 years ago

#12472 closed enhancement (fixed)

Java 8: Use error-prone in build

Reported by: simon04 Owned by: Don-vip
Priority: normal Milestone: 16.07
Component: Core Version:
Keywords: build error prone ant java8 Cc: Don-vip, bastiK

Description

Error-prone aims at identifying issues at compile time. Running it on the current code base reveals:

compile:
    [javac] Compiling 434 source files to /home/simon/src/josm.svngit/build
    [javac] Note: /home/simon/src/josm.svngit/src/oauth/signpost/AbstractOAuthProvider.java uses or overrides a deprecated API.
    [javac] Note: Recompile with -Xlint:deprecation for details.
    [javac] Note: Some input files use unchecked or unsafe operations.
    [javac] Note: Recompile with -Xlint:unchecked for details.
    [javac] Compiling 60 source files to /home/simon/src/josm.svngit/build
    [javac] /home/simon/src/josm.svngit/src/org/openstreetmap/gui/jmapviewer/JMapViewer.java:305: warning: [SynchronizeOnNonFinalField] Synchronizing on non-final fields is not safe: if the field is ever updated, different threads may end up locking on different objects.
    [javac]             synchronized (mapMarkerList) {
    [javac]                          ^
    [javac]     (see http://errorprone.info/bugpattern/SynchronizeOnNonFinalField)
    [javac] /home/simon/src/josm.svngit/src/org/openstreetmap/gui/jmapviewer/JMapViewer.java:319: warning: [SynchronizeOnNonFinalField] Synchronizing on non-final fields is not safe: if the field is ever updated, different threads may end up locking on different objects.
    [javac]             synchronized (mapRectangleList) {
    [javac]                          ^
    [javac]     (see http://errorprone.info/bugpattern/SynchronizeOnNonFinalField)
    [javac] /home/simon/src/josm.svngit/src/org/openstreetmap/gui/jmapviewer/JMapViewer.java:334: warning: [SynchronizeOnNonFinalField] Synchronizing on non-final fields is not safe: if the field is ever updated, different threads may end up locking on different objects.
    [javac]             synchronized (mapPolygonList) {
    [javac]                          ^
    [javac]     (see http://errorprone.info/bugpattern/SynchronizeOnNonFinalField)
    [javac] /home/simon/src/josm.svngit/src/org/openstreetmap/gui/jmapviewer/JMapViewer.java:653: warning: [SynchronizeOnNonFinalField] Synchronizing on non-final fields is not safe: if the field is ever updated, different threads may end up locking on different objects.
    [javac]             synchronized (mapPolygonList) {
    [javac]                          ^
    [javac]     (see http://errorprone.info/bugpattern/SynchronizeOnNonFinalField)
    [javac] /home/simon/src/josm.svngit/src/org/openstreetmap/gui/jmapviewer/JMapViewer.java:662: warning: [SynchronizeOnNonFinalField] Synchronizing on non-final fields is not safe: if the field is ever updated, different threads may end up locking on different objects.
    [javac]             synchronized (mapRectangleList) {
    [javac]                          ^
    [javac]     (see http://errorprone.info/bugpattern/SynchronizeOnNonFinalField)
    [javac] /home/simon/src/josm.svngit/src/org/openstreetmap/gui/jmapviewer/JMapViewer.java:671: warning: [SynchronizeOnNonFinalField] Synchronizing on non-final fields is not safe: if the field is ever updated, different threads may end up locking on different objects.
    [javac]             synchronized (mapMarkerList) {
    [javac]                          ^
    [javac]     (see http://errorprone.info/bugpattern/SynchronizeOnNonFinalField)
    [javac] 6 warnings
    [javac] Compiling 1290 source files to /home/simon/src/josm.svngit/build
    [javac] /home/simon/src/josm.svngit/src/org/openstreetmap/josm/data/osm/Storage.java:414: warning: [NonAtomicVolatileUpdate] This update of a volatile variable is non-atomic
    [javac]             modCount++;
    [javac]                     ^
    [javac]     (see http://errorprone.info/bugpattern/NonAtomicVolatileUpdate)
    [javac] /home/simon/src/josm.svngit/src/org/openstreetmap/josm/gui/SideButton.java:59: warning: [StringEquality] String comparison using reference equality instead of value equality
    [javac]                     if (evt.getPropertyName() == javax.swing.Action.SMALL_ICON) {
    [javac]                                               ^
    [javac]     (see http://errorprone.info/bugpattern/StringEquality)
    [javac]   Did you mean 'if (javax.swing.Action.SMALL_ICON.equals(evt.getPropertyName())) {'?
    [javac] /home/simon/src/josm.svngit/src/org/openstreetmap/josm/io/NmeaReader.java:54: warning: [NonOverridingEquals] equals method doesn't override Object.equals; enum instances can safely be compared by reference equality, so please delete this
    [javac]         public boolean equals(String type) {
    [javac]                        ^
    [javac]     (see http://errorprone.info/bugpattern/NonOverridingEquals)
    [javac]   Did you mean to remove this line?
    [javac] 3 warnings
     [copy] Copying 1 file to /home/simon/src/josm.svngit/build

To integrate it, the following modifications on build.xml are required:

  • build.xml

    diff --git a/build.xml b/build.xml
    index bd7e95c..019fd16 100644
    a b Build-Date: ${build.tstamp} 
    225225            <exclude name="org/apache/commons/logging/impl/ServletContextCleaner.java"/>
    226226        </javac>
    227227        <!-- JMapViewer -->
    228         <javac sourcepath="" srcdir="${src.dir}" excludes="com/**,oauth/**,org/apache/commons/**,org/glassfish/**,org/openstreetmap/gui/jmapviewer/Demo.java,org/openstreetmap/josm/**,JOSM.java,gnu/**"
     228        <javac compiler="com.google.errorprone.ErrorProneAntCompilerAdapter"
     229            sourcepath="" srcdir="${src.dir}" excludes="com/**,oauth/**,org/apache/commons/**,org/glassfish/**,org/openstreetmap/gui/jmapviewer/Demo.java,org/openstreetmap/josm/**,JOSM.java,gnu/**"
    229230            destdir="build" target="1.7" source="1.7" debug="on" includeantruntime="false" createMissingPackageInfoClass="false" encoding="UTF-8">
     231            <compilerclasspath>
     232              <pathelement location="./error_prone_ant-2.0.8.jar"/>
     233            </compilerclasspath>
    230234            <compilerarg value="-Xlint:cast"/>
    231235            <compilerarg value="-Xlint:deprecation"/>
    232236            <compilerarg value="-Xlint:dep-ann"/>
    Build-Date: ${build.tstamp} 
    242246            <compilerarg value="-XDignore.symbol.file"/>
    243247        </javac>
    244248        <!-- JOSM -->
    245         <javac sourcepath="" srcdir="${src.dir}" excludes="com/**,oauth/**,org/apache/commons/**,org/glassfish/**,org/openstreetmap/gui/jmapviewer/Demo.java"
     249        <javac compiler="com.google.errorprone.ErrorProneAntCompilerAdapter"
     250            sourcepath="" srcdir="${src.dir}" excludes="com/**,oauth/**,org/apache/commons/**,org/glassfish/**,org/openstreetmap/gui/jmapviewer/Demo.java"
    246251            destdir="build" target="1.7" source="1.7" debug="on" includeantruntime="false" createMissingPackageInfoClass="false" encoding="UTF-8">
     252            <compilerclasspath>
     253              <pathelement location="./error_prone_ant-2.0.8.jar"/>
     254            </compilerclasspath>
    247255            <compilerarg value="-Xlint:cast"/>
    248256            <compilerarg value="-Xlint:deprecation"/>
    249257            <compilerarg value="-Xlint:dep-ann"/>

Shall we add this to our build process?

Attachments (0)

Change History (25)

comment:1 Changed 7 years ago by simon04

In 9703/josm:

see #12472 - Fix warnings identified by error-prone

comment:2 Changed 7 years ago by Don-vip

Keywords: error prone ant added
Milestone: 16.02

I'd say yes if you put jar file in tools.

comment:3 Changed 7 years ago by simon04

Keywords: java8 added
Milestone: 16.0216.04

Nowhere documented, but it requires JDK 8.

comment:4 Changed 7 years ago by stoecker

I think these are also in Coverity. Do we need another test for the same?

comment:5 in reply to:  4 Changed 7 years ago by stoecker

Replying to stoecker:

I think these are also in Coverity. Do we need another test for the same?

No. It seems I mixed something up. These are not in Coverity.

comment:6 Changed 7 years ago by Don-vip

Even if it were, it's always better to have an autonomous offline system. We have quotas with Coverity and sometimes the website can be slow, or even unavailable.

comment:7 Changed 7 years ago by Don-vip

Milestone: 16.0416.05

comment:8 Changed 7 years ago by Don-vip

Milestone: 16.0516.06

comment:9 Changed 7 years ago by Don-vip

Milestone: 16.0616.07

comment:10 Changed 7 years ago by Don-vip

Owner: changed from team to Don-vip
Status: newassigned
Summary: Use error-prone in build[PATCH] Java 8: Use error-prone in build

comment:11 Changed 7 years ago by Don-vip

Resolution: fixed
Status: assignedclosed

In 10581/josm:

see #11390, fix #12472 - Use error-prone in build (patch by simon04, modified) - requires java 8

comment:12 Changed 7 years ago by Don-vip

In 10589/josm:

see #12472 - fix "Non-exhaustive switch" compilation warning

comment:13 Changed 7 years ago by Don-vip

In 10614/josm:

see #11390, see #12472 - Update to error-prone 2.0.11

comment:14 Changed 7 years ago by Don-vip

In 10628/josm:

see #12472 - error-prone causes problems with coverity and java 9, introduce an ant property to override it

comment:15 Changed 7 years ago by Don-vip

Summary: [PATCH] Java 8: Use error-prone in buildJava 8: Use error-prone in build

comment:16 Changed 7 years ago by Don-vip

In 10653/josm:

see #12472 - fix warning "ClassCanBeStatic"

comment:17 Changed 7 years ago by Don-vip

In 10655/josm:

see #12472 - fix warning "OperatorPrecedence"

comment:18 Changed 7 years ago by Don-vip

In 10656/josm:

see #12472 - fix warning "ReferenceEquality"

comment:19 Changed 7 years ago by Don-vip

In 10659/josm:

see #12472 - fix more warnings, increase maximum number of reported warnings from 100 to 1000

comment:20 Changed 7 years ago by Don-vip

In 10662/josm:

see #12472, fix #13230, fix #13225, fix #13228 - disable ReferenceEquality warning + partial revert of r10656 + r10659, causes too much problems

comment:21 Changed 7 years ago by Don-vip

In 10670/josm:

see #12472 - fix "OperatorPrecedence" warnings

comment:22 Changed 7 years ago by Don-vip

In 10671/josm:

see #12472 - fix "MissingCasesInEnumSwitch" warnings

comment:23 Changed 7 years ago by Don-vip

In 10672/josm:

see #12472 - fix "UnsynchronizedOverridesSynchronized" warnings

comment:24 Changed 7 years ago by Don-vip

In 10678/josm:

see #12472 - fix "OperatorPrecedence" warnings

comment:25 Changed 7 years ago by Don-vip

In 10704/josm:

see #12472 - fix build when error-prone is not used

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain Don-vip.
as The resolution will be set.
The resolution will be deleted.

Add Comment


E-mail address and name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.