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

Control Characters are not skipped with google_checks config #3700

Closed
dimo414 opened this Issue Jan 8, 2017 · 5 comments

Comments

Projects
None yet
4 participants
@dimo414
Contributor

dimo414 commented Jan 8, 2017

/tmp $ javac Demo.java

/tmp $ cat Demo.java
public class Demo {
  public static void main(String[] args) {
    System.out.println("\u0007");
  }
}

/tmp $ java -cp checkstyle-7.4-all.jar com.puppycrawl.tools.checkstyle.Main -c /google_checks.xml Demo.java 
Starting audit...
[WARN] /tmp/Demo.java:3: Unicode escape(s) usage should be avoided. [AvoidEscapedUnicodeCharacters]
Audit done.

Describe what you expect in detail.
I expected this file to pass with no warnings, as \u0007 BEL is a control character, and google_checks.xml permits unicode escapes for such characters.


If I switch from \u0007 to \u0008 BACKSPACE I see a second, even more confusing error:

$ java -cp checkstyle-7.4-all.jar com.puppycrawl.tools.checkstyle.Main -c /google_checks.xml Demo.java 
Starting audit...
[WARN] /tmp/Demo.java:3: Unicode escape(s) usage should be avoided. [AvoidEscapedUnicodeCharacters]
[WARN] /tmp/Demo.java:3:24: Avoid using corresponding octal or Unicode escape. [IllegalTokenText]
Audit done.

It's not clear what I'm supposed to do here.

I'm happy to submit patches to address this, but I'm not certain what the expected behavior is.

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Jan 8, 2017

Member

issue is confirmed.

mapping of google style rule to checkstyle Check - http://checkstyle.sourceforge.net/google_style.html#a2.3.3

Google style config has option "allowEscapesForControlCharacters=true" , https://github.com/search?q=path%3Asrc%2Fmain%2Fresources+filename%3Agoogle_checks.xml+repo%3Acheckstyle%2Fcheckstyle+AvoidEscapedUnicodeCharacters

smth wrong in regexp - https://github.com/checkstyle/checkstyle/blob/master/src/main/java/com/puppycrawl/tools/checkstyle/checks/AvoidEscapedUnicodeCharactersCheck.java#L122


Second problem is misconfiguration, moved to #3701.


@dimo414 , please be welcome with PRs.

Member

romani commented Jan 8, 2017

issue is confirmed.

mapping of google style rule to checkstyle Check - http://checkstyle.sourceforge.net/google_style.html#a2.3.3

Google style config has option "allowEscapesForControlCharacters=true" , https://github.com/search?q=path%3Asrc%2Fmain%2Fresources+filename%3Agoogle_checks.xml+repo%3Acheckstyle%2Fcheckstyle+AvoidEscapedUnicodeCharacters

smth wrong in regexp - https://github.com/checkstyle/checkstyle/blob/master/src/main/java/com/puppycrawl/tools/checkstyle/checks/AvoidEscapedUnicodeCharactersCheck.java#L122


Second problem is misconfiguration, moved to #3701.


@dimo414 , please be welcome with PRs.

@romani romani added the approved label Jan 8, 2017

@romani romani changed the title from Incorrect and/or confusing errors with ASCII unicode escapes to Control Characters are not skipped with google_checks config Jan 9, 2017

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Feb 13, 2017

Member

@romani Should only 0007 be allowed as control character?
Is there someplace that lists all the control characters? I assume we go by this list? https://en.wiktionary.org/wiki/Appendix:Control_characters

Member

rnveach commented Feb 13, 2017

@romani Should only 0007 be allowed as control character?
Is there someplace that lists all the control characters? I assume we go by this list? https://en.wiktionary.org/wiki/Appendix:Control_characters

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Feb 15, 2017

Member

@rnveach , 0007 should be enforced by UTs to show user that it will not happen again.
It will be good to recheck all other characters and enforce them by UTs too.

Member

romani commented Feb 15, 2017

@rnveach , 0007 should be enforced by UTs to show user that it will not happen again.
It will be good to recheck all other characters and enforce them by UTs too.

@Luolc

This comment has been minimized.

Show comment
Hide comment
@Luolc

Luolc Mar 2, 2017

Contributor

I am on it :)

Contributor

Luolc commented Mar 2, 2017

I am on it :)

Luolc added a commit to Luolc/checkstyle that referenced this issue Mar 2, 2017

Luolc added a commit to Luolc/checkstyle that referenced this issue Mar 3, 2017

Luolc added a commit to Luolc/checkstyle that referenced this issue Mar 3, 2017

Luolc added a commit to Luolc/checkstyle that referenced this issue Mar 7, 2017

Luolc added a commit to Luolc/checkstyle that referenced this issue Mar 7, 2017

Luolc added a commit to Luolc/checkstyle that referenced this issue Mar 7, 2017

Luolc added a commit to Luolc/checkstyle that referenced this issue Mar 8, 2017

Luolc added a commit to Luolc/checkstyle that referenced this issue Mar 8, 2017

Luolc added a commit to Luolc/checkstyle that referenced this issue Mar 8, 2017

Luolc added a commit to Luolc/checkstyle that referenced this issue Mar 8, 2017

Luolc added a commit to Luolc/checkstyle that referenced this issue Mar 8, 2017

Luolc added a commit to Luolc/checkstyle that referenced this issue Mar 8, 2017

Luolc added a commit to Luolc/checkstyle that referenced this issue Mar 8, 2017

Luolc added a commit to Luolc/checkstyle that referenced this issue Mar 8, 2017

Luolc added a commit to Luolc/checkstyle that referenced this issue Mar 8, 2017

Luolc added a commit to Luolc/checkstyle that referenced this issue Mar 8, 2017

Luolc added a commit to Luolc/checkstyle that referenced this issue Mar 9, 2017

romani added a commit that referenced this issue Mar 9, 2017

@romani romani added this to the 7.7 milestone Mar 9, 2017

@romani romani added the bug label Mar 9, 2017

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Mar 9, 2017

Member

fix is merged

Member

romani commented Mar 9, 2017

fix is merged

@romani romani closed this Mar 9, 2017

sagar-shah94 pushed a commit to sagar-shah94/checkstyle that referenced this issue Mar 18, 2017

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