diff --git a/its/ruling/src/test/resources/expected/eclipse-jetty-similar-to-main/java-S9434.json b/its/ruling/src/test/resources/expected/eclipse-jetty-similar-to-main/java-S9434.json new file mode 100644 index 00000000000..2d341fe52d4 --- /dev/null +++ b/its/ruling/src/test/resources/expected/eclipse-jetty-similar-to-main/java-S9434.json @@ -0,0 +1,9 @@ +{ +"eclipse-jetty:jetty-io/src/main/java/org/eclipse/jetty/io/ssl/SslConnection.java": [ +371, +380 +], +"eclipse-jetty:jetty-server/src/main/java/org/eclipse/jetty/server/HttpChannel.java": [ +370 +] +} diff --git a/its/ruling/src/test/resources/expected/eclipse-jetty/java-S9434.json b/its/ruling/src/test/resources/expected/eclipse-jetty/java-S9434.json new file mode 100644 index 00000000000..0514eb02cca --- /dev/null +++ b/its/ruling/src/test/resources/expected/eclipse-jetty/java-S9434.json @@ -0,0 +1,20 @@ +{ +"eclipse-jetty:jetty-io/src/main/java/org/eclipse/jetty/io/ssl/SslConnection.java": [ +371, +380 +], +"eclipse-jetty:jetty-server/src/main/java/org/eclipse/jetty/server/HttpChannel.java": [ +370 +], +"eclipse-jetty:jetty-util/src/main/java/org/eclipse/jetty/util/RolloverFileOutputStream.java": [ +410, +411, +412 +], +"eclipse-jetty:jetty-util/src/main/java/org/eclipse/jetty/util/ssl/SslSelectionDump.java": [ +46 +], +"eclipse-jetty:jetty-util/src/main/java/org/eclipse/jetty/util/thread/ScheduledExecutorScheduler.java": [ +99 +] +} diff --git a/its/ruling/src/test/resources/expected/guava/java-S9434.json b/its/ruling/src/test/resources/expected/guava/java-S9434.json new file mode 100644 index 00000000000..72964a4892c --- /dev/null +++ b/its/ruling/src/test/resources/expected/guava/java-S9434.json @@ -0,0 +1,5 @@ +{ +"guava:src/com/google/common/collect/TreeRangeMap.java": [ +547 +] +} diff --git a/java-checks-test-sources/default/src/main/java/checks/RedundantQualifiedThisCheckSample.java b/java-checks-test-sources/default/src/main/java/checks/RedundantQualifiedThisCheckSample.java new file mode 100644 index 00000000000..affc4891785 --- /dev/null +++ b/java-checks-test-sources/default/src/main/java/checks/RedundantQualifiedThisCheckSample.java @@ -0,0 +1,147 @@ +package checks; + +import java.util.function.Supplier; + +public class RedundantQualifiedThisCheckSample { + + private final String id = "id"; + private int counter; + + String redundantInMethod() { + return RedundantQualifiedThisCheckSample.this.id; // Noncompliant {{Remove this redundant qualifier.}} +// ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + } + + void redundantOnMethodCall() { + RedundantQualifiedThisCheckSample.this.increment(); // Noncompliant +// ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + } + + RedundantQualifiedThisCheckSample redundantWithoutMemberAccess() { + return RedundantQualifiedThisCheckSample.this; // Noncompliant +// ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + } + + Supplier redundantInLambda() { + return () -> RedundantQualifiedThisCheckSample.this.id; // Noncompliant +// ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + } + + Supplier> redundantInNestedLambda() { + return () -> () -> RedundantQualifiedThisCheckSample.this.id; // Noncompliant +// ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + } + + void increment() { + counter++; + } + + String compliantExplicitThis() { + return this.id; + } + + String compliantImplicitThis() { + return id; + } + + class Inner { + private final String id = "inner"; + + String outerQualifierIsRequired() { + return RedundantQualifiedThisCheckSample.this.id; + } + + String ownQualifierIsRedundant() { + return Inner.this.id; // Noncompliant +// ^^^^^^^^^^ + } + + Supplier outerQualifierIsRequiredInLambda() { + return () -> RedundantQualifiedThisCheckSample.this.id; + } + + Supplier ownQualifierIsRedundantInLambda() { + return () -> Inner.this.id; // Noncompliant +// ^^^^^^^^^^ + } + + class Innermost { + String enclosingQualifiersAreRequired() { + return RedundantQualifiedThisCheckSample.this.id + Inner.this.id; + } + + String ownQualifierIsRedundant() { + return Innermost.this.toString(); // Noncompliant +// ^^^^^^^^^^^^^^ + } + } + } + + Supplier anonymousClassNeedsQualifier() { + return new Supplier() { + @Override + public String get() { + return RedundantQualifiedThisCheckSample.this.id; + } + }; + } + + Supplier lambdaInAnonymousClassNeedsQualifier() { + return new Supplier() { + @Override + public String get() { + Supplier nested = () -> RedundantQualifiedThisCheckSample.this.id; + return nested.get(); + } + }; + } + + String localClassNeedsQualifier() { + class Local { + String get() { + return RedundantQualifiedThisCheckSample.this.id; + } + } + return new Local().get(); + } + + String localClassOwnQualifierIsRedundant() { + class Local { + String get() { + return Local.this.toString(); // Noncompliant +// ^^^^^^^^^^ + } + } + return new Local().get(); + } + + static class StaticNested { + private final String id = "nested"; + + String ownQualifierIsRedundant() { + return StaticNested.this.id; // Noncompliant +// ^^^^^^^^^^^^^^^^^ + } + + Supplier ownQualifierIsRedundantInLambda() { + return () -> StaticNested.this.id; // Noncompliant +// ^^^^^^^^^^^^^^^^^ + } + } + + enum Status { + OPEN; + + String ownQualifierIsRedundant() { + return Status.this.name(); // Noncompliant +// ^^^^^^^^^^^ + } + } + + record Point(int x, int y) { + int ownQualifierIsRedundant() { + return Point.this.x; // Noncompliant +// ^^^^^^^^^^ + } + } +} diff --git a/java-checks/src/main/java/org/sonar/java/checks/RedundantQualifiedThisCheck.java b/java-checks/src/main/java/org/sonar/java/checks/RedundantQualifiedThisCheck.java new file mode 100644 index 00000000000..17bd6ea69a6 --- /dev/null +++ b/java-checks/src/main/java/org/sonar/java/checks/RedundantQualifiedThisCheck.java @@ -0,0 +1,67 @@ +/* + * SonarQube Java + * Copyright (C) SonarSource Sàrl + * mailto:info AT sonarsource DOT com + * + * You can redistribute and/or modify this program under the terms of + * the Sonar Source-Available License Version 1, as published by SonarSource Sàrl. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. + * See the Sonar Source-Available License for more details. + * + * You should have received a copy of the Sonar Source-Available License + * along with this program; if not, see https://sonarsource.com/license/ssal/ + */ +package org.sonar.java.checks; + +import java.util.List; +import javax.annotation.CheckForNull; +import org.sonar.check.Rule; +import org.sonar.java.model.ExpressionUtils; +import org.sonar.plugins.java.api.IssuableSubscriptionVisitor; +import org.sonar.plugins.java.api.semantic.Symbol; +import org.sonar.plugins.java.api.tree.ClassTree; +import org.sonar.plugins.java.api.tree.MemberSelectExpressionTree; +import org.sonar.plugins.java.api.tree.Tree; + +@Rule(key = "S9434") +public class RedundantQualifiedThisCheck extends IssuableSubscriptionVisitor { + + private static final String MESSAGE = "Remove this redundant qualifier."; + + @Override + public List nodesToVisit() { + return List.of(Tree.Kind.MEMBER_SELECT); + } + + @Override + public void visitNode(Tree tree) { + MemberSelectExpressionTree memberSelect = (MemberSelectExpressionTree) tree; + if (!ExpressionUtils.isThis(memberSelect.identifier())) { + return; + } + Symbol qualifier = memberSelect.expression().symbolType().symbol(); + if (!qualifier.isUnknown() && qualifier.equals(innermostEnclosingClass(memberSelect))) { + reportIssue(memberSelect, MESSAGE); + } + } + + /** + * Returns the symbol of the class whose instance an unqualified {@code this} would denote at the given position. + * Lambda bodies are transparent because a lambda does not introduce its own {@code this}, while anonymous and + * local classes do and are therefore returned like any other class. + */ + @CheckForNull + private static Symbol innermostEnclosingClass(Tree tree) { + Tree parent = tree.parent(); + while (parent != null) { + if (parent instanceof ClassTree classTree) { + return classTree.symbol(); + } + parent = parent.parent(); + } + return null; + } +} diff --git a/java-checks/src/test/java/org/sonar/java/checks/RedundantQualifiedThisCheckTest.java b/java-checks/src/test/java/org/sonar/java/checks/RedundantQualifiedThisCheckTest.java new file mode 100644 index 00000000000..1f8984bed02 --- /dev/null +++ b/java-checks/src/test/java/org/sonar/java/checks/RedundantQualifiedThisCheckTest.java @@ -0,0 +1,45 @@ +/* + * SonarQube Java + * Copyright (C) SonarSource Sàrl + * mailto:info AT sonarsource DOT com + * + * You can redistribute and/or modify this program under the terms of + * the Sonar Source-Available License Version 1, as published by SonarSource Sàrl. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. + * See the Sonar Source-Available License for more details. + * + * You should have received a copy of the Sonar Source-Available License + * along with this program; if not, see https://sonarsource.com/license/ssal/ + */ +package org.sonar.java.checks; + +import org.junit.jupiter.api.Test; +import org.sonar.java.checks.verifier.CheckVerifier; + +import static org.sonar.java.checks.verifier.TestUtils.mainCodeSourcesPath; + +class RedundantQualifiedThisCheckTest { + + private static final String SAMPLE = "checks/RedundantQualifiedThisCheckSample.java"; + private static final RedundantQualifiedThisCheck CHECK = new RedundantQualifiedThisCheck(); + + @Test + void test() { + CheckVerifier.newVerifier() + .onFile(mainCodeSourcesPath(SAMPLE)) + .withCheck(CHECK) + .verifyIssues(); + } + + @Test + void test_without_semantic() { + CheckVerifier.newVerifier() + .onFile(mainCodeSourcesPath(SAMPLE)) + .withCheck(CHECK) + .withoutSemantic() + .verifyIssues(); + } +} diff --git a/sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S9434.html b/sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S9434.html new file mode 100644 index 00000000000..29fc4931079 --- /dev/null +++ b/sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S9434.html @@ -0,0 +1,78 @@ +

Why is this an issue?

+

A qualified this expression, written Outer.this, is the Java syntax for reaching the instance of an enclosing class from +inside an inner class. It is only needed when a plain this would refer to something else.

+

When the qualifier names the class that the expression is already lexically inside, and no inner class sits between the expression and that class, +Outer.this and this denote the very same object. The qualifier therefore adds no information.

+

This hurts readability in two ways. The expression is longer than it needs to be, and, more importantly, it tells the reader that an enclosing +instance is involved when none is. Anyone reading the code has to look for a surrounding class to understand which object is being accessed, only to +discover that it is the obvious one.

+

Lambdas are the case developers most often get wrong. A lambda body does not introduce its own this: inside a lambda, +this is still the instance of the class where the lambda is written. A qualified this in a lambda declared directly in the +class it names is just as redundant as one written in a method body.

+

To fix an issue, delete the qualifier and keep the plain this. The expression keeps referring to the same object, so the change cannot +alter behavior.

+

Exceptions

+

No issue is raised when the qualifier is required to disambiguate. Inside an anonymous class or a named inner class, plain this refers +to the instance of that inner class, so the qualified form is the only way to reach the enclosing instance:

+
+public class Invoice {
+  private String id;
+
+  public Runnable renamer(String newId) {
+    return new Runnable() {
+      @Override
+      public void run() {
+        Invoice.this.id = newId; // Compliant: "this" would be the Runnable
+      }
+    };
+  }
+}
+
+

Removing the qualifier is therefore safe only when the enclosing class is the innermost class around the expression. If an issue is raised on a +qualified this that does resolve to a different object than a plain this would, the finding is a false positive and worth +reporting rather than silencing.

+

How to fix it

+

Remove the class name and the dot in front of this, leaving a plain this. If the member access is not ambiguous either, +for example because no local variable or parameter shadows the field, you may drop this as well, but keeping it is enough to resolve the +issue.

+

Code examples

+

Noncompliant code example

+
+public class Invoice {
+  private String id;
+  private List<String> lines = new ArrayList<>();
+
+  public String describe() {
+    return "Invoice " + Invoice.this.id; // Noncompliant: "Invoice.this" is the same object as "this"
+  }
+
+  public List<String> nonEmptyLines() {
+    return Invoice.this.lines.stream() // Noncompliant: "Invoice.this" is the same object as "this"
+      .filter(line -> !line.isEmpty() && line.startsWith(Invoice.this.id)) // Noncompliant: a lambda does not introduce its own "this"
+      .collect(Collectors.toList());
+  }
+}
+
+

Compliant solution

+
+public class Invoice {
+  private String id;
+  private List<String> lines = new ArrayList<>();
+
+  public String describe() {
+    return "Invoice " + this.id;
+  }
+
+  public List<String> nonEmptyLines() {
+    return this.lines.stream()
+      .filter(line -> !line.isEmpty() && line.startsWith(this.id))
+      .collect(Collectors.toList());
+  }
+}
+
+

Resources

+

Documentation

+ + diff --git a/sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S9434.json b/sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S9434.json new file mode 100644 index 00000000000..781490e36eb --- /dev/null +++ b/sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S9434.json @@ -0,0 +1,23 @@ +{ + "title": "Qualified \"this\" should not be used when the unqualified form is enough", + "type": "CODE_SMELL", + "code": { + "impacts": { + "MAINTAINABILITY": "LOW" + }, + "attribute": "CLEAR" + }, + "status": "ready", + "remediation": { + "func": "Constant\/Issue", + "constantCost": "2min" + }, + "tags": [ + "clumsy" + ], + "defaultSeverity": "Minor", + "ruleSpecification": "RSPEC-9434", + "sqKey": "S9434", + "scope": "All", + "quickfix": "unknown" +} diff --git a/sonar-java-plugin/src/main/resources/profiles/Sonar_way/S9434 b/sonar-java-plugin/src/main/resources/profiles/Sonar_way/S9434 new file mode 100644 index 00000000000..e69de29bb2d