Bug fix: Prevent intermediate state from appearing in final txlog #5048
Conversation
Wow
Nice solution! |
| if (!modifiedNode.arguments) { | ||
| modifiedNode.arguments = variables; | ||
| } | ||
| if ( |
There was a problem hiding this comment.
Minor nit. I was confused if the comments on 39-40 applied to this block of code. It seems it doesn't. A blank line, or a new comment, could help the reader.
There was a problem hiding this comment.
I think it does apply, actually?
There was a problem hiding this comment.
Oops. I highlighted the wrong block. It was this snippet of code that didn't reference a null or undefined that didn't relate to the comment.
+ if (
+ modifiedNode.type === "callexternal" &&
+ modifiedNode.kind === "library"
+ ) {
+ modifiedNode.kind = "function";
+ delete modifiedNode.data;
+ }
There was a problem hiding this comment.
Oh, I see. Yeah, I can clarify that.
Yeah I'm pretty confused about this TBH. It seems like sometimes it works fine and sometimes it doesn't? This is hardly the first time I've run into this problem with multiple actions... and yet there to be other cases where it's fine? I have to admit I don't really understand this. |
This reverts commit fbda90a.
Well, uh, I ended up removing this as unnecessary... see above. |
Addresses #3884.
This PR fixes a problem where intermediate state -- specifically, the
waitingForFunctionDefinitionfield -- was appearing in final txlogs. Why was this problem occurring? Basically, we had a case where the conditions for a jump out and for an identify were triggered on the same step. So an action was triggered for each, and, uh, I guess these things don't always work synchronously? IDK, I'm a little unclear on that point. Regardless, the jump out action deleted thewaitingForFunctionDefinitionintermediate state... and then the identify added it back in (setting it tofalse). Oops.So, in order to prevent this from happening, I had to make the identify case exclusive of the other cases. Why wasn't it exclusive before? Well, what if we have a simultaneous jump-in and identify? When I wrote txlog, actually, I incorrectly thought that was the usual case. It isn't, and, well, how would that ever happen? You jump in, then you're on the function... simultaneous doesn't make sense.
In order to make sure we can handle that case, I also created combined jump-in/identify actions. This meant factoring the identification code a bit. Anyway so now jump-in w/o identify, identified-jump in, identify w/o jump-in are three exclusive cases using three different actions. (Five, actually, since jump-ins can be absorbed or not absorbed.) Rather than relying on two actions for the jump-in and identify case. (Not that I've seen any cases of this, but I want to be able to handle it.)In order to make sure this problem doesn't crop up again, this PR also adds tests. Or rather, it adds to the existing tests, since the situation that was leading to the problem was among our existing tests already, so I didn't create any new tests. Rather, I just added to all of the existing tests a check that there's no intermediate state left at the end. And that's it!
This'll cause merge conflicts with #4882, I'm sure. :-/ Also:
prettiermade a bunch of changes on this PR. Sorry.