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: Remove unused tokens #6416

Closed
pbludov opened this Issue Feb 10, 2019 · 9 comments

Comments

Projects
None yet
4 participants
@pbludov
Copy link
Collaborator

pbludov commented Feb 10, 2019

From #6379

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

Acceptable tokens for AnnotationLocationCheck are

    public int[] getAcceptableTokens() {
        return new int[] {
            TokenTypes.CLASS_DEF,
            TokenTypes.INTERFACE_DEF,
            TokenTypes.PACKAGE_DEF,
            TokenTypes.ENUM_CONSTANT_DEF,
            TokenTypes.ENUM_DEF,
            TokenTypes.METHOD_DEF,
            TokenTypes.CTOR_DEF,
            TokenTypes.VARIABLE_DEF,
            TokenTypes.PARAMETER_DEF,
            TokenTypes.ANNOTATION_DEF,
            TokenTypes.TYPECAST,
            TokenTypes.LITERAL_THROWS,
            TokenTypes.IMPLEMENTS_CLAUSE,
            TokenTypes.TYPE_ARGUMENT,
            TokenTypes.LITERAL_NEW,
            TokenTypes.DOT,
            TokenTypes.ANNOTATION_FIELD_DEF,
        };
    }

Tokens LITERAL_THROWS, DOT, IMPLEMENTS_CLAUSE shouldn't be here.
They have no annotation tokens among children.
Tokens TYPECAST and LITERAL_NEW may have annotations, but they aren't fit to the check:

Object var = @Ann
                    new Type();
return (@Ann
          ) var;

This code will compile, but no one will use this style.
For now, AnnotationLocationCheck ignores these tokens, although they are acceptable.

Full list of removed tokens:

            TokenTypes.TYPECAST,
            TokenTypes.LITERAL_THROWS,
            TokenTypes.IMPLEMENTS_CLAUSE,
            TokenTypes.TYPE_ARGUMENT,
            TokenTypes.LITERAL_NEW,
            TokenTypes.DOT,
@rnveach

This comment has been minimized.

Copy link
Member

rnveach commented Feb 10, 2019

@pbludov

LITERAL_THROWS
They have no annotation tokens among children.

The java grammar says annotations can appear right after the throws token.
https://github.com/checkstyle/checkstyle/blob/master/src/main/resources/com/puppycrawl/tools/checkstyle/grammar/java.g#L947

Are you still saying there is no way for the check to validate them? Is this just a bug that we need fix suppport for them?
What about the other tokens being removed too?

Example:

@Target(ElementType.TYPE_USE)
@interface Annotation {}

void method() throws @Annotation Exception {
    |  |--LITERAL_THROWS -> throws [2:18]
    |  |  |--ANNOTATIONS -> ANNOTATIONS [2:25]
    |  |  |  `--ANNOTATION -> ANNOTATION [2:25]
    |  |  |      |--AT -> @ [2:25]
    |  |  |      `--IDENT -> Annotation [2:26]
    |  |  `--IDENT -> Exception [2:37]

This code will compile, but no one will use this style.

If the code can compile and the check can create violations on them, then I think they should stay. It is not up to use to say who's style will be used or not. Check creator went through our process to create the check which isn't always easy.

@pbludov

This comment has been minimized.

Copy link
Collaborator Author

pbludov commented Feb 10, 2019

@rnveach can you please spot a repository with code like that:

void method foo() throws
  @Ann("foo")
Exception {
}

?

@romani

This comment has been minimized.

Copy link
Member

romani commented Feb 11, 2019

Annotations are in use over code more and more, especially to do hints to some tools or even use codegeneration. But it is up to us to decide what Check should cover it. It is clear that it not always reasonable to have a check that can do all token support, some cases we can skip also.
Current Check was designed initially to cover declaration tokens, see list of defauls tokens CLASS_DEF, INTERFACE_DEF, ENUM_DEF, METHOD_DEF, CTOR_DEF, VARIABLE_DEF., so all other cases of annotation usage that are not related to declaration blocks (that result in line wrapping rather than related to documentation comment block) or fields declarations probably should be covered by some other Check is users care about this.

Tokens TYPECAST and LITERAL_NEW may have annotations, but they aren't fit to the check

I would say, it is line wrap cases, some another Check should cover such cases if somebody care.

@pbludov , please define strict list of tokens you think should be removed, to make it clear why we remove each token.

@rnveach

This comment has been minimized.

Copy link
Member

rnveach commented Feb 11, 2019

can you please spot a repository with code like that

No because there are too many repositories and too many private projects and such. It still doesn't mean someone would not use it. There are also other options in check to allow some singleline annotations in that scenario.
Even doing a search of the normal regression repositories show that annotations in exceptions is rarely used. http://rveach.no-ip.org/checkstyle/regression/reports/227/ . Only 2 other projects use it and even then only in their tests.

The history of these tokens goes back to #604 (comment) point number 3. They were added without any tests, so that may be why they may or may not work correctly.
Since @romani asked for them to be added, I am fine with them being removed if he wants to go that way.

@pbludov

This comment has been minimized.

Copy link
Collaborator Author

pbludov commented Feb 15, 2019

please define strict list of tokens you think should be removed, to make it clear why we remove each token.

           TokenTypes.TYPECAST,
            TokenTypes.LITERAL_THROWS,
            TokenTypes.IMPLEMENTS_CLAUSE,
            TokenTypes.TYPE_ARGUMENT,
            TokenTypes.LITERAL_NEW,
            TokenTypes.DOT,
  • TYPECAST
( @ann
Foo &
  @Ann
Bar) smth;`
  • LITERAL_THROWS
void method foo() throws
  @Ann("foo")
Exception {
}
  • IMPLEMENTS_CLAUSE
class Foo implements
  @Ann
Bar
  • TYPE_ARGUMENT
class Foo<
  @Ann
Bar>
  • LITERAL_NEW
return
       @Ann
  smth;
  • TokenTypes.DOT ???
    I have no idea what it should be. varargs perhaps?
@romani

This comment has been minimized.

Copy link
Member

romani commented Feb 18, 2019

I am ok to remove such Tokens, I extended comment at #6416 (comment) with details on a reason.

All that tokens are not related to original idea of Check Check enforce to locate annotations immediately after documentation block and before target element, annotation should be located on separate line from target element. ... attention to documentation block.
Line wrapping that is possible with annotation usage should be covered by some other Check, I am not ready even tell about possible design for new Check .... live/users will show us what might be valuable in such validations.

I am ok to remove proposed list of tokens and cause breaking compatibility.

@rnveach , please review this issue one more time and approve it.

@rnveach

This comment has been minimized.

Copy link
Member

rnveach commented Feb 18, 2019

Fix was merged

@rnveach rnveach closed this Feb 18, 2019

@rnveach rnveach added this to the 8.18 milestone Feb 18, 2019

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

@Vampire

This comment has been minimized.

Copy link
Contributor

Vampire commented Feb 22, 2019

@romani with that reasoning applied, shouldn't PARAMETER_DEF also be removed and VARIABLE_DEF only support fields and not local variable definitions? Because both do not have documentation comments in front.

@romani

This comment has been minimized.

Copy link
Member

romani commented Feb 23, 2019

@Vampire ,
Please move this to separate issue, and lets discuss on examples why we need to remove certain token , each token separately.

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.