Support LITERAL_SYNCHRONIZED token for NoWhitespaceAfter Rule #2803

Closed
elingg opened this Issue Jan 5, 2016 · 5 comments

Comments

Projects
None yet
3 participants
@elingg

elingg commented Jan 5, 2016

I would like to create a rule to make sure there is no space before the left param for synchronized, i.e. I would like to have the style synchronized(this) instead of synchronized (this) as this is common syntax. Adding the LITERAL_SYNCHRONIZED token to NoWhitespaceAfter(http://checkstyle.sourceforge.net/config_whitespace.html#NoWhitespaceAfter) would address this issue.

Problem is in set of Allowed tokens - https://github.com/checkstyle/checkstyle/blob/master/src/main/java/com/puppycrawl/tools/checkstyle/checks/whitespace/NoWhitespaceAfterCheck.java#L102

See the discussion on the mailing list here, https://groups.google.com/forum/#!topic/checkstyle/cgNOY2uJDAY.

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Jan 8, 2016

Member

here aa10329 Check was allowed to have any token validation (2002 year).

list of strict token set is restored at 103a206 (2006 year)

so we did not change it recently ... that is good.
Still not clear why this Check have limitation of AllowedTokens at all. We need to investigate how much harm there would be to allow user define any token.

synchorized looks like a method declaration inside a method, that is called right-away

    public void inc2() {
        synchronized(lock2) {
            c2++;
        }
    }

Validation of no spaces between method name and list of arguments is done by http://checkstyle.sourceforge.net/config_whitespace.html#MethodParamPad
message like '(' is preceded with whitespace. [MethodParamPad] . That Check works for method declaration and method usage cases.

MethodParamPad is too specific to method to be reused, it will not be clear for user that such could do this.

Resolution:
We need to update NoWhitespaceAfter to add LITERAL_SYNCHRONIZED token, token also need to be added to getDefaultTokens.

Member

romani commented Jan 8, 2016

here aa10329 Check was allowed to have any token validation (2002 year).

list of strict token set is restored at 103a206 (2006 year)

so we did not change it recently ... that is good.
Still not clear why this Check have limitation of AllowedTokens at all. We need to investigate how much harm there would be to allow user define any token.

synchorized looks like a method declaration inside a method, that is called right-away

    public void inc2() {
        synchronized(lock2) {
            c2++;
        }
    }

Validation of no spaces between method name and list of arguments is done by http://checkstyle.sourceforge.net/config_whitespace.html#MethodParamPad
message like '(' is preceded with whitespace. [MethodParamPad] . That Check works for method declaration and method usage cases.

MethodParamPad is too specific to method to be reused, it will not be clear for user that such could do this.

Resolution:
We need to update NoWhitespaceAfter to add LITERAL_SYNCHRONIZED token, token also need to be added to getDefaultTokens.

@mkordas

This comment has been minimized.

Show comment
Hide comment
@mkordas

mkordas Jan 8, 2016

Contributor

@romani I would suggest just providing test case for each token. I imagine that there might me some tokens where this issue does not make sense, like e.g SLIST... So we should extend that list but in reasonable way.

Contributor

mkordas commented Jan 8, 2016

@romani I would suggest just providing test case for each token. I imagine that there might me some tokens where this issue does not make sense, like e.g SLIST... So we should extend that list but in reasonable way.

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Jan 8, 2016

Member

@mkordas , the reason of AllowedTokens collection is to restrict user from usage token that will break Check logic and cause him to throw Exceptions or produce false-positives.
This set does not have intend to limit user to be crazy and configure Check to demand from code weird style - it is problems of user , Checkstyle just follow instructions from configuration.

In short: misconfiguration is problem of user, exceptions and incorrect validation is Checkstyle problems.

But I do not want to allow all tokens in this Check for now.

Member

romani commented Jan 8, 2016

@mkordas , the reason of AllowedTokens collection is to restrict user from usage token that will break Check logic and cause him to throw Exceptions or produce false-positives.
This set does not have intend to limit user to be crazy and configure Check to demand from code weird style - it is problems of user , Checkstyle just follow instructions from configuration.

In short: misconfiguration is problem of user, exceptions and incorrect validation is Checkstyle problems.

But I do not want to allow all tokens in this Check for now.

@romani romani added the approved label Jan 8, 2016

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Jan 8, 2016

Member

@elingg , if you have time and desire to help, please update Check code and UTs for this case, and xdoc files to have on our website example of how to do this (users like examples).

Google style configuration need to be updated, see http://checkstyle.sourceforge.net/google_style.html, NoWhitespaceAfter need to be used in default configuration to cover "4.6.2" point.

you are welcome with PR.

Member

romani commented Jan 8, 2016

@elingg , if you have time and desire to help, please update Check code and UTs for this case, and xdoc files to have on our website example of how to do this (users like examples).

Google style configuration need to be updated, see http://checkstyle.sourceforge.net/google_style.html, NoWhitespaceAfter need to be used in default configuration to cover "4.6.2" point.

you are welcome with PR.

rnveach added a commit to rnveach/checkstyle that referenced this issue Dec 12, 2016

rnveach added a commit to rnveach/checkstyle that referenced this issue Dec 12, 2016

romani added a commit that referenced this issue Dec 12, 2016

@romani romani added the new feature label Dec 12, 2016

@romani romani added this to the 7.4 milestone Dec 12, 2016

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Dec 12, 2016

Member

Fix is merged

Member

romani commented Dec 12, 2016

Fix is merged

@romani romani closed this Dec 12, 2016

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