Routines to format AnalysisResult objects for debugging/logging #3522

mernst · 2020-07-29T04:45:13Z

~~Incorporates changes from #3527, which should be merged first.~~

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

dataflow/src/main/java/org/checkerframework/dataflow/analysis/AnalysisResult.java

+     *
+     * @return a string representation of this
+     */
+    public String repr() {


Is there a reason to not simply override toString? That would be easier to find for people.

This is much more verbose. It would clutter most output and is useful for debugging. I have updated the description.

At the moment there is no toString method, so currently nothing useful is output at all.
So it's not clear what output this would clutter - at the moment any output is brief but useless.
What output are you concerned about?

It is not the case that nothing useful is output. Object.toString outputs the run-time class name and a unique identifier. Users may prefer that to dozens of lines of output. If toString is overridden, users don't have a way to get less information. Creating a different method gives each user the option of how much output to put in logs.

Another customary difference between toString() and repr() is that toString() reflects the abstraction or client view, whereas repr() shows the implementation or representation. (Not every implementation of toString() and repr() follows the convention perfectly.)

This is the first class where we add a repr method, so I would like to understand what the motivation is and don't understand what you mean with "customary".
I see 483 matches for such a repr function on GitHub for Java code. This seems popular in Python code.

In general it seems more useful to output concrete information instead of what Object.toString gives. If the behavior of Object.toString is really desired in some place, one can simply call getClass and hashCode instead.

So far in similar classes we have overriden toString. What makes these classes special to not do the same?

This is the first class where we add a repr method, so I would like to understand what the motivation is

I added text from this conversation to the developer manual.

In general it seems more useful to output concrete information instead of what Object.toString gives.

It is useful to give clients a choice about what information to output.

So far in similar classes we have overriden toString. What makes these classes special to not do the same?

The difference is not the classes, the difference is the methods.

dataflow/src/main/java/org/checkerframework/dataflow/analysis/AnalysisResult.java

dataflow/src/main/java/org/checkerframework/dataflow/cfg/ControlFlowGraph.java

wmdietl · 2020-07-30T00:10:05Z

dataflow/src/main/java/org/checkerframework/dataflow/cfg/ControlFlowGraph.java

@@ -303,4 +305,32 @@ public String toString() {
        String stringGraph = (String) res.get("stringGraph");
        return stringGraph == null ? super.toString() : stringGraph;


If stringGraph is null, you could use repr instead of the un-helpful super implementation.

That's a change to toString(), and repr() is more about the internal implementation. (Also, this pull request doesn't make any changes to toString().) Should the code instead throw BugInCF if stringGraph is null? A separate pull request could do that.

dataflow/src/main/java/org/checkerframework/dataflow/cfg/ControlFlowGraph.java

…alysisresult-repr

mernst · 2020-07-31T00:49:13Z

@wmdietl Thanks for the feedback. I think I have addressed it (except for a change to an existing toString() routine that I propose to defer to a later pull request). Could you see whether I did so? Thanks.

wmdietl · 2020-07-31T20:04:12Z

dataflow/src/main/java/org/checkerframework/dataflow/analysis/AnalysisResult.java

+        for (Map.Entry<Tree, Set<Node>> entry : treeLookup.entrySet()) {
+            for (Node n : entry.getValue()) {
+                if (!nodeValues.containsKey(n)) {
+                    SystemUtil.sleep(100); // without this, printf output is sometimes interleaved


What printf output is this referring to? Throwing a BugInCF outputs the message using the compiler messager and should happen one at a time. In general, the compiler is single-threaded and a sleep seems very inappropriate.

Is this maybe from running gradle concurrently?
How can I reproduce this issue?

As this checkRep method isn't called anywhere, how about moving this method to a separate PR?

wmdietl · 2020-07-31T20:11:01Z

dataflow/src/main/java/org/checkerframework/dataflow/analysis/AnalysisResult.java

+     *
+     * @return a string representation of this
+     */
+    public String repr() {


This is the first class where we add a repr method, so I would like to understand what the motivation is and don't understand what you mean with "customary".
I see 483 matches for such a repr function on GitHub for Java code. This seems popular in Python code.

In general it seems more useful to output concrete information instead of what Object.toString gives. If the behavior of Object.toString is really desired in some place, one can simply call getClass and hashCode instead.

So far in similar classes we have overriden toString. What makes these classes special to not do the same?

wmdietl

For classes that do not have a toString method, I'm not convinced that adding only a repr method will be the most intuitive behavior, but I'm fine with this (modulo the inconsistency in javadoc).

wmdietl · 2020-08-04T22:39:44Z

dataflow/src/main/java/org/checkerframework/dataflow/cfg/node/Node.java

@@ -157,4 +158,32 @@ public void setAssignmentContext(AssignmentContext assignmentContext) {
        }
        return transitiveOperands;
    }
+
+    /**
+     * Returns a verbose printed representation of this, useful for debugging.


For consistency, in a few places in this PR, I would replace the printed with string.

I agree. That was for consistency with existing methods' documentation, but I have changed those too.

mernst · 2020-08-05T01:47:49Z

@wmdietl Would you prefer that I rename all the repr methods to debugToString? That would be consistent with naming in some other parts of the Checker Framework. And it's less concise and therefore less obscure.

mernst added 4 commits July 28, 2020 18:23

Routines to format AnalysisResult objects

90cdc60

Merge ../checker-framework-branch-master into analysisresult-repr

62376b7

Fix issues

c057cdf

Javadoc fixes

fe18877

mernst assigned wmdietl Jul 29, 2020

mernst mentioned this pull request Jul 29, 2020

Use .equals() for nodeValues if == does not work; fixes #3484 #3523

Closed

mernst added 3 commits July 29, 2020 13:49

Add a repr() method for ControlFlowGraph

702f803

Add SystemUtil.sleep method

2f6efa5

Remove stray character

d1045e1

wmdietl requested changes Jul 30, 2020

View reviewed changes

wmdietl assigned mernst and unassigned wmdietl Jul 30, 2020

mernst added 4 commits July 30, 2020 10:58

Address code review feedback

4c9b3cd

Merge ../checker-framework-branch-master into analysisresult-repr

b4a3db7

Merge ../checker-framework-fork-mernst-branch-javacutil-sleep into an…

3e37f6f

…alysisresult-repr

Fix import statement

31f73b8

mernst assigned wmdietl and unassigned mernst Jul 30, 2020

mernst added 3 commits July 30, 2020 11:34

Merge ../checker-framework-branch-master into javacutil-sleep

cf227e1

Fix typo, remove throw

ffc8fc9

Merge ../checker-framework-fork-mernst-branch-javacutil-sleep into an…

b5f101b

…alysisresult-repr

wmdietl assigned mernst and unassigned wmdietl Jul 31, 2020

mernst added 4 commits July 30, 2020 17:39

Address code review feedback

1f01fcd

Merge ../checker-framework-branch-master into analysisresult-repr

6b00d1e

More changes from code review

6d76803

Merge ../checker-framework-branch-master into analysisresult-repr

0453ce1

mernst assigned wmdietl and unassigned mernst Jul 31, 2020

Merge ../checker-framework-branch-master into analysisresult-repr

20f244c

wmdietl requested changes Jul 31, 2020

View reviewed changes

wmdietl assigned mernst and unassigned wmdietl Jul 31, 2020

mernst added 4 commits July 31, 2020 14:40

Merge ../checker-framework-branch-master into analysisresult-repr

6b2cec3

Remove AnalysisResult.checkRep

ca60260

Merge ../checker-framework-branch-master into analysisresult-repr

6e10edb

Explain repr()

835d152

mernst assigned wmdietl and unassigned mernst Jul 31, 2020

mernst added 4 commits July 31, 2020 16:28

Tweak wording

f0fc6cc

Fix HTML tag

cbdde4b

Merge ../checker-framework-branch-master into analysisresult-repr

4bcb7ef

Merge ../checker-framework-branch-master into analysisresult-repr

49548c0

wmdietl approved these changes Aug 4, 2020

View reviewed changes

wmdietl assigned mernst and unassigned wmdietl Aug 4, 2020

mernst added 2 commits August 4, 2020 18:45

Merge ../checker-framework-branch-master into analysisresult-repr

06a1b9a

"printed" => "string"

bc2dc10

mernst added 4 commits August 4, 2020 20:34

"repr()" => "toStringDebug()"

1eb6ddd

Add Javadoc

9741aff

Merge ../checker-framework-branch-master into analysisresult-repr

3e84c67

Fix Javadoc

07a4313

mernst merged commit a741394 into typetools:master Aug 5, 2020

mernst deleted the analysisresult-repr branch August 5, 2020 19:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Routines to format AnalysisResult objects for debugging/logging #3522

Routines to format AnalysisResult objects for debugging/logging #3522

mernst commented Jul 29, 2020 •

edited

wmdietl Jul 29, 2020

mernst Jul 30, 2020

wmdietl Jul 30, 2020

mernst Jul 31, 2020

mernst Jul 31, 2020

wmdietl Jul 31, 2020

mernst Jul 31, 2020

wmdietl Jul 30, 2020

mernst Jul 31, 2020

mernst commented Jul 31, 2020

wmdietl Jul 31, 2020

wmdietl Jul 31, 2020

wmdietl left a comment

wmdietl Aug 4, 2020

mernst Aug 5, 2020

mernst commented Aug 5, 2020

		@@ -303,4 +305,32 @@ public String toString() {
		String stringGraph = (String) res.get("stringGraph");
		return stringGraph == null ? super.toString() : stringGraph;

Routines to format AnalysisResult objects for debugging/logging #3522

Routines to format AnalysisResult objects for debugging/logging #3522

Conversation

mernst commented Jul 29, 2020 • edited

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

mernst commented Jul 31, 2020

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

mernst commented Aug 5, 2020

mernst commented Jul 29, 2020 •

edited