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

Issue #3655: Fix NPE in NeedBraces on single line default stmt #3673

Closed
wants to merge 1 commit into from

Conversation

liscju
Copy link
Contributor

@liscju liscju commented Dec 21, 2016

Problem in #3655 was that isSingleLineDefault assumed that default stmt
has slist. This commit fixes this by checking if slist exists.

@romani
Copy link
Member

romani commented Dec 21, 2016

Looks like git commit message is multi-line, it should be single line.
Please update PR description to reference issue number.

@codecov-io
Copy link

codecov-io commented Dec 21, 2016

Current coverage is 100% (diff: 100%)

Merging #3673 into master will not change coverage

@@           master   #3673   diff @@
=====================================
  Files         275     275          
  Lines       13571   13573     +2   
  Methods         0       0          
  Messages        0       0          
  Branches     3051    3052     +1   
=====================================
+ Hits        13571   13573     +2   
  Misses          0       0          
  Partials        0       0          

Powered by Codecov. Last update b0972ba...4c51a57

@romani
Copy link
Member

romani commented Dec 21, 2016

good, thanks a lot.

please do the same fix for "isSingleLineCase" it is not referenced in issue test case, but problem is the same.

  1. please launch regression testing for this option "allowSingleLineStatement" for all tokens.
    by https://github.com/checkstyle/contribution/tree/master/checkstyle-tester on all project at https://github.com/checkstyle/contribution/blob/master/checkstyle-tester/projects-to-test-on.properties
    just to make sure we did not left any similar Exceptional case.
    Code change is simple enough, I do not how can we change validation results, so tst for Exceptions only is enough.

@romani
Copy link
Member

romani commented Dec 30, 2016

please do the same fix for "isSingleLineCase" it is not referenced in issue test case, but problem is the same.

It is possible:

 |--CASE_GROUP -> CASE_GROUP [9:12]
 |   `--LITERAL_CASE -> case [9:12]
 |       |--EXPR -> EXPR [9:17]
 |       |   `--NUM_INT -> 1 [9:17]
 |       `--COLON -> : [9:18]
 `--RCURLY -> } [10:8]

@romani
Copy link
Member

romani commented Dec 30, 2016

one more exception on case:

@interface Example {
    String priority() default "value";
}

exception:

java.lang.NullPointerException
	at com.puppycrawl.tools.checkstyle.checks.blocks.NeedBracesCheck.isSingleLineDefault(NeedBracesCheck.java:446)
	at com.puppycrawl.tools.checkstyle.checks.blocks.NeedBracesCheck.isSingleLineStatement(NeedBracesCheck.java:289)
	at com.puppycrawl.tools.checkstyle.checks.blocks.NeedBracesCheck.isSkipStatement(NeedBracesCheck.java:222)
	at com.puppycrawl.tools.checkstyle.checks.blocks.NeedBracesCheck.visitToken(NeedBracesCheck.java:208)
	at com.puppycrawl.tools.checkstyle.TreeWalker.notifyVisit(TreeWalker.java:361)
	at com.puppycrawl.tools.checkstyle.TreeWalker.processIter(TreeWalker.java:498)
	at com.puppycrawl.tools.checkstyle.TreeWalker.walk(TreeWalker.java:303)
	at com.puppycrawl.tools.checkstyle.TreeWalker.processFiltered(TreeWalker.java:178)
	at com.puppycrawl.tools.checkstyle.api.AbstractFileSetCheck.process(AbstractFileSetCheck.java:78)
	at com.puppycrawl.tools.checkstyle.Checker.processFile(Checker.java:314)
	at com.puppycrawl.tools.checkstyle.Checker.processFiles(Checker.java:284)
	at com.puppycrawl.tools.checkstyle.Checker.process(Checker.java:211)
	at com.puppycrawl.tools.checkstyle.Main.runCheckstyle(Main.java:425)
	at com.puppycrawl.tools.checkstyle.Main.runCli(Main.java:359)
	at com.puppycrawl.tools.checkstyle.Main.main(Main.java:174)

ast:

ANNOTATION_DEF -> ANNOTATION_DEF [11:0]
|--MODIFIERS -> MODIFIERS [11:0]
|--AT -> @ [11:0]
|--LITERAL_INTERFACE -> interface [11:1]
|--IDENT -> Example [11:11]
`--OBJBLOCK -> OBJBLOCK [11:19]
    |--LCURLY -> { [11:19]
    |--ANNOTATION_FIELD_DEF -> ANNOTATION_FIELD_DEF [12:4]
    |   |--MODIFIERS -> MODIFIERS [12:4]
    |   |--TYPE -> TYPE [12:4]
    |   |   `--IDENT -> String [12:4]
    |   |--IDENT -> priority [12:11]
    |   |--LPAREN -> ( [12:19]
    |   |--RPAREN -> ) [12:20]
    |   |--LITERAL_DEFAULT -> default [12:22]
    |   |   `--EXPR -> EXPR [12:30]
    |   |       `--STRING_LITERAL -> "value" [12:30]
    |   `--SEMI -> ; [12:37]
    `--RCURLY -> } [13:0]

@romani
Copy link
Member

romani commented Dec 31, 2016

commit was updated and merged in scope of #3682 .

Stress testing was done on all projects in checkstyle-tester

@romani romani closed this Dec 31, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants