Make sure state ref gets mutated in evalFunc

[tedbauer]

In the speculate case in evalFunc, the state name is reassigned to a new object, so the original state object doesn't get mutated as it should from that point on:

case 'speculate': {
  // Begin speculation.
  state = {
    ...state,
    env: new Map(state.env),  // Clone the environment.
    specparent: state,  // Save current state for aborts.
  };
  break;
}

For example, updates to the icount and env fields don't make it to the original object whose reference is passed as the actual for state. This PR stores the original object, and makes sure that it gets mutated properly.

[kq-li]

i noticed this too -- thought it might be a bit cleaner to change the speculate and abort logic to not create new objects, but just rewrite fields of the old object. here's how i did it:

@@ -728,11 +739,8 @@ function evalFunc(func: bril.Function, state: State): Value | null {
       }
       case 'speculate': {
         // Begin speculation.
-        state = {
-          ...state,
-          env: new Map(state.env),  // Clone the environment.
-          specparent: state,  // Save current state for aborts.
-        };
+        state.specparent = {...state}; // Save current state for aborts.
+        state.env = new Map(state.env); // Clone the environment.
         break;
       }
       case 'commit': {
@@ -748,9 +756,11 @@ function evalFunc(func: bril.Function, state: State): Value | null {
         if (!state.specparent) {
           throw error(`abort in non-speculative state`);
         }
-        let icount = state.icount;
-        state = state.specparent;
-        state.icount = icount;
+        state.env = state.specparent.env;
+        state.lastlabel = state.specparent.lastlabel;
+        state.curlabel = state.specparent.curlabel;
+        state.specparent = state.specparent.specparent;
         break;
       }

[tedbauer]

I changed to your approach! Mine doesn't even work, I think, because the 'end' case returns before I make the state mutations.

[sampsyo]

Thanks for looking into this!! I think I am being slow here, but I don't yet see why the new version behaves differently from the old version... maybe my first question here is whether the problem you're seeing is with env or with icount? (Or maybe I can get one of you to explain a little more during class today. 😃

[sampsyo]

I'm embarrassed to say I've been staring at this change for a while and I still don't get it. 😳 When you get a chance, could you explain a little more?

Here's what I thought happened:

• The program is executing along happily, doing its thing. It's updating env, icount, whatever.
• Now we speculate. We get a fresh state that's basically a copy of the old state, except that we save a pointer to the old snapshot of the state.
• Keep executing. Update icount and env and stuff. The thing we want to mutate is this new state, not the old one we saved away for safekeeping, which should remain unmodified.
• When it's time to abort, we restore the old specparent we staved when speculation started. So we reset env, icount, etc., etc., to the old saved state. Except for one thing!!! We want to preserve the modified icount to charge ourselves for the aborted speculation. So we need to copy that over.

As I'm typing this out, it's dawning on me that the problem you're seeing is probably when speculation commits, rather than when it aborts. That is, we might be using the wrong icount in that case. I'll look into that!

[sampsyo]

Arg, finally got it after playing with the code for a while. The problem is that the evalFunc call's "contract" is that it's supposed to mutate the state on behalf of its caller, and we're not doing that, of course. We could instead go functional and return an updated state but that would be annoying.

[sampsyo]

Updated slightly and merged! Thanks, @tedbauer and @kq-li!!!