Issue#1395 : Dump on error fix #3406

aditya3434 · 2020-06-27T08:11:50Z

Fix for issue #1395 (dump-on-error). Compound checkers such as nullness and index checkers were not giving adequate stack trace to pin-point where the error in the code had occurred. This was because in BaseTypeChecker, errors due to checkers with subcheckers were stored in CheckerMessage but those without were printed right away giving a correct stack trace. This storage caused the stack trace to unwind and take a different path. Since, all errors were stored in the same place, they gave the same stack trace when the -doe option is used. To fix this problem, I added a CheckerMessage constructor to store the stack trace when the error occurred. Then, I added a method to print this stored stack trace for all the stored errors. The code gives the correct stack trace when used with the '-AdumpOnErrors' compiler option.

Fixes #1395.

This reverts commit fd536c4.

wmdietl · 2020-07-01T14:30:44Z

docs/manual/creating-a-checker.tex

@@ -1783,6 +1783,10 @@
 already reported the bug, and you want to continue using the checker on a
 large codebase without being inundated in stack traces.

+\item \code{-AdumpOnErrors}: Outputs the stack trace of the program when the
+Checker Framework encounters an error or warning. The option also requires a
+boolean parameter to enables its functionality (e.g. \code{-AdumpOnErrors=true}).


The boolean shouldn't be required, see the other boolean flags above. Just passing -AdumOnErrors should be equal to -AdumOnErrors=true. The user could turn off the flag using -AdumOnErrors=false.

wmdietl · 2020-07-01T14:34:29Z

framework/src/main/java/org/checkerframework/common/basetype/BaseTypeChecker.java

+        if (dumpOnErrors) {
+            for (StackTraceElement elem : trace) {
+                String msg = "\tat " + elem;
+                message(Diagnostic.Kind.NOTE, msg);


We don't want individual messages for each element. Build up one string, including line breaks, and issue one NOTE.

framework/src/main/java/org/checkerframework/framework/source/SourceChecker.java

wmdietl · 2020-07-01T14:38:13Z

Earlier reviews aditya3434#2

wmdietl · 2020-07-20T20:00:07Z

docs/manual/creating-a-checker.tex

@@ -1783,6 +1783,9 @@
 already reported the bug, and you want to continue using the checker on a
 large codebase without being inundated in stack traces.

+\item \code{-AdumpOnErrors}: Outputs the stack trace of the program when the


I think you can remove the of the program and shorten to output a stack trace when reporting errors or warnings.

I have updated the documentation in my latest commit.

wmdietl · 2020-07-20T20:02:36Z

framework/src/main/java/org/checkerframework/framework/source/SourceChecker.java

@@ -315,6 +316,10 @@
    // org.checkerframework.framework.util.typeinference.DefaultTypeArgumentInference
    "showInferenceSteps",

+    // Output stack trace when checker encounters errors


Use the short message I proposed for the manual.

I have used the short message in my latest commit.

wmdietl · 2020-07-20T20:05:56Z

framework/src/main/java/org/checkerframework/common/basetype/BaseTypeChecker.java

+        }
+
+        /**
+         * Constructor method for checker message if stack trace needs to be stored.


Don't say constructor message. The same Create a new CheckerMessage. as for the other constructor is fine.

Actually, there are no calls of the constructor without a trace, so why have two constructors?

I have updated the summary and removed the first redundant constructor from BaseTypeChecker.

framework/src/main/java/org/checkerframework/framework/source/SourceChecker.java

wmdietl · 2020-07-20T20:11:17Z

framework/src/main/java/org/checkerframework/framework/source/SourceChecker.java

+     */
+    private void printStackTrace(StackTraceElement[] trace) {
+        boolean dumpOnErrors =
+                getOptions().containsKey("dumpOnErrors") && getBooleanOption("dumpOnErrors", true);


Shouldn't this just be getBooleanOption("dumpOnErrors")? That method already checks whether the option is given and false should be the default, shouldn't it?

I originally tried getBooleanOption("dumpOnErrors", false), however it returns false whenever no boolean argument is given. getBooleanOption("dumpOnErrors", true) works for the most part but returns true even if the dumpOnErrors option is not provided. To prevent that, I used the getOptions().containsKey("dumpOnErrors") to check whether the dumpOnErrors option is given.

That's a bit odd, looking at

checker-framework/framework/src/main/java/org/checkerframework/framework/source/SourceChecker.java

Line 1589 in 3498970

if (value == null) {

I wouldn't expect that behavior.
Can you investigate why this doesn't work as specified?

If we write dumpOnErrors without a boolean argument, the value variable in the getBooleanOption() method would be null. Hence, the function would return the default value which, if we used getBooleanOption("dumpOnErrors", false) would be false. So the stack trace won't be printed if we don't give the boolean value true. If we use getBooleanOption("dumpOnErrors", true), then we don't need a boolean argument to print the stack trace, however, then it would print the stack trace even if dumpOnErrors is not provided because value will be null, and it would return the default value which is true.

Is simply hasOption("dumpOnErrors") enough? Then you don't need to introduce a local variable for this.
In a separate PR, can you look into what getBooleanOption is doing wrong? It is hardly used, so we should discuss what the correct behavior is.

hasOption("dumpOnErrors") is the same as getOptions().containtsKey("dumpOnErrors"). If we give the option -AdumpOnErrors=false, hasOption("dumpOnErrors") will return true. Hence, we cannot use this directly. I have replaced getOptions().containtsKey("dumpOnErrors") with hasOption("dumpOnErrors").

wmdietl · 2020-07-20T20:14:04Z

framework/src/main/java/org/checkerframework/framework/source/SourceChecker.java

+        boolean dumpOnErrors =
+                getOptions().containsKey("dumpOnErrors") && getBooleanOption("dumpOnErrors", true);
+        if (dumpOnErrors) {
+            String msg = new String();


Don't build up with just a String! In particular that initial new String() is very ugly.
Use a StringBuilder or a StringJoiner.

I have used a StringBuilder to build the stack trace and print it as a note in my latest commit.

wmdietl · 2020-07-21T22:38:57Z

framework/src/main/java/org/checkerframework/common/basetype/BaseTypeChecker.java

@@ -599,6 +600,8 @@ private void printStoredMessages(CompilationUnitTree unit) {
        final String message;
        /** The source code that the message is about. */
        final @InternedDistinct Tree source;
+        /** Stores the stack trace till the point the checker encounters an error. */
+        final StackTraceElement @Nullable [] trace;


Why is the field @Nullable? Same for the trace parameter that sets the field.
Thread.currentThread().getStackTrace() returns something non-null.

I think the IDE added it automatically when I initialized it. I have removed the annotation since it was, as you said, redundant.

framework/src/main/java/org/checkerframework/framework/source/SourceChecker.java

wmdietl · 2020-07-21T22:49:12Z

framework/src/main/java/org/checkerframework/framework/source/SourceChecker.java

+     */
+    private void printStackTrace(StackTraceElement[] trace) {
+        boolean dumpOnErrors =
+                getOptions().containsKey("dumpOnErrors") && getBooleanOption("dumpOnErrors", true);


That's a bit odd, looking at

checker-framework/framework/src/main/java/org/checkerframework/framework/source/SourceChecker.java

Line 1589 in 3498970

if (value == null) {

I wouldn't expect that behavior.
Can you investigate why this doesn't work as specified?

wmdietl · 2020-07-29T13:49:20Z

framework/src/main/java/org/checkerframework/common/basetype/BaseTypeChecker.java

@@ -599,6 +600,8 @@ private void printStoredMessages(CompilationUnitTree unit) {
        final String message;
        /** The source code that the message is about. */
        final @InternedDistinct Tree source;
+        /** Stores the stack trace till the point the checker encounters an error. */


This is the stack trace when it encountered an error, so this should be something like The stack trace when the message is created.
Also change the parameter documentation to the same.

I have changed the field and parameter description in my latest commit.

wmdietl · 2020-07-29T13:54:58Z

framework/src/main/java/org/checkerframework/framework/source/SourceChecker.java

+    }
+
+    /**
+     * printOrStoreMessage method with an added stack trace argument. The stack trace is printed


Like for other overloaded methods, start out with the normal description of what the method is for and then describe the differences.
Also, @see cross-references can be useful.

I have updated the documentation in my latest commit.

wmdietl · 2020-07-29T13:56:05Z

framework/src/main/java/org/checkerframework/framework/source/SourceChecker.java

+            Tree source,
+            CompilationUnitTree root,
+            StackTraceElement[] trace) {
+        // Not calling printOrStoreMessage(kind, message, source, root) since the error message


That comment is out-of-date - you would now create an infinite loop, so this question no longer arises.

I have deleted the comment in my latest commit.

wmdietl · 2020-07-29T13:58:56Z

framework/src/main/java/org/checkerframework/framework/source/SourceChecker.java

+     */
+    private void printStackTrace(StackTraceElement[] trace) {
+        boolean dumpOnErrors =
+                getOptions().containsKey("dumpOnErrors") && getBooleanOption("dumpOnErrors", true);


Is simply hasOption("dumpOnErrors") enough? Then you don't need to introduce a local variable for this.
In a separate PR, can you look into what getBooleanOption is doing wrong? It is hardly used, so we should discuss what the correct behavior is.

aditya3434 added 21 commits June 11, 2020 13:23

Fix for feature request typetools#3313 in Aliasing Checker

97ff12d

Revert "Fix for feature request typetools#3313 in Aliasing Checker"

629a1a9

This reverts commit fd536c4.

Fix for Issue typetools#1395

deab153

Fix for Issue typetools#1395

c0e6deb

Adding documentation

d1ffe78

Adding dcumentation

11e70d3

Merge remote-tracking branch 'upstream/master'

25b602a

Merge branch 'master' into 1395-dump-on-error-fix

4719fac

Commiting requested changes and documentation

1e1ce11

Fix for feature request typetools#3313 in Aliasing Checker

eb424fd

Revert "Fix for feature request typetools#3313 in Aliasing Checker"

c324094

This reverts commit fd536c4.

Fix for feature request typetools#3313 in Aliasing Checker

941cab0

Revert "Fix for feature request typetools#3313 in Aliasing Checker"

d71f608

This reverts commit fd536c4.

Merge branch 'master' into 1395-dump-on-error-fix

2237a00

Merge remote-tracking branch 'upstream/master'

53a6c12

Merge remote-tracking branch 'upstream/master'

4bfe445

Merge branch 'master' into 1395-dump-on-error-fix

46aef3a

Adding requested changes

e5efd64

Merge remote-tracking branch 'upstream/master'

7b431cb

Merge remote-tracking branch 'upstream/master'

8f4123a

Merge branch 'master' into 1395-dump-on-error-fix

c16f886

aditya3434 requested a review from wmdietl June 27, 2020 09:14

aditya3434 assigned wmdietl and aditya3434 and unassigned aditya3434 Jun 27, 2020

Tweaks

987c560

wmdietl requested changes Jul 1, 2020

View reviewed changes

smillst assigned aditya3434 and unassigned wmdietl Jul 2, 2020

Annotating parameters

e1c8ac2

aditya3434 assigned wmdietl and unassigned aditya3434 Jul 10, 2020

aditya3434 added 2 commits July 11, 2020 14:56

Merge branch 'master' into 1395-dump-on-error-fix

f712249

Merge branch 'master' into 1395-dump-on-error-fix

b615462

wmdietl requested changes Jul 20, 2020

View reviewed changes

wmdietl assigned aditya3434 and unassigned wmdietl Jul 20, 2020

aditya3434 added 2 commits July 21, 2020 15:27

Merge branch 'master' into 1395-dump-on-error-fix

7d44943

Requested changes

3f99303

wmdietl mentioned this pull request Jul 21, 2020

1395 dump on error fix aditya3434/checker-framework#2

Closed

wmdietl requested changes Jul 21, 2020

View reviewed changes

aditya3434 added 4 commits July 22, 2020 14:23

Removing @nullable and adding implementation comment

062a284

Merge branch 'master' into 1395-dump-on-error-fix

4ccbadb

Merge branch 'master' into 1395-dump-on-error-fix

4460e8c

Calling method in Source Checker

7f7ce23

aditya3434 assigned wmdietl and unassigned aditya3434 Jul 24, 2020

Merge branch 'master' into 1395-dump-on-error-fix

b434c02

wmdietl requested changes Jul 29, 2020

View reviewed changes

wmdietl assigned aditya3434 and unassigned wmdietl Jul 29, 2020

aditya3434 added 2 commits July 29, 2020 23:46

Making requested changes

8fafccd

Merge branch 'master' into 1395-dump-on-error-fix

004d5e8

aditya3434 assigned wmdietl and unassigned aditya3434 Jul 30, 2020

Simplify code

ae972ff

wmdietl approved these changes Jul 31, 2020

View reviewed changes

wmdietl merged commit e8be209 into typetools:master Jul 31, 2020

aditya3434 deleted the 1395-dump-on-error-fix branch July 31, 2020 06:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue#1395 : Dump on error fix #3406

Issue#1395 : Dump on error fix #3406

aditya3434 commented Jun 27, 2020 •

edited by wmdietl

wmdietl Jul 1, 2020

wmdietl Jul 1, 2020

wmdietl commented Jul 1, 2020

wmdietl Jul 20, 2020

aditya3434 Jul 21, 2020

wmdietl Jul 20, 2020

aditya3434 Jul 21, 2020

wmdietl Jul 20, 2020

wmdietl Jul 20, 2020

aditya3434 Jul 21, 2020

wmdietl Jul 20, 2020

aditya3434 Jul 21, 2020

wmdietl Jul 21, 2020

aditya3434 Jul 22, 2020

wmdietl Jul 29, 2020

aditya3434 Jul 29, 2020

wmdietl Jul 20, 2020

aditya3434 Jul 21, 2020

wmdietl Jul 21, 2020

aditya3434 Jul 22, 2020

wmdietl Jul 21, 2020

wmdietl Jul 29, 2020

aditya3434 Jul 29, 2020

wmdietl Jul 29, 2020

aditya3434 Jul 29, 2020

wmdietl Jul 29, 2020

aditya3434 Jul 29, 2020

wmdietl Jul 29, 2020

Issue#1395 : Dump on error fix #3406

Issue#1395 : Dump on error fix #3406

Conversation

aditya3434 commented Jun 27, 2020 • edited by wmdietl

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wmdietl commented Jul 1, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aditya3434 commented Jun 27, 2020 •

edited by wmdietl