Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix false-negatives regarding LITERAL_CATCH to detect K&R Blocks in google_checks.xml #15792

Open
Zopsss opened this issue Oct 21, 2024 · 3 comments

Comments

@Zopsss
Copy link
Contributor

Zopsss commented Oct 21, 2024

I have read check documentation: https://checkstyle.org/checks/blocks/leftcurly.html https://checkstyle.org/checks/blocks/rightcurly.html
I have downloaded the latest checkstyle from: https://checkstyle.org/cmdline.html#Download_and_Run
I have executed the cli and showed it below, as cli describes the problem better than 1,000 words

follow up of #15664

In the above mentioned issue, LITERAL_FINALLY was moved from RightCurlySame to RightCurlyAlone as part of this PR: #15789

Now the current behavior of google_checks.xml for detecting K & R blocks for LITERAL_CATCH is as follows:

From google_checks.xml:

    <module name="RightCurly">
      <property name="id" value="RightCurlySame"/>
      <property name="tokens"
               value="LITERAL_TRY, LITERAL_CATCH, LITERAL_IF, LITERAL_ELSE,
                    LITERAL_DO"/>
    </module>
    <module name="RightCurly">
      <property name="id" value="RightCurlyAlone"/>
      <property name="option" value="alone"/>
      <property name="tokens"
               value="CLASS_DEF, METHOD_DEF, CTOR_DEF, LITERAL_FOR, LITERAL_WHILE, STATIC_INIT,
                    INSTANCE_INIT, ANNOTATION_DEF, ENUM_DEF, INTERFACE_DEF, RECORD_DEF,
                    COMPACT_CTOR_DEF, LITERAL_SWITCH, LITERAL_CASE, LITERAL_FINALLY"/>
    </module>
    <module name="SuppressionXpathSingleFilter">
      <!-- suppression is required till https://github.com/checkstyle/checkstyle/issues/7541 -->
      <property name="id" value="RightCurlyAlone"/>
      <property name="query" value="//RCURLY[parent::SLIST[count(./*)=1]
                                     or preceding-sibling::*[last()][self::LCURLY]]"/>
    </module>
package com.google.checkstyle.test.chapter4formatting.rule412nonemptyblocks;

/** some javadoc. */
public class InputTryCatchIfElse {

  @interface TesterAnnotation {} //ok

  void foo() throws Exception {
    int a = 90;
    boolean test = true;

    try (MyResource r = new MyResource()) { }

    try (MyResource r = new MyResource()) {}

    try (MyResource r = new MyResource()) {} catch (Exception expected) {} // false-negative

    try (MyResource r = new MyResource()) {} catch (Exception expected) { } // false-negative

    try (MyResource r = new MyResource()) {
    } catch (Exception expected) {} // false-negative

    try (MyResource r = new MyResource()) {
    } catch (Exception expected) { } // false-negative

    try (MyResource r = new MyResource()) { ; }

    try {
      /* foo */
    }
    catch (NullPointerException e) {
      /* foo */
    }
    catch (Exception e) {
      /* foo */
    }
    finally {
      test = true;
    }

    try {
      /* foo */
    } catch (NullPointerException e) {
      /* foo */
    }
    catch (Exception e) {
      /* foo */
    }
    finally {
      test = true;
    }

    try {
      /* foo */
    }
    catch (Exception e) {
      /* foo */
    }
    finally {
      test = true;
    }

    try {
      /* foo */
    } catch (Exception e) {
      /* foo */
    }
    finally {
      test = true;
    }

    try {
      /* foo */
    } catch (NullPointerException e) {
      /* foo */
    } catch (Exception e) {
      /* foo */
    } finally {
      test = true;
    }
  }

  /** some javadoc. */
  public class MyResource implements AutoCloseable {
    /** some javadoc. */
    @Override
    public void close() throws Exception {
      System.out.println("Closed MyResource");
    }
  }
}
$ java -jar .\checkstyle-10.19.0-SNAPSHOT-all.jar -c .\google_checks.xml .\InputTryCatchIfElse.java 
Starting audit...
[WARN] C:\checkstyle testing\.\InputTryCatchIfElse.java:12: Empty blocks should have no spaces. Empty blocks                                    may only be represented as {} when not part of a                                    multi-block statement (4.1.3) [RegexpSinglelineJava]
[WARN] C:\checkstyle testing\.\InputTryCatchIfElse.java:18: Empty blocks should have no spaces. Empty blocks                                    may only be represented as {} when not part of a                                    multi-block statement (4.1.3) [RegexpSinglelineJava]
[WARN] C:\checkstyle testing\.\InputTryCatchIfElse.java:24: Empty blocks should have no spaces. Empty blocks                                    may only be represented as {} when not part of a                                    multi-block statement (4.1.3) [RegexpSinglelineJava]
[WARN] C:\checkstyle testing\.\InputTryCatchIfElse.java:26:43: '{' at column 43 should have line break after. [LeftCurlyEol]
[WARN] C:\checkstyle testing\.\InputTryCatchIfElse.java:30:5: '}' at column 5 should be on the same line as the next part of a multi-block statement (one that directly contains multiple blocks: if/else-if/else, do/while or try/catch/finally). [RightCurlySame]
[WARN] C:\checkstyle testing\.\InputTryCatchIfElse.java:33:5: '}' at column 5 should be on the same line as the next part of a multi-block statement (one that directly contains multiple blocks: if/else-if/else, do/while or try/catch/finally). [RightCurlySame]
[WARN] C:\checkstyle testing\.\InputTryCatchIfElse.java:36:5: '}' at column 5 should be on the same line as the next part of a multi-block statement (one that directly contains multiple blocks: if/else-if/else, do/while or try/catch/finally). [RightCurlySame]
[WARN] C:\checkstyle testing\.\InputTryCatchIfElse.java:45:5: '}' at column 5 should be on the same line as the next part of a multi-block statement (one that directly contains multiple blocks: if/else-if/else, do/while or try/catch/finally). [RightCurlySame]
[WARN] C:\checkstyle testing\.\InputTryCatchIfElse.java:48:5: '}' at column 5 should be on the same line as the next part of a multi-block statement (one that directly contains multiple blocks: if/else-if/else, do/while or try/catch/finally). [RightCurlySame]
[WARN] C:\checkstyle testing\.\InputTryCatchIfElse.java:55:5: '}' at column 5 should be on the same line as the next part of a multi-block statement (one that directly contains multiple blocks: if/else-if/else, do/while or try/catch/finally). [RightCurlySame]
[WARN] C:\checkstyle testing\.\InputTryCatchIfElse.java:58:5: '}' at column 5 should be on the same line as the next part of a multi-block statement (one that directly contains multiple blocks: if/else-if/else, do/while or try/catch/finally). [RightCurlySame]
[WARN] C:\checkstyle testing\.\InputTryCatchIfElse.java:67:5: '}' at column 5 should be on the same line as the next part of a multi-block statement (one that directly contains multiple blocks: if/else-if/else, do/while or try/catch/finally). [RightCurlySame]
Audit done.

As part of this issue, we need to fix all false-negatives regarding LITERAL_CATCH. Generate diff reports and find edge cases, fix them too.

The issue will most probably require modifying suppression for LITERAL_CATCH.

@AmitKumarDeoghoria
Copy link
Contributor

May I contribute to this issue?

@romani
Copy link
Member

romani commented Oct 25, 2024

@AmitKumarDeoghoria , sure, all issues are welcoming contributions.

I have not seen any PR from your username
https://github.com/checkstyle/checkstyle/pulls/AmitKumarDeoghoria .
Selection of this issue might be challenging. We usually recommend new contributors to star being familiar with project on more simple issues https://github.com/checkstyle/checkstyle/wiki/To-New-Contributor

@AmitKumarDeoghoria
Copy link
Contributor

Thanks @romani I will familiarize myself with the project by working on beginner-friendly issues first and then proceed to tackle this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants