fix: JSON deserialization fails (bug #6091) (collapsed procedure call…

[tweini]

… blocks)

The basics

• I branched from develop
• My pull request is against develop
• My code follows the style guide
• I ran npm run format and npm run lint
• BONUS: I ran npm run test

The details

Resolves

#6091

Proposed Changes

Flipping the call order in core/serialization/blocks.js of .loadAttributes() and .loadExtraState() does the trick.

Behavior Before Change

During deserialization of the call block, a very nasty bug begins here:

blockly/blocks/procedures.js

Line 739 in d2329f8

this.setCollapsed(false);

Because the collapsed attribute was set to true beforehand, this becomes a crime scene:

blockly/core/block_svg.js

Line 642 in d2329f8

if (this.collapsed_ === collapsed) {

this.collapsed_ is true, so the if-cond isn't met and super.setCollapsed(collapsed) is getting executed. This sets collapsed to false, so the condition if (!collapsed) is met and this.updateCollapsed_() gets executed.

updateCollapse_() maintains the logic of setting a dummy with _TEMP_COLLAPSED_INPUT (hence the error "block.js:2022 Uncaught Error: Input not found: _TEMP_COLLAPSED_INPUT")

Behavior After Change

For whom is interested in, the "right" call to updateCollapse_() will happen here:

blockly/core/block_svg.js

Line 1741 in d2329f8

this.updateCollapsed_();

As the block must get rendered collapsed.

Reason for Changes

bug 🐛

Test Coverage

Existing tests passed. Manually tested both types "procedures_callnoreturn" and procedures_callreturn" in collapsed state, and observed changed behaviour in debugger.

Potential Failures:
As changing the order does change the deserialization of other "attributes":

blockly/core/serialization/blocks.js

Line 426 in d2329f8

const loadAttributes = function(block, state) {

I spotted the data-attribute as a potential "let's introduce a regression bug", as it shows up very early in xml deserialization:

blockly/core/xml.js

Line 882 in d4196c1

Blockly.Xml.applyDataTagNodes_(xmlChildNameMap.data, block);

So it may be better to split up the .loadAttributes() method?

Documentation

Maybe I should have left a comment in the code 😳

Additional Information

Yes, I will add a comment in the code. I will do this, after the code is reviewed.

[BeksOmega]

So sadly we can't change the deserialization order :/ because that would be a breaking change.

Currently attributes are deserialized before extraState, which means that in a loadExtraState function, an external developer could be expecting collapsed, disabled, x, y, etc to be properly deserialized into the block. If we change the load order, we break that promise, and possibly break people's code. See also the developer docs.

I hate adding if-statements to fix bugs, but do you think that adding an if (this.rendered) call around the setCollapsed in setProcedureParameters_ would work? This way setting collapsed to false would be skipped while deserializing, so that this bug doesn't get triggered.

Also, would you be able to add a new test case for this? You would go to the playground, set up the correct scenario, export it to XML, and then add the stringified XML to that linked file. Unit tests wouldn't have worked for your previous PR because it required dragging to reproduce the bug, but here we can totally add a unit test =)

Thank you again for digging into this!

[tweini]

Because I think this is a tip of an iceberg, this bug leads us to, I want to add my point of view:

If the developer relies on the 'collapsed" attributes, he would rely on the mutation is present in the block:

The root cause of this bug is, that the 'collapsed' attribute isn't a mere presentation attribute, it is literally a mutator, which mutates the block. This mutation creates the additional input "_TEMP_COLLAPSED_INPUT".

So, the flip side of this order could also be a developer, which not only relies on the correct "collapse" attribute present, but also on the entire mutated state of that block. (Fun fact: Blockly could be seen as a developer who was trapped, as it fails to deserialize correctly.)

So we could interpret that the breaking change was altering the order, as it was deserialzed in the very end in the xml serializer component.

I will review your proposal of fixing the bug. And I will do further investigation, as I hate adding if-statements to fix bugs as much as you do.

Testing is on my agenda. Thanks for nudging me.

[BeksOmega]

The root cause of this bug is, that the 'collapsed' attribute isn't a mere presentation attribute, it is literally a mutator, which mutates the block. This mutation creates the additional input "_TEMP_COLLAPSED_INPUT".

Yeah I've had the same frustration. It is unfortunate that collapsing was implemented by changing the block's inputs, rather than modifying the renderer to have a separate "collapsed" mode/state.

So we could interpret that the breaking change was altering the order, as it was deserialzed in the very end in the xml serializer component.

I see what you're saying here too, but I don't think it was actually a breaking change. With the XML serializer we didn't actually make any promises about deserialization order. In the past XML was just deserialized in the order that nodes existed in, so developers couldn't rely on things being deserialized before other things. But this required duplicating a lot of extra state if you wanted to be safe, which bloated save file sizes. So we decided to make it explicit with the JSON serializer.

Plus the JSON serilializer was a new feature, so nothing about it could silently break developers' code. If we change the JSON deserialization order now that it's been released, it could.

---

Side note: I would say separate out the logic in setProcedureParameters_ into multiple functions so that the setCollapsed call doesn't get hit during deserialization. This would avoid if-statements. But I might actually be tackling #3725 and other related procedure refactors soon, and I don't want you to have to do duplicate work.

Really appreciate hearing your thoughts. I'll be thinking about this more too.

[tweini]

Appreciate your positive feedback. I am absolutely with you to decouple things, because side effects are causing all this sort of bugs.

Dealing with legacy code means thinking like a detective. And this led me to the question:

Why the setCollapsed(false) (L739) call in the code?

Removing this call is safe as long as there is no input connected to a variable, while being removed in collapsed state. An open mutator seems to be the precondition, where L739 makes sense, so we could append an else on the existing if at L721?

I tried it and ran the tests ✅ and the bug is gone. So, my assumption is, that L739 is an ancient bug fix, which has been placed too prominently. (Changing the role from detective to historian 😄 )

Out of curiousity: How is L727 reached. The precondition is if (!paramIds) and the comment says "Reset the quarks (a mutator is about to open)."?

Why are these parameters named "quarks"?

[BeksOmega]

Why the setCollapsed(false) (L739) call in the code?

Removing this call is safe as long as there is no input connected to a variable, while being removed in collapsed state. An open mutator seems to be the precondition, where L739 makes sense, so we could append an else on the existing if at L721?

I tried it and ran the tests ✅ and the bug is gone. So, my assumption is, that L739 is an ancient bug fix, which has been placed too prominently. (Changing the role from detective to historian 😄 )

I think that could definitely work! And from checking out the git blame, it does look like an ancient bug fix. Sadly it doesn't have any details (eg reproduction steps) attached. I imagine the bug was something like:

1. Create a procedure definition and caller.
2. Collapse the caller block.
3. Add an argument to the procedure definition.
4. Observe how the collapsed caller block is rendered incorrectly somehow?

So if I'd like to check that case before we merge anything.

Given the historical context: Do you think it would work to just remove the setCollapsed call entirely?

Out of curiousity: How is L727 reached. The precondition is if (!paramIds) and the comment says "Reset the quarks (a mutator is about to open)."?

Hmmmm. Grrrrr. It looks like it actually isn't being hit anymore. Seems to be another ancient bug fix with no docs.

(imho the procedure blocks really need to be rewritten from scratch, but its hard with such a small team)

Why are these parameters named "quarks"?

Hahaha, good question! I think the idea was that the block is an "atom" that you are breaking down into its component pieces - ie quarks! It is mixing metaphors a bit when changing the block is already called a "mutation" though haha.

[BeksOmega]

You know what, I bet the setCollapsed call was originally solving for: #3784 which has since been fixed.

So I'm leaning more and more toward just removing it.

[tweini]

(imho the procedure blocks really need to be rewritten from scratch, but its hard with such a small team)

Yep. As a tech guy I feel you. But managing technical debt is too complex. Also bugfixing is an investment to reduce technical debt, as the knowledge of system grows and the requirements for a rewrite become pretty clear. As long as the bugfix does real research and investigates root causes. (We definitely do.)

Removing this call is safe as long as there is no input connected to a variable, while being removed in collapsed state. An open mutator seems to be the precondition, where L739 makes sense, so we could append an else on the existing if at L721?

As I started to name it "removing" let's state for the readers we mean "relocate" the setCollapsed() as mentioned above.

Out of curiousity: How is L727 reached. The precondition is if (!paramIds) and the comment says "Reset the quarks (a mutator is about to open)."?

Hmmmm. Grrrrr. It looks like it actually isn't being hit anymore. Seems to be another ancient bug fix with no docs.

It really can be safely removed, really deleted, as the only caller of setProcedureParameters_ makes it impossible to reach the code:

blockly/blocks/procedures.js

Line 865 in 2bf842a

const paramIds = [];

no side effects between L865 and L872

blockly/blocks/procedures.js

Line 872 in 2bf842a

this.setProcedureParameters_(args, paramIds);

So, we don't need to hesitate, but two pairs of eyes (eagle eyes, owl eyes, choose your weapon) are a good base for making decisions.

My next steps would be:

• Change the code with mentioned relocation
• Add a test case
• remove L725 - L728

Maybe it will take some days, maybe not. Stay tuned.

A requirement for the future rewrite we could be: "The Mutator should be able to mutate the call block without expanding it." But here we will definitely run in UX issues 😏 'cause the changes are "invisible", not revertible (as imho the undo isn't active while mutating.)

From a pure technical view it should definitely be possible, as it assures clean code as the rendered view and the data structure behind the block should be clearly separated.

Could be a cool exception from a YAGNI feature 😃

[BeksOmega]

LGTM! Thank you for the fix! And thank you for adding those unit tests, they look great =)