Fix for circular references #659

JimSuplizio · 2018-10-04T20:29:50Z

What does this PR do?

There was an issue where circular references weren't being hooked up correctly. This was causing the value of the name/value pair displayed in VS Code to display the wrong thing. Initially I'd opted stop hooking things up when there was a circular reference but this ended leaving a dangling reference. This also caused the value, when it was computed, to be incorrect as well. Right now, with the way it's hooked up it'll mimic the live debugger which is to say that you can just open up the variables until rapture. There is a slight difference in the replay debugger vs live debugger here. The live debugger does something special when it encounters a circular reference and puts it in braces (see picture) instead of keeping it in the same list/variable format as non-recusive references.

The replay-debugger keeps the live debugging format but still identifies the circular dependency. Mainly because the code to special case and add braces would be painful to deal with (the live debugger has a bit more leniency when dealing with variables since it doesn't have to compute the values from heap dump extent entries.

What issues does this PR fix or reference?

@W-5491876@

JimSuplizio · 2018-10-04T20:33:48Z

packages/salesforcedx-apex-replay-debugger/src/core/heapDumpService.ts

@@ -298,7 +301,8 @@ export class HeapDumpService {
  // cloned into a new variable that has a variable name.
  private copyReferenceContainer(
    refContainer: ApexVariableContainer,
-    varName: string
+    varName: string,
+    createVarRef = true


This was necessary because the variablesRef (which is what tells VS Code that the variable can be expanded) didn't need to be recreated for a variable that's part of the circular reference. The reason cloning was necessary was due to the name changing. For instance local var MyVar of type Foo has field that's a List and we add MyVar to its own list. The name of the local variable is MyVar but once added to the list the name is '0' or whatever number in the list it is.

vazexqi · 2018-10-04T20:40:31Z

...salesforcedx-apex-replay-debugger/test/unit/adapter/apexReplayDebugVariablesHandling.test.ts

@@ -636,6 +637,71 @@ describe('Replay debugger adapter variable handling - unit', () => {
        ) as ApexVariableContainer;
        expect(updatedNonRefVariable.value).to.be.eq('5');
      });
+
+      it('Should correctly deal with circular references and variable values', () => {


Please clean up needless comments.

Nothing in that test is a 'needless' comment. Please be specific about which ones you think are needless.

JimSuplizio · 2018-10-04T20:40:48Z

packages/salesforcedx-apex-replay-debugger/src/core/heapDumpService.ts

@@ -597,24 +630,44 @@ export class HeapDumpService {
  public createVariableFromReference(
    varName: string,
    refVariable: ApexVariableContainer,
-    visitedMap: Map<string, null>
+    visitedMap: Map<string, ApexVariableContainer | null>,
+    updateAfterVarCreation: ApexVariableContainer[]


UpdateAfterVarCreation is used in the finally clause. I ended up getting into a chicken and egg situation here and some variable values cannot be updated until the variable is finished being created. This is only applicable to circular references. createVariableFromReference is a recursive function and because of the visitedMap we when we remove the last entry we know we're done with the initial variable being created. It's at this that time it's safe to update the values of any variables that we couldn't update during creation because we know the variable is complete. Trying to create the value earlier, when the variable was incomplete, would end up creating a value that was also incomplete.

vazexqi · 2018-10-04T20:43:46Z

packages/salesforcedx-apex-replay-debugger/src/core/heapDumpService.ts

@@ -134,7 +134,8 @@ export class HeapDumpService {
                const updatedVariable = this.createVariableFromReference(
                  symbolName,
                  refVar,
-                  new Map<string, null>()
+                  new Map<string, null>(),


The replaceFrameVariablesWithHeapDump is quite long.
It is worth considering breaking this up into smaller methods, which each method having a more descriptive name than having all the comments.

I don't know if you have time to do this now but, in general, this code type of long method code is not maintainable.

I can create a work item to do this but it's not happening right now. There are other bugs that need to get immediately looked at from the Blitz.

vazexqi

Please clean up all unnecessary comments.

The method itself is too long for me to actually make any meaningful comments since I can't follow the logic of all this.

vazexqi · 2018-10-04T20:47:34Z

...salesforcedx-apex-replay-debugger/test/unit/adapter/apexReplayDebugVariablesHandling.test.ts

+        // was set up correctly. The local variable cf1, of type CircularReference contains
+        // a field that's a List<CircularReference>. After creating cf1 we add cf1 to its own
+        // list.
+        const expectedVariableValue =


Remove this comment.

I'm not removing this comment, this is what the test case is setting up and verifying.

vazexqi · 2018-10-04T20:47:38Z

...salesforcedx-apex-replay-debugger/test/unit/adapter/apexReplayDebugVariablesHandling.test.ts

+
+        // Verify the value is set correctly and there's exactly 1 item
+        // in the list
+        expect(listChildVar.value).to.be.eq(expectedListVarValue);


Remove this comment.

I'll remove.

vazexqi · 2018-10-04T20:47:44Z

...salesforcedx-apex-replay-debugger/test/unit/adapter/apexReplayDebugVariablesHandling.test.ts

+        expect(updatedLocRefVariable.value).to.be.eq(expectedVariableValue);
+
+        // There should be 2 children, 1 list named cfList and 1 integer named someInt
+        expect(updatedLocRefVariable.variables.size).to.be.eq(2);


Remove this comment.

Sure, I'll remove these.

vazexqi · 2018-10-04T20:47:51Z

...salesforcedx-apex-replay-debugger/test/unit/adapter/apexReplayDebugVariablesHandling.test.ts

+
+        // There should be 1 child item on the list and it should have the same
+        // value as cf1, expectedVariableValue.
+        const listElementVar = listChildVar.variables.get(


Remove this comment.

vazexqi · 2018-10-04T20:47:56Z

...salesforcedx-apex-replay-debugger/test/unit/adapter/apexReplayDebugVariablesHandling.test.ts

+          '0'
+        ) as ApexVariableContainer;
+        expect(listElementVar.value).to.be.eq(expectedVariableValue);
+        // Lastly, verify that it's list has the same value as cfList, expectedListVarValue


Remove this comment.

I'll remove.

vazexqi · 2018-10-04T20:48:51Z

...salesforcedx-apex-replay-debugger/test/unit/adapter/apexReplayDebugVariablesHandling.test.ts

+        topFrame.id = id;
+        frameInfo.locals.set(localRefVariable.name, localRefVariable);
+        // Update it with the heap dump
+        heapDumpService.replaceVariablesWithHeapDump();


Remove this comment.

I'll remove.

vazexqi · 2018-10-04T20:48:54Z

...salesforcedx-apex-replay-debugger/test/unit/adapter/apexReplayDebugVariablesHandling.test.ts

+          .returns(heapdump);
+
+        // Create a ref variable to update
+        const localRefVariable = new ApexVariableContainer(


Remove this comment.

I'll remove.

vazexqi · 2018-10-04T20:48:58Z

...salesforcedx-apex-replay-debugger/test/unit/adapter/apexReplayDebugVariablesHandling.test.ts

+
+      it('Should correctly deal with circular references and variable values', () => {
+        // Create the heapdump
+        const heapdump = createHeapDumpWithCircularRefs();


Remove this comment.

vazexqi

We can agree to disagree about our standards of comments.

I would share https://blog.codinghorror.com/coding-without-comments/. The technique of making smaller methods with more descriptive names, often obviates the need for comments.

This is something that keeps coming up during the reviews so I thought I will drill into it more. It is also codified in our coding guidelines.

codecov · 2018-10-04T21:41:39Z

Codecov Report

Merging #659 into develop will increase coverage by 0.03%.
The diff coverage is 86.36%.

@@             Coverage Diff             @@
##           develop     #659      +/-   ##
===========================================
+ Coverage    74.83%   74.87%   +0.03%     
===========================================
  Files          158      158              
  Lines         6405     6431      +26     
  Branches      1001     1009       +8     
===========================================
+ Hits          4793     4815      +22     
- Misses        1357     1358       +1     
- Partials       255      258       +3

Impacted Files	Coverage Δ
...lay-debugger/test/unit/adapter/heapDumpTestUtil.ts	`100% <100%> (ø)`	⬆️
...x-apex-replay-debugger/src/core/heapDumpService.ts	`75.78% <85%> (+0.56%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 738161d...89c0699. Read the comment docs.

vazexqi · 2018-10-04T21:54:38Z

packages/salesforcedx-apex-replay-debugger/src/core/heapDumpService.ts

@@ -327,7 +337,8 @@ export class HeapDumpService {
  //   1: '2'
  //   2: '3'
  private createStringFromVarContainer(
-    varContainer: ApexVariableContainer
+    varContainer: ApexVariableContainer,
+    visitedMap: Map<string, null>


Is there a reason why we use it as a Map<string, null>? I think we don't ever use the key so whatever it is fine. But, in that case, why not use a Set since you are just checking for membership?

Habit and slight ignorance. Both Map and Set are close to O(1) in lookup times and I keep seeing Set and thinking Array

Cool, please change it then. Using a Setis much clearer rather than a Map<string, null> since there is nothing keyed off the value.

It's changed with the latest commit.

vazexqi · 2018-10-04T21:59:05Z

packages/salesforcedx-apex-replay-debugger/src/core/heapDumpService.ts

+    // If the varContainer is a ref and it's already been visited then return the string
+    if (varContainer.ref) {
+      if (visitedMap.has(varContainer.ref)) {
+        return 'already output';


I am guessing that this is the text matches the interactive debugger?

Yes. The pictures in the description illustrate live VS replay which is why I'd used "already output"

vazexqi

In general, the technique seems to be a "check-if-we-have-seen-this-before" and it we have put out "already output". We store what we have seen in a Map (possibly replaceable by a Set). This looks OK to me.

@DatGuyJonathan - Any other concerns here?

JimSuplizio · 2018-10-04T22:19:34Z

@vazexqi sure, I may tend to over comment and I admit to that. With simple code doing simple things, sure, it's 'self commenting' but I've had to deal with a lot of code that wasn't simple code doing simple things that had no comments. In the case of dealing heap dumps and extents I'd much rather over comment than under comment, especially when we're dealing with things like
heapdump.extents.extent.value.entry.value.value or heapdump.extents.extent.value.value.value. Also, as you're previously pointed out, there's the curse of knowledge, and while I may know the code but there should be enough comments for someone else to pick up and understand.

DatGuyJonathan · 2018-10-05T00:10:58Z

I'm trying to figure out where in this change it would've ran into the error RangeError: Maximum call stack size exceeded. Do you have your SFDX project attached to the bug? The bug's description says it's attached somewhere, but I can't find it.

JimSuplizio · 2018-10-05T14:50:22Z

@DatGuyJonathan the project wasn't added initially because I'm the only that ended up fixing the bug. It's been added now. The Max Stacksize exceeded was due to in an infinite loop in in the heap dump processing, not something coming from the server.

DatGuyJonathan · 2018-10-05T15:26:47Z

Okay, my question was about W-5491858 which got duped out to W-5491876. So, if W-5491858 is not fixed yet, you should probably re-open it.

JimSuplizio · 2018-10-05T15:31:40Z

Okay, my question was about W-5491858 which got duped out to W-5491876. So, if W-5491858 is not fixed yet, you should probably re-open it.

It was duped because the issues were related and I fixed both them with this PR.

salesforce-cla · 2020-07-28T19:25:17Z

Thanks for the contribution! Unfortunately we can't verify the commit author(s): James Suplizio <j***@s***.com>. One possible solution is to add that email to your GitHub account. Alternatively you can change your commits to another email and force push the change. After getting your commits associated with your GitHub account, refresh the status of this Pull Request.

salesforce-cla · 2021-01-08T17:17:16Z

Thanks for the contribution! Unfortunately we can't verify the commit author(s): James Suplizio <j***@s***.com>. One possible solution is to add that email to your GitHub account. Alternatively you can change your commits to another email and force push the change. After getting your commits associated with your GitHub account, refresh the status of this Pull Request.

Fix for circular references

21878fe

JimSuplizio commented Oct 4, 2018

View reviewed changes

vazexqi requested review from DatGuyJonathan and vazexqi October 4, 2018 20:38

vazexqi reviewed Oct 4, 2018

View reviewed changes

JimSuplizio commented Oct 4, 2018

View reviewed changes

JimSuplizio requested review from ntotten, DatGuyJonathan, AnanyaJha and allileong and removed request for DatGuyJonathan October 4, 2018 20:43

vazexqi reviewed Oct 4, 2018

View reviewed changes

Clean up comments in the test class

f193f57

Comment removal pt2

0f57355

vazexqi reviewed Oct 4, 2018

View reviewed changes

ntotten approved these changes Oct 5, 2018

View reviewed changes

change map to set

8b64090

DatGuyJonathan approved these changes Oct 5, 2018

View reviewed changes

Merge branch 'develop' into jsuplizio/CircularRefFix

89c0699

vazexqi approved these changes Oct 5, 2018

View reviewed changes

JimSuplizio merged commit f2c8bdf into develop Oct 5, 2018

vazexqi deleted the jsuplizio/CircularRefFix branch July 9, 2019 18:07

salesforce-cla bot added the cla:missing label Jul 28, 2020

snyk-bot mentioned this pull request Jan 22, 2022

[Snyk] Fix for 20 vulnerabilities Sarrac3873/salesforcedx-vscode#31

Open

Sarrac3873 mentioned this pull request Nov 28, 2023

[Snyk] Fix for 9 vulnerabilities Sarrac3873/salesforcedx-vscode#248

Open

Sarrac3873 mentioned this pull request May 22, 2024

[Snyk] Fix for 2 vulnerabilities Sarrac3873/salesforcedx-vscode#330

Open

Fix for circular references #659

Fix for circular references #659

Conversation

JimSuplizio commented Oct 4, 2018

What does this PR do?

What issues does this PR fix or reference?

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

vazexqi 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

vazexqi left a comment

Choose a reason for hiding this comment

codecov bot commented Oct 4, 2018 • edited Loading

Codecov Report

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

vazexqi left a comment

Choose a reason for hiding this comment

JimSuplizio commented Oct 4, 2018

DatGuyJonathan commented Oct 5, 2018

JimSuplizio commented Oct 5, 2018

DatGuyJonathan commented Oct 5, 2018

JimSuplizio commented Oct 5, 2018

salesforce-cla bot commented Jul 28, 2020

salesforce-cla bot commented Jan 8, 2021

codecov bot commented Oct 4, 2018 •

edited

Loading