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

Add Check Support for Java 17 Sealed Classes: AnnotationOnSameLine #15074

Closed
mahfouz72 opened this issue Jun 21, 2024 · 12 comments
Closed

Add Check Support for Java 17 Sealed Classes: AnnotationOnSameLine #15074

mahfouz72 opened this issue Jun 21, 2024 · 12 comments
Assignees

Comments

@mahfouz72
Copy link
Member

mahfouz72 commented Jun 21, 2024

child of #14969

Check documentation : https://checkstyle.org/checks/annotation/annotationonsameline.html#AnnotationOnSameLine



PS D:\CS\test> javac src/Test.java
PS D:\CS\test> cat config.xml
<?xml version="1.0"?>
<!DOCTYPE module PUBLIC
"-//Puppy Crawl//DTD Check Configuration 1.3//EN"
"http://www.puppycrawl.com/dtds/configuration_1_3.dtd">
<module name="Checker">
<property name="charset" value="UTF-8"/>
<module name="TreeWalker">
<module name="AnnotationOnSameLine">
<property name="tokens" value="INTERFACE_DEF, CLASS_DEF,IMPLEMENTS_CLAUSE"/>
</module>
</module>
</module>
PS D:\CS\test> cat src/Test.java
import java.lang.annotation.Target;

import static java.lang.annotation.ElementType.*;
import static java.lang.annotation.ElementType.TYPE_USE;
class Test{}

sealed @Ann // violation
@Ann2 interface A permits B {}

final @Ann2 // violation
class B implements @Ann // violation
A {}

@Ann // violation
sealed
@Ann2 // violation
class C permits D {}

@Ann sealed
@Ann2 // violation
class E permits F {}


final class D extends C {}
final class F extends E {}

 @Target({TYPE_USE}) @interface Ann {}
@Target({TYPE_USE}) @interface Ann2 {}

PS D:\CS\test> java -jar checkstyle-10.17.0-all.jar -c config.xml src/Test.java
Starting audit...
[ERROR] D:\CS\test\src\Test.java:7:8: Annotation 'Ann' should be on the same line with its target. [AnnotationOnSameLine]
[ERROR] D:\CS\test\src\Test.java:10:7: Annotation 'Ann2' should be on the same line with its target. [AnnotationOnSameLine]
[ERROR] D:\CS\test\src\Test.java:11:20: Annotation 'Ann' should be on the same line with its target. [AnnotationOnSameLine]
[ERROR] D:\CS\test\src\Test.java:14:1: Annotation 'Ann' should be on the same line with its target. [AnnotationOnSameLine]
[ERROR] D:\CS\test\src\Test.java:16:1: Annotation 'Ann2' should be on the same line with its target. [AnnotationOnSameLine]
[ERROR] D:\CS\test\src\Test.java:20:1: Annotation 'Ann2' should be on the same line with its target. [AnnotationOnSameLine]
Audit done.
Checkstyle ends with 6 errors.
PS D:\CS\test>


This check working correctly no update is needed.

Note that annotations are not acceptable after permits

PS D:\CS\test> cat src/Test.java
sealed @Ann
@Ann2 interface A permits @Ann // compile time error
B {}

final @Ann2
class B implements @Ann
A {}
....
PS D:\CS\test> javac src/Test.java
src\Test.java:8: error: <identifier> expected
@Ann2 interface A permits @Ann
^
src\Test.java:9: error: class, interface, enum, or record expected
B {}
^
2 errors
PS D:\CS\test>
@rnveach
Copy link
Member

rnveach commented Jun 22, 2024

@mahfouz72 I am not seeing a compile check that this is valid syntax.

I expect a violation on the annotation after permits

https://docs.oracle.com/javase/specs/jls/se22/html/jls-8.html#jls-ClassPermits
I am not seeing annotations listed as acceptable here in permits.

If this was allowed, why would someone put an annotation on a permits?

@rnveach rnveach self-assigned this Jun 22, 2024
@mahfouz72
Copy link
Member Author

sorry this won't compile IntelliJ didn't show an error on this line and I forgot to check Javac. First post is updated

@rnveach
Copy link
Member

rnveach commented Jun 23, 2024

sealed @Ann // violation
@Ann2 interface A permits B {}

Is it possible for annotations to be before sealed? Show an example with and without violations if you can.

@mahfouz72
Copy link
Member Author

it is possible and I think it works correctly please check first post

@mahfouz72
Copy link
Member Author

mahfouz72 commented Jun 23, 2024

@ann sealed
@ann2 // violation
class E permits F {}

I am not sure about this part.

@Ann 's target is class E but we added it before sealed modifier so should this be a violation because the annotation is not on the same line as its target class?

EDIT: I tried to use a public modifier instead of sealed to see how the check deals with this case and the behavior is similar.

@Ann public
@Ann2  // violation
class E {}

@rnveach
Copy link
Member

rnveach commented Jun 23, 2024

should this be a violation because the annotation is not on the same line as its target class?

https://checkstyle.org/checks/annotation/annotationonsameline.html#Description

Checks that annotations are located on the same line with their targets

If @ann is targeting E then this seems like a yes, this should be a violation.

If the following isn't a violation, then this seems like a bug, but unrelated to sealed classes.

@Ann public
@Ann2 class E {}

@target({CONSTRUCTOR, FIELD, METHOD, PARAMETER, TYPE, TYPE_PARAMETER, TYPE_USE}) @interface Ann {}

You don't use most of these like constructor or field. I would trim these down to the bare minimum and confirm just so we aren't making some type of mistake here on what the "target" is.

@rnveach
Copy link
Member

rnveach commented Jun 26, 2024

@mahfouz72 I see updates were done, please remember to ping me back so I can remember to come back here and view. So much is going on that I rely on notifications a lot more now.

If the following isn't a violation, then this seems like a bug, but unrelated to sealed classes.

Did you confirm this?

@mahfouz72
Copy link
Member Author

Yes, this bug is unrelated to the sealed class it exists with any modifier. When the Anno is on the same line with the target class modifier but not on the same line with the target class itself

PS D:\CS\test> javac src/Test.java                                               
PS D:\CS\test> cat config.xml                                                    
<?xml version="1.0"?>
<!DOCTYPE module PUBLIC
          "-//Checkstyle//DTD Checkstyle Configuration 1.3//EN"
          "https://checkstyle.org/dtds/configuration_1_3.dtd">

<module name="Checker">
  <module name="TreeWalker">
    <module name="AnnotationOnSameLine"/>
  </module>
</module>
PS D:\CS\test> cat src/Test.java                                                 
import java.lang.annotation.Target;

import static java.lang.annotation.ElementType.TYPE_USE;

public  class Test {
@Ann public   // expected
@Ann2 class E {}

@Ann private  // expected
@Ann2 class A {}

@Ann sealed  // expected
@Ann2 class B permits C {}

@Ann final  // expected
@Ann2 class C extends B {}


@Target({TYPE_USE}) @interface Ann {}
@Target({TYPE_USE}) @interface Ann2 {}
}
PS D:\CS\test> java  -jar checkstyle-10.17.0-all.jar -c config.xml  src/Test.java
Starting audit...
Audit done.

@rnveach I guess we should close this and open a new issue with this code because this is not related to sealed directly

@rnveach
Copy link
Member

rnveach commented Jun 27, 2024

this is not related to sealed directly

I agree. I feel we can just repurpose this existing issue by updating the title and first post. I am not against just creating a new issue.

@mahfouz72 mahfouz72 changed the title Add Check Support for Java 17 Sealed Classes: AnnotationOnSameLine AnnotationOnSameLine: FalseNegative when the annotation is on the same line of the modifer of the target class Jun 27, 2024
@mahfouz72
Copy link
Member Author

@rnveach I updated title and first post please approve if you are good

@rnveach rnveach assigned nrmancuso and unassigned rnveach Jun 27, 2024
@nrmancuso
Copy link
Member

@mahfouz72 please make a new issue as @rnveach suggested

@mahfouz72 mahfouz72 changed the title AnnotationOnSameLine: FalseNegative when the annotation is on the same line of the modifer of the target class Add Check Support for Java 17 Sealed Classes: AnnotationOnSameLine Jun 28, 2024
@mahfouz72
Copy link
Member Author

This can be closed no update is needed related to sealed classes. The discoverd (unrelated to sealed classes) bug has been moved to a new issue #15161

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

No branches or pull requests

3 participants