Fix issue S1130 Exceptions in 'throws' clauses should not be superfluous #9073

jlerbsc · 2024-03-25T13:15:03Z

The aim of this RP is to remove violations of the Sonar rule S1130 Exceptions in 'throws' clauses should not be superfluous.

Superfluous exceptions within throws clauses have negative effects on the readability and maintainability of the code. An exception in a throws clause is superfluous if it is:

listed multiple times
a subclass of another listed exception
not actually thrown by any execution path of the method (This case is not currently supported by Indepth)

These changes have been made automatically by our "Indepth" java code remediation solution, which aims to improve code quality. You can also use it periodically to remove issues that have appeared in the source code. Use of this solution is free for all open source projects. You can find all the documentation and the download link on the site https://www.indepth.fr/

MarkEWaite

I don't think that we want to remove the declared exceptions from the method declarations.

MarkEWaite · 2024-03-25T13:36:55Z

cli/src/main/java/hudson/util/QuotedStringTokenizer.java

+
+ {


I don't understand this change. The method body includes a throw new NoSuchElementException(); and yet the method declaration has removed the throws NoSuchElementException.

I also don't understand why the indentation was changed on line 286 and the blank line was inserted. Those both seem like unnecessary changes.

Hi @MarkEWaite NoSuchElementException does not need to be propagated because it inherits from the RuntimeException class. Not propagating exceptions that are propagated automatically simplifies reading the code. This is the purpose of this Sonar rule.

There is indeed an error in the formatting of the code, certainly caused by the deletion of the throws statement. Unfortunately, these changes were made automatically by our software. There is no other way than to correct this formatting error manually. It would be a shame not to accept this PR correcting 60 issues for this type of error. What do you think?

We're still going to analyse this formatting issue because it's not the expected behaviour.

Correcting code formatting

basil · 2024-03-25T19:07:34Z

core/src/main/java/hudson/ExtensionList.java

@@ -158,7 +158,7 @@ public void addListener(@NonNull ExtensionListListener listener) {
 *
 * Meant to simplify call inside @Extension annotated class to retrieve their own instance.
 */
- public @NonNull <U extends T> U getInstance(@NonNull Class<U> type) throws IllegalStateException {
+ public @NonNull <U extends T> U getInstance(@NonNull Class<U> type) {


Effective Java 3rd Edition §74 says:

Use the Javadoc @throws tag to document each exception that a method can throw, but do not use the throws keyword on unchecked exceptions.

This PR removes the throws keyword on this unchecked exception (good), but it does not add the Javadoc @throws tag (bad).

The dilemma is either to keep the existing code which returns unchecked exceptions or to validate the PR which fixes this case but does not declare the exception unchecked. Another solution would be to make these corrections manually. According to Sonar's estimate, it would take around 5 hours of work to remediate the code.

It's not a dilemma for me, as I simply won't approve this PR unless the changes I requested are made. According to my estimate, it would take around 15 minutes of work to make the requested changes.

It was just a suggestion for improvement that you are free to reject. Would you like me to close the PR?

It's not a net improvement if it trades one problem for another. I see no reason for this PR to be closed once the requested changes are made.

basil · 2024-03-25T19:08:32Z

core/src/main/java/hudson/Functions.java

@@ -612,7 +612,7 @@ public static String getCookie(HttpServletRequest req, String name, String defau
 private static final Pattern ICON_SIZE = Pattern.compile("\\d+x\\d+");

 @Restricted(NoExternalUse.class)
- public static String validateIconSize(String iconSize) throws SecurityException {
+ public static String validateIconSize(String iconSize) {


Effective Java 3rd Edition §74 says:

Use the Javadoc @throws tag to document each exception that a method can throw, but do not use the throws keyword on unchecked exceptions.

This PR removes the throws keyword on this unchecked exception (good), but it does not add the Javadoc @throws tag (bad).

basil · 2024-03-25T19:15:38Z

core/src/main/java/jenkins/model/Jenkins.java

@@ -832,7 +832,7 @@ public static Jenkins get() throws IllegalStateException {
 */
 @Deprecated
 @NonNull
- public static Jenkins getActiveInstance() throws IllegalStateException {
+ public static Jenkins getActiveInstance() {


Effective Java 3rd Edition §74 says:

Use the Javadoc @throws tag to document each exception that a method can throw, but do not use the throws keyword on unchecked exceptions.

This PR removes the throws keyword on this unchecked exception (good), but it does not add the Javadoc @throws tag (bad).

basil · 2024-03-25T19:16:28Z

core/src/main/java/jenkins/model/lazy/AbstractLazyLoadRunMap.java

@@ -592,7 +592,7 @@ public synchronized int maxNumberOnDisk() {
 return numberOnDisk.max();
 }

- protected final synchronized void proposeNewNumber(int number) throws IllegalStateException {
+ protected final synchronized void proposeNewNumber(int number) {


Effective Java 3rd Edition §74 says:

Use the Javadoc @throws tag to document each exception that a method can throw, but do not use the throws keyword on unchecked exceptions.

This PR removes the throws keyword on this unchecked exception (good), but it does not add the Javadoc @throws tag (bad).

basil · 2024-03-25T19:17:45Z

core/src/main/java/jenkins/util/io/PathRemover.java

@@ -100,7 +100,7 @@ public void forceRemoveRecursive(@NonNull Path path) throws IOException {
 @Restricted(NoExternalUse.class)
 @FunctionalInterface
 public interface PathChecker {
- void check(@NonNull Path path) throws SecurityException;
+ void check(@NonNull Path path);


Effective Java 3rd Edition §74 says:

Use the Javadoc @throws tag to document each exception that a method can throw, but do not use the throws keyword on unchecked exceptions.

This PR removes the throws keyword on this unchecked exception (good), but it does not add the Javadoc @throws tag (bad).

basil · 2024-03-25T19:17:53Z

core/src/main/java/org/acegisecurity/Authentication.java

@@ -49,7 +49,7 @@ public interface Authentication extends Principal, Serializable {

 boolean isAuthenticated();

- void setAuthenticated(boolean isAuthenticated) throws IllegalArgumentException;
+ void setAuthenticated(boolean isAuthenticated);


Effective Java 3rd Edition §74 says:

Use the Javadoc @throws tag to document each exception that a method can throw, but do not use the throws keyword on unchecked exceptions.

This PR removes the throws keyword on this unchecked exception (good), but it does not add the Javadoc @throws tag (bad).

basil · 2024-03-25T19:28:32Z

core/src/main/java/hudson/util/CompoundEnumeration.java

@@ -38,7 +38,7 @@ public boolean hasMoreElements() {
 }

 @Override
- public T nextElement() throws NoSuchElementException {
+ public T nextElement() {


Creates an unused import for NoSuchElementException.

Thank you for identifying this problem, we'll be looking very quickly at how to ensure that 'Indepth' doesn't reproduce this issue again.

Remove the unnecessary import of NoSuchElementException

Fix issue S1130 Exceptions in 'throws' clauses should not be superfluous

f689136

MarkEWaite requested changes Mar 25, 2024

View reviewed changes

Update QuotedStringTokenizer.java

7d5831f

Correcting code formatting

basil reviewed Mar 25, 2024

View reviewed changes

Update CompoundEnumeration.java

b9d383a

Remove the unnecessary import of NoSuchElementException

basil added the needs-comments-resolved Comments in the PR need to be addressed before it can be considered for merging label Mar 26, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix issue S1130 Exceptions in 'throws' clauses should not be superfluous #9073

Fix issue S1130 Exceptions in 'throws' clauses should not be superfluous #9073

jlerbsc commented Mar 25, 2024

MarkEWaite left a comment

MarkEWaite Mar 25, 2024

jlerbsc Mar 25, 2024

jlerbsc Mar 25, 2024 •

edited

jlerbsc Mar 25, 2024 •

edited

basil Mar 25, 2024

jlerbsc Mar 25, 2024

basil Mar 25, 2024

jlerbsc Mar 25, 2024

basil Mar 25, 2024

basil Mar 25, 2024

basil Mar 25, 2024

basil Mar 25, 2024

basil Mar 25, 2024

basil Mar 25, 2024

basil Mar 25, 2024

jlerbsc Mar 25, 2024

Fix issue S1130 Exceptions in 'throws' clauses should not be superfluous #9073

Are you sure you want to change the base?

Fix issue S1130 Exceptions in 'throws' clauses should not be superfluous #9073

Conversation

jlerbsc commented Mar 25, 2024

MarkEWaite left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jlerbsc Mar 25, 2024 • edited

Choose a reason for hiding this comment

jlerbsc Mar 25, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jlerbsc Mar 25, 2024 •

edited

jlerbsc Mar 25, 2024 •

edited