typehandlerlibrary qa, jdrueckert feedback

especially take out pmd guardlog suppresses. fix part of them
in a catch even if not necessary from a performance point of
view. and create a pull request towards teraconfig to not
use the guardlog rule any more:
MovingBlocks/TeraConfig#23

Co-authored-by: jdrueckert <jd.rueckert@googlemail.com>

build-logic/src/main/kotlin/terasology-metrics.gradle.kts

@@ -21,8 +21,9 @@ dependencies {
     // spotbugs annotations to suppress warnings are not included via spotbugs plugin
     // see bug: https://github.com/spotbugs/spotbugs-gradle-plugin/issues/1018
     compileOnly("com.github.spotbugs:spotbugs-annotations:4.8.0")
-    pmd("net.sourceforge.pmd:pmd-core:6.55.0")
-    pmd("net.sourceforge.pmd:pmd-java:6.55.0")
+    pmd("net.sourceforge.pmd:pmd-ant:7.0.0-rc4")
+    pmd("net.sourceforge.pmd:pmd-core:7.0.0-rc4")
+    pmd("net.sourceforge.pmd:pmd-java:7.0.0-rc4")
 
     testRuntimeOnly("ch.qos.logback:logback-classic:1.2.11") {
         because("runtime: to configure logging during tests with logback.xml")

subsystems/TypeHandlerLibrary/src/main/java/org/terasology/persistence/serializers/Serializer.java

@@ -22,9 +22,6 @@
  * methods that use {@link #serializeToPersisted(Object, TypeInfo)} and {@link #deserializeFromPersisted(PersistedData,
  * TypeInfo)}.
  */
-// log statements after an if are marked as false positive, suppress.
-// see pmd bug: https://github.com/pmd/pmd/issues/4731
-@SuppressWarnings("PMD.GuardLogStatementJavaUtil")
 public final class Serializer<D extends PersistedData> {
 
     private static final Logger logger = LoggerFactory.getLogger(Serializer.class);
@@ -87,7 +84,7 @@ public <T> Optional<T> deserialize(TypeInfo<T> type, InputStream inputStream) {
             D persistedData = reader.read(inputStream);
             return deserializeFromPersisted(persistedData, type);
         } catch (IOException e) {
-            logger.error("Cannot deserialize type [" + type + "]", e);
+            logger.error("Cannot deserialize type [{}]", type, e);
         }
         return Optional.empty();
     }
@@ -105,7 +102,7 @@ public <T> Optional<T> deserialize(TypeInfo<T> type, byte[] bytes) {
             D persistedData = reader.read(bytes);
             return deserializeFromPersisted(persistedData, type);
         } catch (IOException e) {
-            logger.error("Cannot deserialize type [" + type + "]", e);
+            logger.error("Cannot deserialize type [{}]", type, e);
         }
         return Optional.empty();
     }
@@ -143,7 +140,7 @@ public <T> void serialize(T object, TypeInfo<T> type, OutputStream outputStream)
             try {
                 writer.writeTo(persistedData.get(), outputStream);
             } catch (IOException e) {
-                logger.error("Cannot serialize [" + type + "]", e);
+                logger.error("Cannot serialize [{}]", type, e);
             }
         } else {
             logger.error("Cannot serialize [{}]", type);

subsystems/TypeHandlerLibrary/src/main/java/org/terasology/persistence/typeHandling/Serializer.java

@@ -14,7 +14,6 @@
  * A serializer provides low-level serialization support for a type, using a mapping of type handlers for each field of that type.
  *
  */
-@SuppressWarnings("PMD.GuardLogStatementJavaUtil")
 public class Serializer {
 
     private static final Logger logger = LoggerFactory.getLogger(Serializer.class);

subsystems/TypeHandlerLibrary/src/main/java/org/terasology/persistence/typeHandling/TypeHandlerLibrary.java

@@ -310,9 +310,6 @@ public <T> TypeHandler<T> getBaseTypeHandler(TypeInfo<T> typeInfo) {
         return new RuntimeDelegatingTypeHandler<>(delegateHandler, typeInfo, context);
     }
 
-    // log after else is false positive, suppress.
-    // see bug: https://github.com/pmd/pmd/issues/4731
-    @SuppressWarnings("PMD.GuardLogStatementJavaUtil")
     private Map<FieldMetadata<?, ?>, TypeHandler> getFieldHandlerMap(ClassMetadata<?, ?> type) {
         Map<FieldMetadata<?, ?>, TypeHandler> handlerMap = Maps.newHashMap();
         for (FieldMetadata<?, ?> field : type.getFields()) {

subsystems/TypeHandlerLibrary/src/main/java/org/terasology/persistence/typeHandling/coreTypes/EnumTypeHandler.java

@@ -31,9 +31,6 @@ public PersistedData serializeNonNull(T value, PersistedDataSerializer serialize
         return serializer.serialize(value.toString());
     }
 
-    // log after else is false positive, suppress.
-    // see bug: https://github.com/pmd/pmd/issues/4731
-    @SuppressWarnings("PMD.GuardLogStatementJavaUtil")
     @Override
     public Optional<T> deserialize(PersistedData data) {
         if (data.isString()) {

subsystems/TypeHandlerLibrary/src/main/java/org/terasology/persistence/typeHandling/coreTypes/GenericMapTypeHandler.java

@@ -60,7 +60,6 @@ private PersistedData serializeEntry(Map.Entry<K, V> entry, PersistedDataSeriali
         return serializer.serialize(result);
     }
 
-    @SuppressWarnings("PMD.GuardLogStatementJavaUtil")
     @Override
     public Optional<Map<K, V>> deserialize(PersistedData data) {
 

subsystems/TypeHandlerLibrary/src/main/java/org/terasology/persistence/typeHandling/coreTypes/ObjectFieldMapTypeHandler.java

@@ -80,9 +80,6 @@ private String getFieldName(Field field) {
         return serializedName.value();
     }
 
-    // log after else is false positive, suppress.
-    // see bug: https://github.com/pmd/pmd/issues/4731
-    @SuppressWarnings("PMD.GuardLogStatementJavaUtil")
     @Override
     public Optional<T> deserialize(PersistedData data) {
         if (!data.isValueMap()) {

subsystems/TypeHandlerLibrary/src/main/java/org/terasology/persistence/typeHandling/coreTypes/factories/CollectionTypeHandlerFactory.java

@@ -24,6 +24,7 @@
 public class CollectionTypeHandlerFactory implements TypeHandlerFactory {
     private static final Logger LOGGER = LoggerFactory.getLogger(CollectionTypeHandlerFactory.class);
 
+    // PMD and intellij cannot judge if this is good or not. suppress.
     @SuppressWarnings("PMD.UnusedLocalVariable") private ConstructorLibrary constructorLibrary;
 
     public CollectionTypeHandlerFactory(ConstructorLibrary constructorLibrary) {

subsystems/TypeHandlerLibrary/src/test/java/org/terasology/persistence/typeHandling/FutureTypeHandlerTest.java

@@ -65,7 +65,10 @@ private RecursiveType(T data, RecursiveType<T>... children) {
         }
     }
 
-    @edu.umd.cs.findbugs.annotations.SuppressFBWarnings
+    // the test verfies that bot, static class (above), and class work. with this typehandler.
+    // sputbugs suggests to optimize the class to static. doing so makes the test fail, and
+    // defeats the purpose of the test in general, so suppress.
+    @edu.umd.cs.findbugs.annotations.SuppressFBWarnings(value = "SIC_INNER_SHOULD_BE_STATIC", justification = "Test code is not performance-relevant, flagged inner ResultCaptor class is a mock with dynamic behavior and cannot be static.")
     private class ResultCaptor<T> implements Answer {
         private T result = null;
         public T getResult() {

subsystems/TypeHandlerLibrary/src/test/java/org/terasology/persistence/typeHandling/InMemorySerializerTest.java

@@ -37,10 +37,6 @@
 import java.util.function.Function;
 import java.util.stream.Stream;
 
-
-// spotbugs does thinks Assertions.assertThrows does not throw the exception, even if
-// test is successful. see bug: https://github.com/spotbugs/spotbugs/issues/2667.
-@edu.umd.cs.findbugs.annotations.SuppressFBWarnings
 class InMemorySerializerTest {
     private final InMemoryPersistedDataSerializer serializer = new InMemoryPersistedDataSerializer();
 

subsystems/TypeHandlerLibrary/src/test/java/org/terasology/persistence/typeHandling/coreTypes/factories/ObjectFieldMapTypeHandlerFactoryTest.java

@@ -20,10 +20,6 @@
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.verify;
 
-// both, pmd and spotbugs complain about not used private fields, suppress in
-// the static test class, but fields are checked. suppress.
-@edu.umd.cs.findbugs.annotations.SuppressFBWarnings
-@SuppressWarnings("PMD.UnusedPrivateField")
 public class ObjectFieldMapTypeHandlerFactoryTest {
     private final TypeHandlerLibrary typeHandlerLibrary = mock(TypeHandlerLibrary.class);
 
@@ -48,6 +44,9 @@ public void testObject() {
         verify(typeHandlerLibrary).getTypeHandler(eq(new TypeInfo<List<Integer>>() { }.getType()));
     }
 
+    // the minimal test case leads to a warning of unused private fields, in both spotbugs and pmd, suppress.
+    @edu.umd.cs.findbugs.annotations.SuppressFBWarnings(value = "UUF_UNUSED_FIELD", justification = "Direct member access is not expected. TypeHandler factory dynamically loads type handlers on type handler creation based on member types of input class TypeInfo. ")
+    @SuppressWarnings("PMD.UnusedPrivateField")
     private static class SomeClass<T> {
         private T t;
         private List<T> list;