CodeGenerator.prototype.statementToCode and valueToCode should throw when given an invalid input name

[cpcallen]

Check for duplicates

• I have searched for similar issues before opening a new one.

Problem

It is a common pattern for the code generators for blocks with statement inputs to look like:

export function blockWithStatementInput(block, generator) {
  const code = generator.statementToCode(block, 'STATEMENT_INPUT');
  return `/* prefix */ {\n${code}}\n /* postfix */`;
}

but if the given input name (in this case 'STATEMENT_INPUT' does not correspond to one of the inputs on the block this fails silently, with code being set to ''. This is probably undesirable.

Request

CodeGenerator.prototype.statementToCode(block, name) should throw an error if block does not have an input named name.

Alternatives considered

As far as alternatives to this proposal: the obvious alternative is the status quo, which can leave developers confused about why their code is not working (see a recent example in the forum).

There are a few alternatives about how this might be achieved. In particular, any of the following functions might be modified to detect this error and throw:

• Block.prototype.getInput(name) could throw if no input named name is found (currently returns null and is so documented).
• Block.prototype.getInputTargetBlock(name) could throw if no input named name is found (currently returns null and is so documented).
• CodeGeneartor.prototype.blockToCode(block) could throw if block is null (currently returns '' and is so documented).
• CodeGenerator.prototype.statementToCode(block, name) could could throw if block.getTargetBlock(name) returns null (currently it carries on and calls this.blockToCode(null), but no specific behaviour is documented).

I (@cpcallen) discussed this with @NeilFraser, who observed:

Hmm, a getter returning null seems fine. Whereas a 'toCode' function should throw in my opinion. Getters are different from 'doers'. I can't think of any case where we call statementToCode speculatively and expect '' to come back out.

My view is that blockToCode should throw when passed null (or any other non-Block value), but evidently the current behaviour is documented and relied upon: a trial implementation breaks 35 of the mocha tests currently. Having only statementToCode throw is less helpful at catching developer bugs and would still be a breaking change, but it my be preferable as it is less disruptive, does not change documented behaviour, and only causes seven mocha test failure.

Additional context

No response

[maribethb]

Rachel & I agree with Neil that the getters can return null, but the toCode functions can throw, unless we get more information. I think the fact that some mocha tests fail might just because we have particularly-written mocha tests and not that we or anyone actually relies on this "feature."

Let's try to do this in v11. #7446

[BeksOmega]

To Fix

1. Modify statementToCode and valueToCode to throw an error (No input with name '${name}') if there is no input with the given name.
2. Modify any mocha tests relying on the old behavior to not do that. Feel free to ask for help with this step!

[BeksOmega]

We're actually relying on valueToCode returning '' when there's no input in the procedure code generators.

In the procedure def return generator, we generate code for the RETURN input. Which is then reused by the procedure def no return generator, which doesn't have that input.

[maribethb]

I think we can just fix ours at the same time, and still include this as a breaking change. Does that sound reasonable?

[BeksOmega]

Yep SGTM! Just wanted to check since sometimes we don't like to change things if they're in block definitions, and I didn't know if the same thing applied to block generators (sorry should have clarified why I was retriaging in the original post).