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

new check: UnnecessarySemicolonAfterTypeMemberDeclaration #6847

Closed
strkkk opened this issue Jun 25, 2019 · 11 comments
Closed

new check: UnnecessarySemicolonAfterTypeMemberDeclaration #6847

strkkk opened this issue Jun 25, 2019 · 11 comments

Comments

@strkkk
Copy link
Member

strkkk commented Jun 25, 2019

$ cat TestClass.java
public class TestClass {

   int field;; // violation expected
    enum A {
        C, D;; // violation expected
    }; // violation expected

    {/*init block*/}; // violation
    
    static {}; // violation

    TestClass (){}; // violation

    class A{}; // violation

    void method(boolean cond){
    }; // violation

    interface aa{}; //violation

    enum aa1{}; // violation

    @interface anno {}; // violation

}; // violation

$ cat conf.xml
<?xml version="1.0"?>
<!DOCTYPE module PUBLIC
          "-//Puppy Crawl//DTD Check Configuration 1.3//EN"
          "https://checkstyle.org/dtds/configuration_1_3.dtd">

<module name="Checker">
    <property name="charset" value="UTF-8"/>

    <module name="TreeWalker">
       <module name="UnnecessarySemicolonAfterTypeMemberDeclaration"/>
    </module>
</module>

$ java -jar checkstyle-8.20-all.jar -c conf.xml Test.java
Starting audit...
here is violations at lines commented with // violation
@romani
Copy link
Member

romani commented Jun 25, 2019

Let's wait for discussion at #6849.

@romani
Copy link
Member

romani commented Jun 26, 2019

Lets agree on name:

UnnecessarySemicolonInTypeMemberDeclaration
vs
UnnecessarySemicolonAfterTypeMemberDeclaration

What name should we choose ?

with name somethig like "1)" I do not think we will target

}; // violation for ; after outer class

as it is not Type Member , I do not see for now how to address this . Probably we can have option like validateOuterTypeSemicolon ? Extra Check for this looks like overkill .... I have never seen such code.
Can we just ignore validation of such weird code? (it is better to focus on more true to life cases)
Any ideas ?

@pbludov
Copy link
Member

pbludov commented Jun 26, 2019

May be UnnecessarySemicolonBetweenTypeMembers?

@strkkk
Copy link
Member Author

strkkk commented Jun 26, 2019

I think it can be named UnnecessarySemicolonAfterTypeMemberDeclaration and it will detect semicolon after outer class as additional case. Its functionality won't be fully described by name, but I don't see a big problem with it and many complaints about it.
It can be just mentioned in docs.

@strkkk
Copy link
Member Author

strkkk commented Jul 4, 2019

@romani any updates?

@romani
Copy link
Member

romani commented Jul 5, 2019

UnnecessarySemicolonBetweenTypeMembers

Not good, as if there is single member and semicolon after it , we still would like to violate it.

UnnecessarySemicolonAfterTypeMemberDeclaration

I am in favor of this name.

Its functionality won't be fully described by name,

Do we really want to validate this ?
I am ok as special property, not activated by default. To make default behavior consistent with name. On our code we should activate such property.

any updates?

@rnveach and @pbludov please do final sign off.
If no reply in one week, I consider this as no objection.

@rnveach
Copy link
Member

rnveach commented Jul 5, 2019

Like I said in other issue, I am still infavor of InTypeMemberDeclaration.
I am fine with logic of the check.

@romani romani changed the title new check: UnnecessarySemicolonAfterBlock new check:UnnecessarySemicolonAfterTypeMemberDeclaration Jul 6, 2019
@romani romani changed the title new check:UnnecessarySemicolonAfterTypeMemberDeclaration new check: UnnecessarySemicolonAfterTypeMemberDeclaration Jul 6, 2019
@romani romani added the approved label Jul 6, 2019
@romani
Copy link
Member

romani commented Jul 6, 2019

InTypeMemberDeclaratio

In my mind, such name should do more than just semicolon after curly brace..
Let's proceed for now with name that has 2 votes.

@pbludov
Copy link
Member

pbludov commented Jul 6, 2019

I have never seen such code.

I've seen it many times. Each C++ engineer initially wrote Java code as follows:
http://hg.openjdk.java.net/jdk/jdk11/file/1ddf9a99e4ad/src/java.desktop/share/classes/javax/accessibility/AccessibleTextSequence.java#l67

I think we need both UnnecessarySemicolonAfterTypeMemberDeclaration and UnnecessarySemicolonAfterTypeDeclaration

@romani
Copy link
Member

romani commented Jul 6, 2019

UnnecessarySemicolonAfterTypeDeclaration

Looks like not only me think about this as something that is separate from UnnecessarySemicolonAfterTypeMemberDeclaration , looks like separate Check is good option for this.

strkkk added a commit to strkkk/checkstyle that referenced this issue Jul 10, 2019
strkkk added a commit to strkkk/checkstyle that referenced this issue Jul 10, 2019
strkkk added a commit to strkkk/checkstyle that referenced this issue Jul 19, 2019
strkkk added a commit to strkkk/checkstyle that referenced this issue Jul 19, 2019
strkkk added a commit to strkkk/checkstyle that referenced this issue Jul 22, 2019
strkkk added a commit to strkkk/checkstyle that referenced this issue Jul 22, 2019
strkkk added a commit to strkkk/checkstyle that referenced this issue Jul 27, 2019
strkkk added a commit to strkkk/checkstyle that referenced this issue Aug 1, 2019
strkkk added a commit to strkkk/checkstyle that referenced this issue Aug 5, 2019
strkkk added a commit to strkkk/checkstyle that referenced this issue Aug 5, 2019
strkkk added a commit to strkkk/checkstyle that referenced this issue Aug 6, 2019
strkkk added a commit to strkkk/checkstyle that referenced this issue Aug 7, 2019
strkkk added a commit to strkkk/checkstyle that referenced this issue Aug 7, 2019
@romani romani added this to the 8.24 milestone Aug 11, 2019
@romani
Copy link
Member

romani commented Aug 11, 2019

Fix is merged

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

4 participants