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

Expand exception violation when haltOnException is off #4350

Closed
rnveach opened this Issue May 12, 2017 · 3 comments

Comments

Projects
None yet
2 participants
@rnveach
Member

rnveach commented May 12, 2017

http://rveach.no-ip.org/checkstyle/regression/reports/44/apache-ant/index.html#A4

Checker Got an exception - java.lang.ArrayIndexOutOfBoundsException: 586

https://github.com/checkstyle/checkstyle/blob/master/src/main/java/com/puppycrawl/tools/checkstyle/Checker.java#L330

$ cat TestClass.java
package com.github.sevntu.checkstyle.checks.coding;

import java.util.HashMap;

public class InputMapIterationInForEachLoopExtends {
    public static class TestMap extends HashMap<Integer, Integer> {
        public void test() {
            for (Entry<Integer, Integer> entry : entrySet()) {
            }
        }
    }
}

$ 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">
    <property name="charset" value="UTF-8"/>
    <property name="haltOnException" value="false"/>

    <module name="TreeWalker">
<module name="com.github.sevntu.checkstyle.checks.coding.MapIterationInForEachLoopCheck">
  <property name="proposeValuesUsage" value="true"/>
  <property name="proposeKeySetUsage" value="true"/>
  <property name="proposeEntrySetUsage" value="true"/>
</module>
    </module>
</module>

$ java -jar checkstyle-7.7-sevntu-1.23.1-all.jar -c TestConfig.xml TestClass.java
Starting audit...
[ERROR] TestClass.java:0: Got an exception - java.lang.NullPointerException: null [Checker]
Audit done.
Checkstyle ends with 1 errors.

Violation message isn't very clear on what the underlying issue is. We don't know what class got the exception or what method. Regression was done on multiple files and checks. It would be very hard to go through each one by hand.

We need to expand the exception violation to be more informative without having to re-run with the option off.

I'm not sure if display should be full 'caused by' stack trace, last non-util checkstyle line in stack trace, or ....

With haltOnException turned on, the output is:

$ java -jar checkstyle-7.7-sevntu-1.23.1-all.jar -c TestConfig.xml TestClass.java
Starting audit...
com.puppycrawl.tools.checkstyle.api.CheckstyleException: Exception was thrown while processing TestClass.java
    at com.puppycrawl.tools.checkstyle.Checker.processFiles(Checker.java:293)
    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)
Caused by: java.lang.NullPointerException
    at com.github.sevntu.checkstyle.checks.coding.MapIterationInForEachLoopCheck.getKeySetOrEntrySetNode(MapIterationInForEachLoopCheck.java:406)
    at com.github.sevntu.checkstyle.checks.coding.MapIterationInForEachLoopCheck.validate(MapIterationInForEachLoopCheck.java:332)
    at com.github.sevntu.checkstyle.checks.coding.MapIterationInForEachLoopCheck.visitToken(MapIterationInForEachLoopCheck.java:307)
    at com.puppycrawl.tools.checkstyle.TreeWalker.notifyVisit(TreeWalker.java:363)
    at com.puppycrawl.tools.checkstyle.TreeWalker.processIter(TreeWalker.java:500)
    at com.puppycrawl.tools.checkstyle.TreeWalker.walk(TreeWalker.java:305)
    at com.puppycrawl.tools.checkstyle.TreeWalker.processFiltered(TreeWalker.java:180)
    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)
    ... 4 more
Checkstyle ends with 1 errors.
@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani May 13, 2017

Member

@rnveach , it is better to update description to reproduce this problem in CLI, I do believe result will be the same.


format of message is ex.getClass().getName() + ": " + ex.getMessage()

from your report:
Got an exception - java.lang.ArrayIndexOutOfBoundsException: 586

586 is exception message ?? interesting case

Got an exception - java.lang.NullPointerException: null

exception message is null , not ideal but happens.


When exception happen and you need to show some details, nothing is more useful than full stacktrace. We will never predict place of exception, so stacktrace is only good content.
We could serialize whole stacktrace to String and put it to Message. I think smb who run checkstyle in mode haltOnException=false should expect non simple messages and should be able to show them correctly.

@rnveach , does it make sense ?

Member

romani commented May 13, 2017

@rnveach , it is better to update description to reproduce this problem in CLI, I do believe result will be the same.


format of message is ex.getClass().getName() + ": " + ex.getMessage()

from your report:
Got an exception - java.lang.ArrayIndexOutOfBoundsException: 586

586 is exception message ?? interesting case

Got an exception - java.lang.NullPointerException: null

exception message is null , not ideal but happens.


When exception happen and you need to show some details, nothing is more useful than full stacktrace. We will never predict place of exception, so stacktrace is only good content.
We could serialize whole stacktrace to String and put it to Message. I think smb who run checkstyle in mode haltOnException=false should expect non simple messages and should be able to show them correctly.

@rnveach , does it make sense ?

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach May 14, 2017

Member

it is better to update description to reproduce this problem in CLI, I do believe result will be the same.

I updated it for the NullPointerException. I have been unable to track down the out of bounds exception right now.

586 is exception message ?? interesting case
exception message is null , not ideal but happens.

Both are right. null will probably be the most common message for exceptions.

We could serialize whole stacktrace to String and put it to Message.
does it make sense ?

Yes, this makes sense. This will be the most ideal for us in our regression reports.

Member

rnveach commented May 14, 2017

it is better to update description to reproduce this problem in CLI, I do believe result will be the same.

I updated it for the NullPointerException. I have been unable to track down the out of bounds exception right now.

586 is exception message ?? interesting case
exception message is null , not ideal but happens.

Both are right. null will probably be the most common message for exceptions.

We could serialize whole stacktrace to String and put it to Message.
does it make sense ?

Yes, this makes sense. This will be the most ideal for us in our regression reports.

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani May 26, 2017

Member

Fix is merged

Member

romani commented May 26, 2017

Fix is merged

@romani romani closed this May 26, 2017

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