Merge pull request #805 from solven-eu/fix/CTorThisCallFields

ThisEmptyInConstructor_notEmptyCtor

Author: blacelle

CHANGES.MD

@@ -2,11 +2,21 @@ This document is intended for CleanThat developers and users (e.g. through [Spot
 
 We adhere to the [keepachangelog](https://keepachangelog.com/en/1.0.0/) format
 
-## [2.18 - Unreleased]
+## [2.19 - Unreleased]
 
 ### Changes
 
 * `RemoveExplicitCallToSuper` now removes `super();` in constructor only if it is not the only statement.
+* `RemoveExplicitCallToSuper` does not remove `this()` anymore.
+
+### Fixed
+
+* `ModifierOrder` shall not prevent further modifiers mutators to operate correctly.
+
+## [2.18]
+
+### Changes
+
 * Spotless licenseHeader default YearMode has been updated to `UPDATE_TO_TODAY`, as CleanThat generally behave as if `ratchetFrom` is the base branch
 * `UnnecessaryBoxing` is not draft anymore
 * `AvoidMultipleUnaryOperators` is not draft anymore

java/src/main/java/eu/solven/cleanthat/engine/java/refactorer/AJavaparserAstMutator.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2023 Benoit Lacelle - SOLVEN
+ * Copyright 2023-2024 Benoit Lacelle - SOLVEN
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -64,8 +64,8 @@ public Optional<Node> walkAst(Node ast) {
 		var astHasMutated = new AtomicBoolean();
 
 		ast.walk(node -> {
-			boolean nodeHasMUtated = walkOneNode(node);
-			if (nodeHasMUtated) {
+			boolean nodeHasMutated = walkOneNode(node);
+			if (nodeHasMutated) {
 				astHasMutated.set(true);
 			}
 		});

java/src/main/java/eu/solven/cleanthat/engine/java/refactorer/mutators/ModifierOrder.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2023 Benoit Lacelle - SOLVEN
+ * Copyright 2023-2024 Benoit Lacelle - SOLVEN
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -15,6 +15,7 @@
  */
 package eu.solven.cleanthat.engine.java.refactorer.mutators;
 
+import java.util.ArrayList;
 import java.util.Collections;
 import java.util.Comparator;
 import java.util.List;
@@ -35,6 +36,7 @@
 import eu.solven.cleanthat.engine.java.IJdkVersionConstants;
 import eu.solven.cleanthat.engine.java.refactorer.AJavaparserNodeMutator;
 import eu.solven.cleanthat.engine.java.refactorer.NodeAndSymbolSolver;
+import eu.solven.cleanthat.engine.java.refactorer.meta.ApplyBeforeMe;
 
 /**
  * Order modifiers according the the Java specification.
@@ -43,6 +45,7 @@
  * @see https://github.com/checkstyle/checkstyle/blob/master/src/xdocs/checks/modifier/modifierorder.xml
  * @see
  */
+@ApplyBeforeMe(UnnecessaryModifier.class)
 public class ModifierOrder extends AJavaparserNodeMutator {
 	private static final Logger LOGGER = LoggerFactory.getLogger(ModifierOrder.class);
 
@@ -108,19 +111,11 @@ protected boolean processNotRecursively(NodeAndSymbolSolver<?> nodeAndContext) {
 			NodeWithModifiers<?> nodeWithModifiers = (NodeWithModifiers<?>) node;
 			NodeList<Modifier> modifiers = nodeWithModifiers.getModifiers();
 
-			NodeList<Modifier> mutableModifiers = new NodeList<>(modifiers);
+			// Do not rely on a `NodeList` to prevent transfer of modifiers ownership
+			// https://github.com/solven-eu/cleanthat/issues/802
+			List<Modifier> mutableModifiers = new ArrayList<>(modifiers);
 
-			Collections.sort(mutableModifiers, new Comparator<Modifier>() {
-
-				@Override
-				public int compare(Modifier o1, Modifier o2) {
-					return compare2(o1.getKeyword().asString(), o2.getKeyword().asString());
-				}
-
-				private int compare2(String left, String right) {
-					return Integer.compare(ORDERED_MODIFIERS.indexOf(left), ORDERED_MODIFIERS.indexOf(right));
-				}
-			});
+			Collections.sort(mutableModifiers, modifiersComparator());
 
 			var changed = areSameReferences(modifiers, mutableModifiers);
 
@@ -132,9 +127,23 @@ private int compare2(String left, String right) {
 		return false;
 	}
 
+	private Comparator<Modifier> modifiersComparator() {
+		return new Comparator<Modifier>() {
+
+			@Override
+			public int compare(Modifier o1, Modifier o2) {
+				return compare2(o1.getKeyword().asString(), o2.getKeyword().asString());
+			}
+
+			private int compare2(String left, String right) {
+				return Integer.compare(ORDERED_MODIFIERS.indexOf(left), ORDERED_MODIFIERS.indexOf(right));
+			}
+		};
+	}
+
 	private boolean applyModifiers(NodeWithModifiers<?> nodeWithModifiers,
 			NodeList<Modifier> originalModifiers,
-			NodeList<Modifier> sortedModifiers) {
+			List<Modifier> sortedModifiers) {
 		if (sortedModifiers.stream()
 				.map(m -> m.getKeyword())
 				.anyMatch(m -> m == Keyword.SEALED || m == Keyword.NON_SEALED)) {
@@ -149,13 +158,18 @@ private boolean applyModifiers(NodeWithModifiers<?> nodeWithModifiers,
 		nodeWithModifiers.setModifiers();
 
 		LOGGER.debug("We fixed the ordering of modifiers");
-		nodeWithModifiers.setModifiers(sortedModifiers);
+		NodeList<Modifier> asNodeList = new NodeList<>(sortedModifiers);
+		nodeWithModifiers.setModifiers(asNodeList);
 
 		return true;
 	}
 
 	@SuppressWarnings("PMD.CompareObjectsWithEquals")
-	private boolean areSameReferences(NodeList<Modifier> modifiers, NodeList<Modifier> mutableModifiers) {
+	private boolean areSameReferences(List<Modifier> modifiers, List<Modifier> mutableModifiers) {
+		if (modifiers.size() != mutableModifiers.size()) {
+			return false;
+		}
+
 		var changed = false;
 		for (var i = 0; i < modifiers.size(); i++) {
 			// Check by reference

java/src/main/java/eu/solven/cleanthat/engine/java/refactorer/mutators/RemoveExplicitCallToSuper.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2023 Benoit Lacelle - SOLVEN
+ * Copyright 2023-2024 Benoit Lacelle - SOLVEN
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -19,6 +19,7 @@
 import java.util.Set;
 
 import com.github.javaparser.ast.body.ConstructorDeclaration;
+import com.github.javaparser.ast.stmt.ExplicitConstructorInvocationStmt;
 import com.google.common.collect.ImmutableSet;
 
 import eu.solven.cleanthat.engine.java.IJdkVersionConstants;
@@ -76,21 +77,27 @@ protected boolean processNotRecursively(NodeAndSymbolSolver<?> node) {
 
 		if (body.getStatements().isEmpty()) {
 			return false;
+		} else if (body.getStatements().size() == 1) {
+			// We keep `super();` if it is the only statement, as it is generally considered better than having an empty
+			// constructor.
+			return false;
 		}
 
 		var firstStatement = body.getStatement(0);
 
 		if (!firstStatement.isExplicitConstructorInvocationStmt()) {
+			// We need either `this(...)` or `super(...)`
 			return false;
 		}
 
-		if (!firstStatement.asExplicitConstructorInvocationStmt().getArguments().isEmpty()) {
+		ExplicitConstructorInvocationStmt asCtorInvocationStmt = firstStatement.asExplicitConstructorInvocationStmt();
+		if (!asCtorInvocationStmt.getArguments().isEmpty()) {
+			// We would remove only a ref to the default constructor
 			return false;
 		}
 
-		if (body.getStatements().size() == 1) {
-			// We keep `super();` if it is the only statement, as it is generally considered better than having an empty
-			// constructor.
+		if (asCtorInvocationStmt.isThis()) {
+			// `this(...)` can not be turned into explicit invocations
 			return false;
 		}
 

java/src/main/java/eu/solven/cleanthat/engine/java/refactorer/mutators/UnnecessaryModifier.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2023 Benoit Lacelle - SOLVEN
+ * Copyright 2023-2024 Benoit Lacelle - SOLVEN
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -20,6 +20,7 @@
 
 import com.github.javaparser.ast.Modifier;
 import com.github.javaparser.ast.Modifier.Keyword;
+import com.github.javaparser.ast.Node;
 import com.github.javaparser.ast.body.AnnotationDeclaration;
 import com.github.javaparser.ast.body.AnnotationMemberDeclaration;
 import com.github.javaparser.ast.body.ClassOrInterfaceDeclaration;
@@ -90,11 +91,12 @@ public String jSparrowUrl() {
 
 	@SuppressWarnings({ "PMD.CognitiveComplexity", "PMD.NPathComplexity" })
 	@Override
-	protected boolean processNotRecursively(NodeAndSymbolSolver<?> node) {
-		if (!(node.getNode() instanceof Modifier)) {
+	protected boolean processNotRecursively(NodeAndSymbolSolver<?> nodeAndSymbolSolver) {
+		Node node = nodeAndSymbolSolver.getNode();
+		if (!(node instanceof Modifier)) {
 			return false;
 		}
-		var modifier = (Modifier) node.getNode();
+		var modifier = (Modifier) node;
 
 		if (modifier.getParentNode().isEmpty()) {
 			return false;

java/src/test/java/eu/solven/cleanthat/engine/java/refactorer/cases/do_not_format_me/TestModifierOrderThenUnnecessaryModifierCases.java

@@ -0,0 +1,27 @@
+package eu.solven.cleanthat.engine.java.refactorer.cases.do_not_format_me;
+
+import java.util.Arrays;
+
+import eu.solven.cleanthat.engine.java.refactorer.annotations.CompareCompilationUnitsAsStrings;
+import eu.solven.cleanthat.engine.java.refactorer.meta.CompositeJavaparserMutator;
+import eu.solven.cleanthat.engine.java.refactorer.meta.IJavaparserAstMutator;
+import eu.solven.cleanthat.engine.java.refactorer.mutators.ModifierOrder;
+import eu.solven.cleanthat.engine.java.refactorer.mutators.UnnecessaryModifier;
+import eu.solven.cleanthat.engine.java.refactorer.test.AJavaparserRefactorerCases;
+
+// https://github.com/solven-eu/cleanthat/issues/802
+public class TestModifierOrderThenUnnecessaryModifierCases extends AJavaparserRefactorerCases {
+	@Override
+	public IJavaparserAstMutator getTransformer() {
+		return new CompositeJavaparserMutator(Arrays.asList(
+				 new ModifierOrder(), 
+				new UnnecessaryModifier()));
+	}
+
+	// https://github.com/solven-eu/cleanthat/issues/802
+	@CompareCompilationUnitsAsStrings(pre = "interface TopLevelInterface { public  final static int i = 0; }",
+			post = "interface TopLevelInterface {\n" + "\n" + "    int i = 0;\n" + "}\n")
+	public static class Issue802 {
+	}
+
+}

java/src/test/java/eu/solven/cleanthat/engine/java/refactorer/cases/do_not_format_me/TestRemoveExplicitCallToSuperCases.java

@@ -90,7 +90,8 @@ public Post() {
 		}
 	}
 
-	@CompareInnerClasses
+	// `this();` can be removed for equivalent behavior only because `Pre()` exists but is a noop
+	@UnmodifiedInnerClass
 	public static class ThisEmptyInConstructor {
 		public class Pre {
 
@@ -103,12 +104,66 @@ public Pre(String s) {
 				s.toString();
 			}
 		}
+	}
 
-		public class Post {
+	// Dropping `this` would lead to compilation error as `s` in final
+	@UnmodifiedInnerClass
+	public static class ThisEmptyInConstructor_notEmptyCtor_finalField {
+		public class Pre {
+			final String s;
 
-			public Post() {
+			public Pre() {
+				s = "";
 			}
 
+			public Pre(String s) {
+				this();
+
+				s.toString();
+			}
+		}
+	}
+
+	// Dropping `this` would prevent `s` initialization
+	@UnmodifiedInnerClass
+	public static class ThisEmptyInConstructor_notEmptyCtor_notFinalField {
+		public class Pre {
+			String s;
+
+			public Pre() {
+				s = "";
+			}
+
+			public Pre(String s) {
+				this();
+
+				s.toString();
+			}
+		}
+	}
+
+	@CompareInnerClasses
+	public static class SuperEmptyInConstructor_notEmptyCtor {
+		class Parent {
+			final String s;
+
+			public Parent() {
+				s = "";
+			}
+
+		}
+
+		public class Pre extends Parent {
+
+			public Pre(String s) {
+				super();
+
+				s.toString();
+			}
+		}
+
+		public class Post extends Parent {
+
 			public Post(String s) {
 
 				s.toString();

java/src/test/java/eu/solven/cleanthat/engine/java/refactorer/cases/do_not_format_me/TestUnnecessaryModifierCases.java

@@ -1,5 +1,6 @@
 package eu.solven.cleanthat.engine.java.refactorer.cases.do_not_format_me;
 
+import eu.solven.cleanthat.engine.java.refactorer.annotations.CompareCompilationUnitsAsStrings;
 import eu.solven.cleanthat.engine.java.refactorer.annotations.CompareInnerAnnotations;
 import eu.solven.cleanthat.engine.java.refactorer.annotations.CompareInnerClasses;
 import eu.solven.cleanthat.engine.java.refactorer.meta.IJavaparserAstMutator;

parent/pom.xml

@@ -378,7 +378,10 @@
 					<version>${spotless.version}</version>
 					<configuration>
 						<!-- optional: limit format enforcement to just the files changed by this feature branch -->
-						<!--ratchetFrom>origin/master</ratchetFrom-->
+						<!-- We keep a ratchetFrom to ensure `LicenseHeader.YearMode==UPDATE_TO_TODAY` -->
+						<!-- https://github.com/diffplug/spotless/blob/main/plugin-maven/src/main/java/com/diffplug/spotless/maven/generic/LicenseHeader.java -->
+						<ratchetFrom>origin/master</ratchetFrom>
+
 						<formats>
 							<!-- you can define as many formats as you want, each is independent -->
 							<format>
@@ -435,11 +438,14 @@
 								<version>${project.version}</version>
 								<mutators>
 									<mutator>SafeAndConsensual</mutator>
+									-
 									<mutator>SafeButNotConsensual</mutator>
+									-
 									<mutator>SafeButControversial</mutator>
+									-
 									<mutator>Guava</mutator>
 								</mutators>
-								<includeDraft>true</includeDraft>
+								<includeDraft>false</includeDraft>
 								<excludedMutators>
 									<!-- https://github.com/javaparser/javaparser/issues/3930 -->
 									<excludedMutator>LambdaReturnsSingleStatement</excludedMutator>