Issue #15058: New Check UnusedCatchParameterShouldBeUnnamed by mahfouz72 · Pull Request #15358 · checkstyle/checkstyle

mahfouz72 · 2024-07-24T14:35:20Z

New module config: https://gist.githubusercontent.com/mahfouz72/afd57aa6e51ca161b7927acd959e5211/raw/3290828c8bb75105d18940f38a97265806b6a8bb/check.xml

mahfouz72 · 2024-07-24T15:47:30Z

github, generate site

github-actions · 2024-07-24T15:58:37Z

https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/c50d036_2024155612/index.html

https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/c50d036_2024155612/checks.html#

https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/c50d036_2024155612/checks/coding/index.html#

https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/c50d036_2024155612/checks/coding/unusedcatchparametershouldbeunnamed.html#

romani

Item:

nrmancuso

Item to discuss before anything else:

nrmancuso

First pass:

nrmancuso · 2024-07-25T02:23:54Z

+     */
+    private void checkIdentifier(DetailAST identAst) {
+        if (catchParameter != null
+                && !isLeftHandSideValue(identAst)


So we don't count reassignment as usage? Judging by the test cases below, we currently do. What am I missing?

Yes we don't count reassignment as usage. here is its test case :

checkstyle/src/test/resources-noncompilable/com/puppycrawl/tools/checkstyle/checks/coding/unusedcatchparametershouldbeunnamed/InputUnusedCatchParameterShouldBeUnnamed.java

Lines 49 to 53 in c50d036

try {

int x = 1/ 0;

}

catch (Exception e) { // violation

e = new Exception();

Judging by the test cases below, we currently do.

can you point to this?

Note I don't consider the test case below to be a reassignment.

checkstyle/src/test/resources-noncompilable/com/puppycrawl/tools/checkstyle/checks/coding/unusedcatchparametershouldbeunnamed/InputUnusedCatchParameterShouldBeUnnamed.java

Lines 55 to 60 in c50d036

try {

int x = 1/ 0;

}

catch (Exception e) {

exception = e;

}

#15358 (comment) is weird to me.If we were to make e unnamed, we would have to remove this reassignment (even though it does nothing). I suppose I am good with this, let's get this check into the hands of users and go from there.

Leaving this one unresolved so others can see.

nrmancuso

Last from me:

mahfouz72 · 2024-07-27T23:36:26Z

github, generate report

github-actions · 2024-07-28T00:37:20Z

https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/f4c2b67_2024002539/reports/diff/index.html

nrmancuso

I have spot checked the report, there is far too many violations to check all of them. This is a good first implementation that we can build on, let's get this check into the hands of users.

Great work!

mahfouz72 · 2024-08-02T10:15:28Z

github, generate report

github-actions · 2024-08-02T10:41:18Z

https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/be69edc_2024103914/reports/diff/index.html

mahfouz72 · 2024-08-02T12:52:29Z

@rnveach @nrmancuso do we agree to suppress this? adding a non-nullity condition will be a surviving mutation

  <checkerFrameworkError unstable="false">
    <fileName>src/main/java/com/puppycrawl/tools/checkstyle/checks/coding/UnusedCatchParameterShouldBeUnnamedCheck.java</fileName>
    <specifier>dereference.of.nullable</specifier>
    <message>dereference of possibly-null reference catchParameter</message>
    <lineContent>if (!&quot;_&quot;.equals(catchParameter.getName()) &amp;&amp; !catchParameter.isUsed()) {</lineContent>
  </checkerFrameworkError>

nrmancuso · 2024-08-03T18:12:28Z

@rnveach @nrmancuso do we agree to suppress this? adding a non-nullity condition will be a surviving mutation

  <checkerFrameworkError unstable="false">
    <fileName>src/main/java/com/puppycrawl/tools/checkstyle/checks/coding/UnusedCatchParameterShouldBeUnnamedCheck.java</fileName>
    <specifier>dereference.of.nullable</specifier>
    <message>dereference of possibly-null reference catchParameter</message>
    <lineContent>if (!&quot;_&quot;.equals(catchParameter.getName()) &amp;&amp; !catchParameter.isUsed()) {</lineContent>
  </checkerFrameworkError>

We can probably make all static analysis happy and also be a bit more expressive:

            final Optional<CatchParameterDetails> unusedCatchParameter = Optional.ofNullable(catchParameters.peek())
                    .filter(p -> !"_".equals(p.getName()))
                    .filter(p -> !p.isUsed());

            unusedCatchParameter.ifPresent(catchParameterDetails ->
                    log(catchParameterDetails.getParameterDefinition(),
                            MSG_UNUSED_CATCH_PARAMETER,
                            catchParameterDetails.getName()));

mahfouz72 · 2024-08-05T11:11:38Z

@rnveach ping

rnveach · 2024-08-06T02:14:17Z

Rebasing to ensure no issues on merge.

rnveach · 2024-08-06T03:13:55Z

checkstyle/contribution#883 (comment) is needed for a merge

mahfouz72 · 2024-08-06T10:50:10Z

@rnveach done

mahfouz72 mentioned this pull request Jul 24, 2024

Config: New Check UnusedCatchParameterShouldBeUnnamed checkstyle/contribution#883

Merged

mahfouz72 force-pushed the new-check-unusedcatch branch 3 times, most recently from ad85082 to c50d036 Compare July 24, 2024 15:33

mahfouz72 commented Jul 24, 2024

View reviewed changes

Comment thread .../com/puppycrawl/tools/checkstyle/checks/coding/UnusedCatchParameterShouldBeUnnamedCheck.java

romani requested changes Jul 24, 2024

View reviewed changes

Comment thread config/checkstyle-checks.xml

nrmancuso suggested changes Jul 25, 2024

View reviewed changes

Comment thread .../com/puppycrawl/tools/checkstyle/checks/coding/UnusedCatchParameterShouldBeUnnamedCheck.java

nrmancuso self-assigned this Jul 25, 2024

nrmancuso suggested changes Jul 25, 2024

View reviewed changes

mahfouz72 force-pushed the new-check-unusedcatch branch 4 times, most recently from e7960cb to c566762 Compare July 26, 2024 01:18

nrmancuso suggested changes Jul 27, 2024

View reviewed changes

mahfouz72 force-pushed the new-check-unusedcatch branch 5 times, most recently from a1da5b6 to f4c2b67 Compare July 27, 2024 23:21

nrmancuso force-pushed the new-check-unusedcatch branch from f4c2b67 to 052fb36 Compare July 28, 2024 01:14

nrmancuso approved these changes Jul 28, 2024

View reviewed changes

nrmancuso assigned romani and unassigned nrmancuso Jul 28, 2024

romani assigned rnveach and unassigned romani Jul 28, 2024

romani requested a review from rnveach July 28, 2024 02:06

rnveach mentioned this pull request Aug 2, 2024

UnusedLocalVariableCheck: AST with violation doesn't make sense #15423

Open

mahfouz72 force-pushed the new-check-unusedcatch branch 3 times, most recently from f0efbb8 to be69edc Compare August 2, 2024 02:14

mahfouz72 force-pushed the new-check-unusedcatch branch 3 times, most recently from e146622 to bd7624c Compare August 2, 2024 23:18

mahfouz72 force-pushed the new-check-unusedcatch branch from bd7624c to f86a9b0 Compare August 4, 2024 01:50

rnveach suggested changes Aug 4, 2024

View reviewed changes

Comment thread ...ding/unusedcatchparametershouldbeunnamed/InputUnusedCatchParameterShouldBeUnnamedNested.java Outdated

mahfouz72 force-pushed the new-check-unusedcatch branch 2 times, most recently from 0d4cf07 to 4867842 Compare August 4, 2024 02:16

rnveach suggested changes Aug 5, 2024

View reviewed changes

Comment thread .../puppycrawl/tools/checkstyle/checks/coding/UnusedCatchParameterShouldBeUnnamedCheckTest.java Outdated

mahfouz72 force-pushed the new-check-unusedcatch branch 3 times, most recently from c70de7d to 66e0f2a Compare August 6, 2024 00:44

rnveach approved these changes Aug 6, 2024

View reviewed changes

Issue checkstyle#15058: New Check UnusedCatchParameterShouldBeUnnamed

771eae7

rnveach force-pushed the new-check-unusedcatch branch from 66e0f2a to 771eae7 Compare August 6, 2024 02:14

rnveach merged commit 93d7140 into checkstyle:master Aug 6, 2024

mahfouz72 mentioned this pull request Aug 10, 2024

Issue #15059: New Check UnusedLambdaParameterShouldBeUnnamed #15480

Merged

rnveach mentioned this pull request Aug 23, 2024

Issue #15159: RedundantModifierCheck should violate final modifier on… #15492

Merged

mahfouz72 deleted the new-check-unusedcatch branch May 9, 2025 09:39

	try {
	int x = 1/ 0;
	}
	catch (Exception e) { // violation
	e = new Exception();

Uh oh!

Conversation

mahfouz72 commented Jul 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mahfouz72 commented Jul 24, 2024

Uh oh!

Uh oh!

github-actions Bot commented Jul 24, 2024

Uh oh!

romani left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nrmancuso left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nrmancuso left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nrmancuso Jul 25, 2024

Choose a reason for hiding this comment

Uh oh!

mahfouz72 Jul 25, 2024

Choose a reason for hiding this comment

Uh oh!

mahfouz72 Jul 25, 2024

Choose a reason for hiding this comment

Uh oh!

nrmancuso Jul 27, 2024

Choose a reason for hiding this comment

Uh oh!

nrmancuso Jul 27, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nrmancuso left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mahfouz72 commented Jul 27, 2024

Uh oh!

github-actions Bot commented Jul 28, 2024

Uh oh!

nrmancuso left a comment

Choose a reason for hiding this comment

Uh oh!

mahfouz72 commented Aug 2, 2024

Uh oh!

github-actions Bot commented Aug 2, 2024

Uh oh!

mahfouz72 commented Aug 2, 2024

Uh oh!

nrmancuso commented Aug 3, 2024

Uh oh!

Uh oh!

mahfouz72 commented Aug 5, 2024

Uh oh!

Uh oh!

rnveach commented Aug 6, 2024

Uh oh!

rnveach commented Aug 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mahfouz72 commented Jul 24, 2024 •

edited

Loading

rnveach commented Aug 6, 2024 •

edited

Loading