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

AnnotationLocation: add support for PACKAGE_DEF, ENUM_CONSTANT_DEF #6379

Closed
pbludov opened this issue Jan 15, 2019 · 6 comments

Comments

Projects
None yet
3 participants
@pbludov
Copy link
Collaborator

commented Jan 15, 2019

https://checkstyle.org/config_annotation.html#AnnotationLocation

$ javac TestClass.java package-info.java
(no output)

$ cat TestConfig.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">
  <module name="TreeWalker">
    <module name="AnnotationLocation">
      <property name="allowSamelineSingleParameterlessAnnotation" value="false"/>
      <!-- all available tokens -->
      <property name="tokens" value="CLASS_DEF,INTERFACE_DEF,ENUM_DEF,METHOD_DEF,CTOR_DEF,VARIABLE_DEF,PARAMETER_DEF,ANNOTATION_DEF,TYPECAST,LITERAL_THROWS,IMPLEMENTS_CLAUSE,TYPE_ARGUMENT,LITERAL_NEW,DOT,ANNOTATION_FIELD_DEF"/>
    </module>
  </module>
</module>  

$ cat TestClass.java

package test;

import java.lang.annotation.ElementType;
import java.lang.annotation.Target;

@Ann enum TestEnum { // ok (line 6)

    @Ann ENUM_VALUE(); // missing violation

    TestEnum() {
        Object o = (@Ann TestEnum & @Ann Cloneable) null; // missing violation  or ... ??
    }
}

@Target({ElementType.TYPE_USE, ElementType.PACKAGE, ElementType.FIELD})
@interface Ann {}

$cat package-info.java

@Ann package test; //missing violation

$ RUN_LOCALE="-Duser.language=en -Duser.country=US"
$RUN_LOCALE java -jar checkstyle-8.16-all.jar -c TestConfig.xml TestClass.java package-info.java

Starting audit...
[WARN] TestClass.java:6: Annotation 'Ann' should be alone on line. [AnnotationLocation]
Audit done.

Expected 2 more violations.

The tokens PACKAGE_DEF, ENUM_CONSTANT_DEF, have no MODIFIERS, their annotations are under ANNOTATIONS token:

for example

    @Ann   ENUM_VALUE();

is

    |--ENUM_CONSTANT_DEF
    |   |--ANNOTATIONS
    |   |   |--ANNOTATION

AnnotationLocation does not support ANNOTATIONS at all. This is a bug and should be fixed.

pbludov added a commit to pbludov/checkstyle that referenced this issue Jan 15, 2019

pbludov added a commit to pbludov/checkstyle that referenced this issue Jan 18, 2019

pbludov added a commit to pbludov/checkstyle that referenced this issue Jan 18, 2019

@checkstyle checkstyle deleted a comment from rnveach Jan 19, 2019

@romani

This comment has been minimized.

Copy link
Member

commented Jan 19, 2019

(@Ann TestEnum & @Ann Cloneable) null; // missing violation

this is expected behavior, Check is for annotations over declarations, not for linewraps.


I have some worries about TYPECAST usage in this Check. Original idea was to define rules for places where multiline mode is highly used by humans.
@rnveach, @pbludov, Please share your thoughts on this.

TYPECAST

Please apply it to our config, if that is not applicable to us, most likely nobody needs this.

AnnotationLocation does not support ANNOTATIONS at all. This is a bug and should be fixed.

Tokens are some sort of targets/modes for Check. It is by design, it is not a bug. If I miss smth, please explain.

@rnveach

This comment has been minimized.

Copy link
Member

commented Jan 20, 2019

I have some worries about TYPECAST usage in this Check

TYPECAST token is already in the check as an acceptable token.

Original idea was to define rules for places where multiline mode is highly used by humans.

How does this explain DOT and LITERAL_NEW tokens then which the check accepts?

I have no problem with this change. It is already an acceptable token. If it isn't working correctly then lets fix it.

@pbludov

This comment has been minimized.

Copy link
Collaborator Author

commented Jan 21, 2019

I have some worries about TYPECAST usage in this Check.

I think the check should handle all annotations. Yes, it is unlikely that anyone will use multiline annotations for TYPECAST, PRAMETER_DEF and the ones that Richard mentioned above.
Definitely, these tokens should not be enabled by default. But their support is worth nothing.
Let the user decide which tokens to use.

@romani

This comment has been minimized.

Copy link
Member

commented Jan 27, 2019

But their support is worth nothing.

Current code probably cost nothing, but maintenance of it will cost a lot. As it is not clear fir what this Check was created. We try to avoid Checks that do all, it never works well. Wr would need to make reasonable behavior to all modes https://checkstyle.org/config_annotation.html#AnnotationLocation_Properties

It is better to update documentation to mention that such Check do not support such cases, and focused on annotations over declarations rather than line wrapping cases.

Does it make sense ?

pbludov added a commit to pbludov/checkstyle that referenced this issue Jan 28, 2019

pbludov added a commit to pbludov/checkstyle that referenced this issue Jan 28, 2019

@pbludov

This comment has been minimized.

Copy link
Collaborator Author

commented Jan 28, 2019

@romani ok, a removed TYPECAST from the check. Anyway, it doesn't matter.

@pbludov pbludov changed the title AnnotationLocation: add support for PACKAGE_DEF, ENUM_CONSTANT_DEF, TYPECAST AnnotationLocation: add support for PACKAGE_DEF, ENUM_CONSTANT_DEF Jan 28, 2019

pbludov added a commit to pbludov/checkstyle that referenced this issue Jan 28, 2019

pbludov added a commit to pbludov/checkstyle that referenced this issue Jan 28, 2019

pbludov added a commit to pbludov/checkstyle that referenced this issue Jan 29, 2019

@romani romani added the approved label Jan 29, 2019

pbludov added a commit to pbludov/checkstyle that referenced this issue Feb 2, 2019

pbludov added a commit to pbludov/checkstyle that referenced this issue Feb 2, 2019

pbludov added a commit to pbludov/checkstyle that referenced this issue Feb 2, 2019

pbludov added a commit to pbludov/checkstyle that referenced this issue Feb 2, 2019

pbludov added a commit to pbludov/checkstyle that referenced this issue Feb 4, 2019

pbludov added a commit to pbludov/checkstyle that referenced this issue Feb 10, 2019

pbludov added a commit to pbludov/checkstyle that referenced this issue Feb 10, 2019

pbludov added a commit to pbludov/checkstyle that referenced this issue Feb 10, 2019

pbludov added a commit to pbludov/checkstyle that referenced this issue Feb 10, 2019

pbludov added a commit to pbludov/checkstyle that referenced this issue Feb 10, 2019

pbludov added a commit to pbludov/checkstyle that referenced this issue Feb 10, 2019

pbludov added a commit to pbludov/checkstyle that referenced this issue Feb 18, 2019

pbludov added a commit to pbludov/checkstyle that referenced this issue Feb 24, 2019

pbludov added a commit to pbludov/checkstyle that referenced this issue Feb 24, 2019

pbludov added a commit to pbludov/checkstyle that referenced this issue Feb 24, 2019

pbludov added a commit to pbludov/checkstyle that referenced this issue Feb 24, 2019

pbludov added a commit to pbludov/checkstyle that referenced this issue Feb 24, 2019

pbludov added a commit to pbludov/checkstyle that referenced this issue Feb 24, 2019

pbludov added a commit to pbludov/checkstyle that referenced this issue Feb 24, 2019

pbludov added a commit to pbludov/checkstyle that referenced this issue Feb 24, 2019

pbludov added a commit to pbludov/checkstyle that referenced this issue Feb 24, 2019

romani added a commit that referenced this issue Mar 1, 2019

@romani romani added the new feature label Mar 1, 2019

@romani romani added this to the 8.19 milestone Mar 1, 2019

@romani

This comment has been minimized.

Copy link
Member

commented Mar 1, 2019

Fix is merged

@romani romani closed this Mar 1, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.