diff --git a/its/ruling/src/test/resources/expected/commons-beanutils/java-S1695.json b/its/ruling/src/test/resources/expected/commons-beanutils/java-S1695.json index 4cb0bc96213..c8b28e7aea3 100644 --- a/its/ruling/src/test/resources/expected/commons-beanutils/java-S1695.json +++ b/its/ruling/src/test/resources/expected/commons-beanutils/java-S1695.json @@ -1,16 +1,4 @@ { -"commons-beanutils:src/main/java/org/apache/commons/beanutils2/BasicDynaBean.java": [ -121, -202, -234, -276, -307, -343, -385 -], -"commons-beanutils:src/main/java/org/apache/commons/beanutils2/LazyDynaBean.java": [ -501 -], "commons-beanutils:src/main/java/org/apache/commons/beanutils2/ResultSetDynaClass.java": [ 163 ], diff --git a/its/ruling/src/test/resources/expected/eclipse-jetty/java-S1695.json b/its/ruling/src/test/resources/expected/eclipse-jetty/java-S1695.json index b793951c56d..a9b2f345882 100644 --- a/its/ruling/src/test/resources/expected/eclipse-jetty/java-S1695.json +++ b/its/ruling/src/test/resources/expected/eclipse-jetty/java-S1695.json @@ -1,7 +1,4 @@ { -"eclipse-jetty:jetty-http/src/main/java/org/eclipse/jetty/http/PrecompressedHttpContent.java": [ -43 -], "eclipse-jetty:jetty-util/src/main/java/org/eclipse/jetty/util/BlockingArrayQueue.java": [ 468 ], diff --git a/its/ruling/src/test/resources/expected/guava/java-S1695.json b/its/ruling/src/test/resources/expected/guava/java-S1695.json index 3ff4ae6c89e..c30895bf790 100644 --- a/its/ruling/src/test/resources/expected/guava/java-S1695.json +++ b/its/ruling/src/test/resources/expected/guava/java-S1695.json @@ -1,20 +1,5 @@ { "guava:src/com/google/common/base/Preconditions.java": [ -212, -228, -251 -], -"guava:src/com/google/common/collect/CollectPreconditions.java": [ -31, -33 -], -"guava:src/com/google/common/collect/ImmutableMultimap.java": [ -216 -], -"guava:src/com/google/common/collect/MapMaker.java": [ -890 -], -"guava:src/com/google/common/collect/ObjectArrays.java": [ -240 +212 ] } diff --git a/java-checks-test-sources/default/src/main/java/checks/NPEThrowCheckSample.java b/java-checks-test-sources/default/src/main/java/checks/NPEThrowCheckSample.java index 44e52d6da24..4aaf03c8d4a 100644 --- a/java-checks-test-sources/default/src/main/java/checks/NPEThrowCheckSample.java +++ b/java-checks-test-sources/default/src/main/java/checks/NPEThrowCheckSample.java @@ -1,18 +1,24 @@ package checks; class NPEThrowCheckSample { - void foo() throws NullPointerException { // Noncompliant {{Throw some other exception here, such as "IllegalArgumentException".}} + void foo() throws NullPointerException { // Noncompliant {{Remove NullPointerException from the throws clause.}} // ^^^^^^^^^^^^^^^^^^^^ } void bar() { throw new - NullPointerException // Noncompliant {{Throw some other exception here, such as "IllegalArgumentException".}} + NullPointerException // Noncompliant {{Use Objects.requireNonNull() instead of explicitly throwing NullPointerException.}} (); } void baz() { - throw new java.lang.NullPointerException(); // Noncompliant {{Throw some other exception here, such as "IllegalArgumentException".}} + throw new java.lang.NullPointerException(); // Noncompliant {{Use Objects.requireNonNull() instead of explicitly throwing NullPointerException.}} // ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ } + void withMessage(Object o) { + throw new NullPointerException("o must not be null"); // Compliant, a message is provided + } + void withComputedMessage(String name) { + throw new java.lang.NullPointerException(name + " must not be null"); // Compliant + } void qix() throws IllegalArgumentException { throw new IllegalArgumentException(); } diff --git a/java-checks/src/main/java/org/sonar/java/checks/NPEThrowCheck.java b/java-checks/src/main/java/org/sonar/java/checks/NPEThrowCheck.java index 07cc5da291e..8896c28c5ac 100644 --- a/java-checks/src/main/java/org/sonar/java/checks/NPEThrowCheck.java +++ b/java-checks/src/main/java/org/sonar/java/checks/NPEThrowCheck.java @@ -40,20 +40,27 @@ public List nodesToVisit() { public void visitNode(Tree tree) { if (tree.is(Tree.Kind.THROW_STATEMENT)) { ExpressionTree expressionTree = ((ThrowStatementTree) tree).expression(); - raiseIssueOnNpe(expressionTree, expressionTree.symbolType()); + if (hasArguments(expressionTree)) { + return; + } + raiseIssueOnNpe(expressionTree, expressionTree.symbolType(), "Use Objects.requireNonNull() instead of explicitly throwing NullPointerException."); } else { for (TypeTree throwClause : ((MethodTree) tree).throwsClauses()) { - raiseIssueOnNpe(throwClause, throwClause.symbolType()); + raiseIssueOnNpe(throwClause, throwClause.symbolType(), "Remove NullPointerException from the throws clause."); } } } - private void raiseIssueOnNpe(Tree tree, Type type) { + private void raiseIssueOnNpe(Tree tree, Type type, String message) { if (type.is("java.lang.NullPointerException")) { - reportIssue(treeAtFault(tree), "Throw some other exception here, such as \"IllegalArgumentException\"."); + reportIssue(treeAtFault(tree), message); } } + private static boolean hasArguments(ExpressionTree expressionTree) { + return expressionTree.is(Tree.Kind.NEW_CLASS) && !((NewClassTree) expressionTree).arguments().isEmpty(); + } + private static Tree treeAtFault(Tree tree) { return tree.is(Tree.Kind.NEW_CLASS) ? ((NewClassTree) tree).identifier() : tree; } diff --git a/sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S1695.html b/sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S1695.html index 2d6d90b6b8d..7eb0d1fbec5 100644 --- a/sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S1695.html +++ b/sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S1695.html @@ -1,20 +1,70 @@ +

An issue is raised when code explicitly throws a NullPointerException using a throw statement.

Why is this an issue?

-

A NullPointerException should indicate that a null value was unexpectedly encountered. Good programming practice dictates -that code is structured to avoid NPE’s.

-

Explicitly throwing NullPointerException forces a method’s callers to explicitly catch it, rather than coding to avoid it. Further, it -makes it difficult to distinguish between the unexpectedly-encountered null value and the condition which causes the method to purposely -throw an NPE.

-

If an NPE is being thrown to indicate that a parameter to the method should not have been null, use the @NotNull annotation -instead.

-

Noncompliant code example

-
-public void doSomething (String aString) throws NullPointerException {
-     throw new NullPointerException();
+
Explicitly throwing a NullPointerException is almost always done to validate that a value is not null, typically with an
+if statement followed by throw new NullPointerException(…​). This is a verbose, hand-rolled way to perform a
+null check that the Java standard library already provides. The rule therefore raises an issue on every explicit throw of a
+NullPointerException, whether or not it is wrapped in a null check.

+
According to Effective Java, 3rd Edition, Item 72, while IllegalArgumentException might seem appropriate for null parameter
+validation, Java conventions actually dictate that NullPointerException should be used for null values. Rather than throwing
+it manually, you should use the standard validation utility that throws it for you: Objects.requireNonNull(), which has been available
+since Java 7. This method:

+
+
What is the potential impact?

+
Hand-written null checks reduce code maintainability and readability:

+
+
How to fix it

+
Replace manual NullPointerException throws with Objects.requireNonNull(). This method validates that an object is not
+null and throws a NullPointerException if it is, using a concise and widely recognized idiom that follows Java
+conventions.

+
Code examples

+
Noncompliant code example

+
+public class UserService {
+    private String username;
+
+    public void setUsername(String username) {
+        if (username == null) {
+            throw new NullPointerException("username must not be null"); // Noncompliant
+        }
+        this.username = username;
+    }
 }
 

-
Compliant solution

-
-public void doSomething (@NotNull String aString) {
+
Compliant solution

+
+public class UserService {
+    private String username;
+
+    public void setUsername(String username) {
+        this.username = Objects.requireNonNull(username, "username must not be null");
+    }
 }
 

+
Resources

+
Documentation

+
+
Articles & blog posts

+
+
Standards

+
 
diff --git a/sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S1695.json b/sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S1695.json
index 37567d20426..9c0de7354a8 100644
--- a/sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S1695.json
+++ b/sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S1695.json
@@ -10,9 +10,11 @@
   "status": "ready",
   "remediation": {
     "func": "Constant\/Issue",
-    "constantCost": "10min"
+    "constantCost": "5min"
   },
   "tags": [
+    "cert",
+    "convention",
     "error-handling",
     "pitfall"
   ],
@@ -20,5 +22,10 @@
   "ruleSpecification": "RSPEC-1695",
   "sqKey": "S1695",
   "scope": "Main",
-  "quickfix": "unknown"
+  "quickfix": "unknown",
+  "securityStandards": {
+    "CERT": [
+      "ERR08-J."
+    ]
+  }
 }
diff --git a/sonar-java-plugin/src/main/resources/profiles/Sonar_way/S1695 b/sonar-java-plugin/src/main/resources/profiles/Sonar_way/S1695
new file mode 100644
index 00000000000..e69de29bb2d