1395 dump on error fix #2

aditya3434 · 2020-06-16T15:02:01Z

Fix for issue typetools#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.

This reverts commit fd536c4.

wmdietl

Thanks for fixing this issue!

wmdietl · 2020-06-16T23:12:00Z

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

@@ -565,10 +565,13 @@ protected void warnUnneededSuppressions() {
    protected void printOrStoreMessage(
            Diagnostic.Kind kind, String message, Tree source, CompilationUnitTree root) {
        assert this.currentRoot == root;
+        StackTraceElement[] trace = new RuntimeException().getStackTrace();


Use Thread.getStackTrace() https://docs.oracle.com/javase/7/docs/api/java/lang/Thread.html#getStackTrace() instead of creating that exception object.

I have used Thread.currentThread().getStackTrace() to acquire the stack trace.

wmdietl · 2020-06-16T23:13:20Z

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

        if (messageStore == null) {
            super.printOrStoreMessage(kind, message, source, root);
+            this.printStackTrace(trace);


You don't need the explicit this in these calls.

wmdietl · 2020-06-16T23:15:53Z

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

+     */
+    private void printStackTrace(StackTraceElement[] trace) {
+        boolean dumpOnErrors =
+                (processingEnv != null


Instead of all this you can simply call getBooleanOption? https://github.com/typetools/checker-framework/blob/master/framework/src/main/java/org/checkerframework/framework/source/SourceChecker.java#L1591

I have used getBooleanOption instead of the previous logic, however, in this one, I have to provide a boolean value to the command line option for the flag to function.
Ex : javac -AdumpOnErrors=true -processor org.checkerframework.checker.nullness.NullnessChecker -implicit:class Test.java

wmdietl · 2020-06-16T23:16:50Z

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

+                        && processingEnv.getOptions() != null
+                        && processingEnv.getOptions().containsKey("dumpOnErrors"));
+        if (dumpOnErrors) {
+            System.err.println(new RuntimeException().toString());


You don't want this output, as it's a stacktrace in this same location for every error.

I have removed this statement in my latest commit

wmdietl · 2020-06-16T23:21:50Z

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

+        if (dumpOnErrors) {
+            System.err.println(new RuntimeException().toString());
+            for (StackTraceElement elem : trace) {
+                System.err.println("\tat " + elem);


Don't use System.err for any compiler output, always use the proper messager.
I would suggest using https://github.com/typetools/checker-framework/blob/master/framework/src/main/java/org/checkerframework/framework/source/SourceChecker.java#L1058 with diagnostic kind NOTE.
As you also have the diagnostic source, you could also use that location for the warning. How does doe output the message?

I have used the given function to print the stack trace. I observed the -doe outputs using a printWriter in the Log.java file.

wmdietl · 2020-06-16T23:22:01Z

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

@@ -600,6 +623,17 @@ private void printStoredMessages(CompilationUnitTree unit) {
         */
        final BaseTypeChecker checker;

+        /** Stores the stack trace till the point the checker encounters an error. */
+        StackTraceElement[] trace;


Make final.

The field can be null, right? So it should be StackTraceElement @Nullable [] trace.

I have made the changes in my latest commit

wmdietl · 2020-06-16T23:22:43Z

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

+         * @param message error message that needs to be printed
+         * @param source tree element causing the error
+         * @param checker the type-checker in use
+         */
        private CheckerMessage(
                Diagnostic.Kind kind, String message, Tree source, BaseTypeChecker checker) {
            this.kind = kind;


This constructor should call the other constructor, providing null for the trace.

I have made the changes in my latest commit

wmdietl · 2020-06-16T23:23:26Z

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

+                String message,
+                Tree source,
+                BaseTypeChecker checker,
+                StackTraceElement[] trace) {


Also nullable.

I have made the changes in my latest commit

wmdietl · 2020-06-16T23:25:59Z

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

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

+    // Output stack trace when checker encounters errors
+    // org.checkerframework.common.basetype.BaseTypeChecker.printStackTrace()
+    "dumpOnErrors",


You should also add documentation to the manual.
There is already a comment on doe https://github.com/typetools/checker-framework/blob/d11aeaedaf4c039beb648ee0e3c177f894c060df/docs/manual/introduction.tex#L835
There you should mention this option.

You also need to document the feature itself, here in the introduction (find the appropriate place there) and in the corresponding section.

I added the documentation in the introduction.tex file in the debugging section.

This reverts commit fd536c4.

wmdietl

This is still assigned to you, so I wasn't sure whether it's ready for another review. Find a few comments. Please move to typetools to not drop the ball on this.

wmdietl · 2020-06-25T13:07:47Z

docs/manual/introduction.tex

@@ -792,6 +792,10 @@
 Print information about the git repository from which the Checker Framework
 was compiled.

+\item
+ \<-AdumpOnErrors>


Merge this with the amount of detail section earlier in this document and then add the details in \ref{creating-debugging-options-detail}.

I have added the documentation in their corresponding sections in my latest commit

wmdietl · 2020-06-25T13:09:01Z

docs/manual/introduction.tex

@@ -792,6 +792,10 @@
 Print information about the git repository from which the Checker Framework
 was compiled.

+\item
+ \<-AdumpOnErrors>
+Prints the stack trace when the checker framework encounters an error.


checker framework should be Checker Framework to be consistent.
Also output a stack trace when errors or warnings are reported.

I have made the changes in my latest commit

wmdietl · 2020-06-25T13:11:04Z

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

+    }
+
+    /**
+     * Prints given stack trace of runtime exception if -AdumpOnErrors flag is passed as compilation


Output the given stack trace if the "dumpOnErrors" option is enabled.

I have made the changes in my latest commit

wmdietl · 2020-06-25T13:12:22Z

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

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

+    // Output stack trace when checker encounters errors
+    // org.checkerframework.common.basetype.BaseTypeChecker.printStackTrace()


Shouldn't the option be handled in this class, in method printOrStoreMessage, too?

I thought we had to mention the place where the dumpOnErrors option was being checked. Since I was checking for the option in the printStackTrace() method, I wrote its path. The option is not used at all in the printOrStoreMessage (only the printStackTrace() method is called). Currently, I have not made any changes. Please let me know what you think I should do.

…4/checker-framework into 1395-dump-on-error-fix

wmdietl · 2020-07-21T22:31:47Z

Now that there is typetools#3406 don't you want to close this PR?

aditya3434 added 8 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

wmdietl suggested changes Jun 16, 2020

View reviewed changes

aditya3434 added 5 commits June 17, 2020 20:48

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.

aditya3434 force-pushed the master branch from 25b602a to d71f608 Compare June 17, 2020 16:21

aditya3434 added 2 commits June 17, 2020 21:54

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

2237a00

Merge remote-tracking branch 'upstream/master'

53a6c12

aditya3434 self-assigned this Jun 21, 2020

wmdietl suggested changes Jun 25, 2020

View reviewed changes

aditya3434 added 6 commits June 26, 2020 18:05

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 force-pushed the master branch from 8f4123a to a130bd6 Compare June 27, 2020 06:32

Tweaks

987c560

wmdietl mentioned this pull request Jul 1, 2020

Issue#1395 : Dump on error fix typetools/checker-framework#3406

Merged

aditya3434 added 2 commits July 3, 2020 19:16

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

f7cb763

Removing multiple note messages and boolean argument

8de96f8

aditya3434 added 11 commits July 3, 2020 19:18

Merge branch '1395-dump-on-error-fix' of https://github.com/aditya343…

80ac56b

…4/checker-framework into 1395-dump-on-error-fix

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

d11fcf7

Modifying documentation and requested changes

c0d2ff5

Resolving merge conflicts

340c22d

Resolving merge conflicts

c82f4b5

Shifting printStackTrace method to SourceChecker.java

42aeb4f

Annotating parameters

e1c8ac2

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

f712249

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

b615462

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

7d44943

Requested changes

3f99303

aditya3434 added 2 commits July 22, 2020 14:23

Removing @nullable and adding implementation comment

062a284

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

4ccbadb

aditya3434 closed this Jul 22, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

1395 dump on error fix #2

1395 dump on error fix #2

aditya3434 commented Jun 16, 2020

wmdietl left a comment

wmdietl Jun 16, 2020

aditya3434 Jun 17, 2020

wmdietl Jun 16, 2020

wmdietl Jun 16, 2020

aditya3434 Jun 17, 2020

wmdietl Jun 16, 2020

aditya3434 Jun 17, 2020

wmdietl Jun 16, 2020

aditya3434 Jun 17, 2020

wmdietl Jun 16, 2020

wmdietl Jun 16, 2020

aditya3434 Jun 17, 2020

wmdietl Jun 16, 2020

aditya3434 Jun 17, 2020

wmdietl Jun 16, 2020

aditya3434 Jun 17, 2020

wmdietl Jun 16, 2020

aditya3434 Jun 17, 2020

wmdietl left a comment

wmdietl Jun 25, 2020

aditya3434 Jun 26, 2020

wmdietl Jun 25, 2020

aditya3434 Jun 26, 2020

wmdietl Jun 25, 2020

aditya3434 Jun 26, 2020

wmdietl Jun 25, 2020

aditya3434 Jun 26, 2020

wmdietl commented Jul 21, 2020

1395 dump on error fix #2

1395 dump on error fix #2

Conversation

aditya3434 commented Jun 16, 2020

wmdietl left a 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

wmdietl left a 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

wmdietl commented Jul 21, 2020