Repository navigation
Parentheses incorrectly added/omitted by getAdjusted #5326
Description
Describe the bug
Parentheses are sometimes unnecessarily added to (or not included when necessary) in JavaScript/PHP/Dart code expressions when calling getAdjusted because the inequality sign is reversed in the following places:
https://github.com/google/blockly/blob/master/generators/javascript.js#L314,
https://github.com/google/blockly/blob/master/generators/php.js#L299, and
https://github.com/google/blockly/blob/master/generators/dart.js#L290
Additionally, order overrides are not accounted for, and there is a lot of code duplication among these functions.
To Reproduce
Steps to reproduce the behavior:
- Go to the Blockly Playground
- Import the following XML:
<xml xmlns="https://developers.google.com/blockly/xml">
<variables>
<variable>foo</variable>
</variables>
<block type="lists_getIndex" x="37" y="37">
<mutation statement="false" at="true"></mutation>
<field name="MODE">GET</field>
<field name="WHERE">FROM_START</field>
<value name="VALUE">
<block type="variables_get">
<field name="VAR">foo</field>
</block>
</value>
<value name="AT">
<block type="math_arithmetic">
<field name="OP">ADD</field>
<value name="A">
<shadow type="math_number">
<field name="NUM">1</field>
</shadow>
</value>
<value name="B">
<shadow type="math_number">
<field name="NUM">1</field>
</shadow>
</value>
</block>
</value>
</block>
</xml>
- Click either "To JavaScript/PHP/Dart"
- Note the generated code has unnecessary outer parentheses:
foo[((1 + 1) - 1)]
Expected behavior
No outer parentheses.
Additional context
Can this issue be assigned to me so I can have my students work on it? Thanks!
Activity
BeksOmega commentedon Aug 9, 2021
@moniika just want to make sure you see this because you're definitely the best person to give tips here :D
BeksOmega commentedon Aug 10, 2021
Ok I've assigned this to you @jschanker :D I've looked through the code and your assessment of the issue sgtm but I'm not an expert in this area. If you run into any trouble definitely talk to @moniika
Also, just to keep you in the loop, there's a big file conversion going on this quarter (as described here) so depending on when your students branch/when they put up their PR there may be merge conflicts.
NeilFraser commentedon Aug 31, 2021
Just a note on code duplication. Under normal circumstances code duplication is Bad™. This case is a little bit unusual for a couple of reasons.
First, virtually every Blockly instance would only have one generator language. Thus we want generators to be completely self-contained. Duplicated code between different generators does not incur a download or memory penalty.
Second, generators are very much within the sphere of being edited by end-developers. Thus we want the code to be simple and easy to hack, rather than intricate and clever.
Different parts of the same project can have different code styles if there are significant differences in how that code is used. The core directory is as compact and clever as we can make it. The generators (and block definitions) are huge chunks of copy-paste.
jschanker commentedon Aug 31, 2021
@NeilFraser Thanks for providing the rationale here. It's always interesting for me to learn the 'why' behind Blockly's design decisions such as when you posted about the historical use of
==versus===here.I'm not sure how common this is, but what about the use case where you allow a user to export code to another language such as Python but also to quickly demo it in the browser by generating JavaScript code to run?
What are your thoughts on defining a Boolean Valued function in
generator.jscalled something likeparensNeeded(withouterOrderandinnerOrderparameter variables) that captures the logic from lines 263 to 288 that you could then also call from the specific language generators? My thinking is that this could provide the best of both worlds, decreasing code duplication while simplifying the logic a little for developers. In particular, it would allow for the order overrides to be accounted for without duplicating this language agnostic logic code.Thanks again!