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#1395 : Dump on error fix #3406

Merged
merged 43 commits into from
Jul 31, 2020
Merged
Show file tree
Hide file tree
Changes from 33 commits
Commits
Show all changes
43 commits
Select commit Hold shift + click to select a range
97ff12d
Fix for feature request #3313 in Aliasing Checker
aditya3434 May 26, 2020
629a1a9
Revert "Fix for feature request #3313 in Aliasing Checker"
aditya3434 May 28, 2020
deab153
Fix for Issue #1395
aditya3434 Jun 16, 2020
c0e6deb
Fix for Issue #1395
aditya3434 Jun 16, 2020
d1ffe78
Adding documentation
aditya3434 Jun 16, 2020
11e70d3
Adding dcumentation
aditya3434 Jun 16, 2020
25b602a
Merge remote-tracking branch 'upstream/master'
aditya3434 Jun 16, 2020
4719fac
Merge branch 'master' into 1395-dump-on-error-fix
aditya3434 Jun 16, 2020
1e1ce11
Commiting requested changes and documentation
aditya3434 Jun 17, 2020
eb424fd
Fix for feature request #3313 in Aliasing Checker
aditya3434 May 26, 2020
c324094
Revert "Fix for feature request #3313 in Aliasing Checker"
aditya3434 May 28, 2020
941cab0
Fix for feature request #3313 in Aliasing Checker
aditya3434 May 26, 2020
d71f608
Revert "Fix for feature request #3313 in Aliasing Checker"
aditya3434 May 28, 2020
2237a00
Merge branch 'master' into 1395-dump-on-error-fix
aditya3434 Jun 17, 2020
53a6c12
Merge remote-tracking branch 'upstream/master'
aditya3434 Jun 19, 2020
4bfe445
Merge remote-tracking branch 'upstream/master'
aditya3434 Jun 26, 2020
46aef3a
Merge branch 'master' into 1395-dump-on-error-fix
aditya3434 Jun 26, 2020
e5efd64
Adding requested changes
aditya3434 Jun 26, 2020
7b431cb
Merge remote-tracking branch 'upstream/master'
aditya3434 Jun 26, 2020
8f4123a
Merge remote-tracking branch 'upstream/master'
aditya3434 Jun 27, 2020
c16f886
Merge branch 'master' into 1395-dump-on-error-fix
aditya3434 Jun 27, 2020
987c560
Tweaks
wmdietl Jul 1, 2020
f7cb763
Merge branch 'master' into 1395-dump-on-error-fix
aditya3434 Jul 3, 2020
8de96f8
Removing multiple note messages and boolean argument
aditya3434 Jul 3, 2020
80ac56b
Merge branch '1395-dump-on-error-fix' of https://github.com/aditya343…
aditya3434 Jul 3, 2020
d11fcf7
Merge branch 'master' into 1395-dump-on-error-fix
aditya3434 Jul 4, 2020
c0d2ff5
Modifying documentation and requested changes
aditya3434 Jul 4, 2020
340c22d
Resolving merge conflicts
aditya3434 Jul 10, 2020
c82f4b5
Resolving merge conflicts
aditya3434 Jul 10, 2020
42aeb4f
Shifting printStackTrace method to SourceChecker.java
aditya3434 Jul 10, 2020
e1c8ac2
Annotating parameters
aditya3434 Jul 10, 2020
f712249
Merge branch 'master' into 1395-dump-on-error-fix
aditya3434 Jul 11, 2020
b615462
Merge branch 'master' into 1395-dump-on-error-fix
aditya3434 Jul 17, 2020
7d44943
Merge branch 'master' into 1395-dump-on-error-fix
aditya3434 Jul 21, 2020
3f99303
Requested changes
aditya3434 Jul 21, 2020
062a284
Removing @Nullable and adding implementation comment
aditya3434 Jul 22, 2020
4ccbadb
Merge branch 'master' into 1395-dump-on-error-fix
aditya3434 Jul 22, 2020
4460e8c
Merge branch 'master' into 1395-dump-on-error-fix
aditya3434 Jul 24, 2020
7f7ce23
Calling method in Source Checker
aditya3434 Jul 24, 2020
b434c02
Merge branch 'master' into 1395-dump-on-error-fix
aditya3434 Jul 27, 2020
8fafccd
Making requested changes
aditya3434 Jul 29, 2020
004d5e8
Merge branch 'master' into 1395-dump-on-error-fix
aditya3434 Jul 29, 2020
ae972ff
Simplify code
wmdietl Jul 30, 2020
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 3 additions & 0 deletions docs/manual/creating-a-checker.tex
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have updated the documentation in my latest commit.

Checker Framework encounters an error or warning.

\end{itemize}


Expand Down
1 change: 1 addition & 0 deletions docs/manual/introduction.tex
Original file line number Diff line number Diff line change
Expand Up @@ -746,6 +746,7 @@
\<-AprintAllQualifiers>,
\<-AprintVerboseGenerics>,
\<-Anomsgtext>
\<-AdumpOnErrors>
Amount of detail in messages; see Section~\ref{creating-debugging-options-detail}.

\item
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -568,10 +568,11 @@ protected void warnUnneededSuppressions() {
protected void printOrStoreMessage(
Diagnostic.Kind kind, String message, Tree source, CompilationUnitTree root) {
assert this.currentRoot == root;
StackTraceElement[] trace = Thread.currentThread().getStackTrace();
if (messageStore == null) {
super.printOrStoreMessage(kind, message, source, root);
super.printOrStoreMessage(kind, message, source, root, trace);
} else {
CheckerMessage checkerMessage = new CheckerMessage(kind, message, source, this);
CheckerMessage checkerMessage = new CheckerMessage(kind, message, source, this, trace);
messageStore.add(checkerMessage);
}
}
Expand All @@ -586,7 +587,7 @@ protected void printOrStoreMessage(
private void printStoredMessages(CompilationUnitTree unit) {
if (messageStore != null) {
for (CheckerMessage msg : messageStore) {
super.printOrStoreMessage(msg.kind, msg.message, msg.source, unit);
super.printOrStoreMessage(msg.kind, msg.message, msg.source, unit, msg.trace);
}
}
}
Expand All @@ -599,6 +600,8 @@ private static class CheckerMessage {
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. */
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

final StackTraceElement @Nullable [] trace;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


/**
* The checker that issued this message. The compound checker that depends on this checker
Expand All @@ -619,10 +622,29 @@ private CheckerMessage(
String message,
@FindDistinct Tree source,
@FindDistinct BaseTypeChecker checker) {
this(kind, message, source, checker, null);
}

/**
* Constructor method for checker message if stack trace needs to be stored.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

*
* @param kind kind of diagnostic, for example, error or warning
* @param message error message that needs to be printed
* @param source tree element causing the error
* @param checker the type-checker in use
* @param trace the stack trace when the checker encounters an error
*/
private CheckerMessage(
Diagnostic.Kind kind,
String message,
@FindDistinct Tree source,
@FindDistinct BaseTypeChecker checker,
StackTraceElement @Nullable [] trace) {
this.kind = kind;
this.message = message;
this.source = source;
this.checker = checker;
this.trace = trace;
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@
import javax.lang.model.element.TypeElement;
import javax.lang.model.util.Elements;
import javax.lang.model.util.Types;
import javax.tools.Diagnostic;
import javax.tools.Diagnostic.Kind;
import org.checkerframework.checker.compilermsgs.qual.CompilerMessageKey;
import org.checkerframework.checker.interning.qual.InternedDistinct;
Expand Down Expand Up @@ -315,6 +316,10 @@
// org.checkerframework.framework.util.typeinference.DefaultTypeArgumentInference
"showInferenceSteps",

// Output stack trace when checker encounters errors
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use the short message I proposed for the manual.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have used the short message in my latest commit.

// org.checkerframework.common.basetype.SourceChecker.printStackTrace()
"dumpOnErrors",
wmdietl marked this conversation as resolved.
Show resolved Hide resolved

/// Visualizing the CFG

// Implemented in the wrapper rather than this file, but worth noting here.
Expand Down Expand Up @@ -1079,6 +1084,43 @@ protected void printOrStoreMessage(
Trees.instance(processingEnv).printMessage(kind, message, source, root);
}

/**
* printOrStoreMessage method with an added stack trace argument. The stack trace is printed
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have updated the documentation in my latest commit.

* when the dumpOnErrors option is enabled.
*
* @param kind the kind of message to print
* @param message the message text
* @param source the souce code position of the diagnostic message
* @param root the compilation unit
* @param trace the stack trace where the checker encountered an error
*/
protected void printOrStoreMessage(
wmdietl marked this conversation as resolved.
Show resolved Hide resolved
javax.tools.Diagnostic.Kind kind,
String message,
Tree source,
CompilationUnitTree root,
StackTraceElement[] trace) {
Trees.instance(processingEnv).printMessage(kind, message, source, root);
wmdietl marked this conversation as resolved.
Show resolved Hide resolved
printStackTrace(trace);
}

/**
* Output the given stack trace if the "dumpOnErrors" option is enabled.
*
* @param trace stack trace when the checker encountered a warning/error
*/
private void printStackTrace(StackTraceElement[] trace) {
boolean dumpOnErrors =
getOptions().containsKey("dumpOnErrors") && getBooleanOption("dumpOnErrors", true);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a bit odd, looking at

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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").

if (dumpOnErrors) {
String msg = new String();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

///////////////////////////////////////////////////////////////////////////
/// Diagnostic message formatting
///
Expand Down