Merge pull request #3551 from scratchfoundation/fix/tolerate-malformed-argument-arrays

fix: tolerate real-world mutation attribute values on procedure blocks

Author: cwillisf

src/blocks/procedures.ts

@@ -37,6 +37,8 @@ type ConnectionMap = Record<
 /**
  * Possible types for procedure arguments.
  */
+type ArgumentDefault = string | number | boolean | null
+
 enum ArgumentType {
   STRING = 's',
   NUMBER = 'n',
@@ -75,39 +77,17 @@ function getRequiredMutationAttribute(xmlElement: Element, name: string): string
   return value
 }
 
-/**
- * Parse a required mutation attribute as JSON, then validate its type.
- * @param xmlElement The mutation element that should contain required attributes.
- * @param name The specific mutation attribute to retrieve and parse.
- * @param parse Validates and narrows the parsed JSON value.
- * @returns Parsed and validated mutation attribute value.
- */
-function parseRequiredMutationJson<T>(
-  xmlElement: Element,
-  name: string,
-  parse: (value: unknown, name: string) => T,
-): T {
-  const rawValue = getRequiredMutationAttribute(xmlElement, name)
-  let parsedValue: unknown
-  try {
-    parsedValue = JSON.parse(rawValue)
-  } catch {
-    throw new Error(`Invalid JSON in mutation attribute: ${name}`)
-  }
-  return parse(parsedValue, name)
-}
-
 /**
  * Parse an optional mutation attribute as JSON, returning a fallback when the
- * attribute is absent. Use this only for attributes that can be safely
- * defaulted in isolation, without invalidating structural invariants that
- * relate them to other attributes on the same mutation. A present-but-malformed
- * attribute still throws, since that indicates corruption rather than an older
- * schema.
+ * attribute is absent or malformed. Older saved projects may omit attributes
+ * that current serialization always writes, and some projects in the wild
+ * contain attributes whose values are not valid for the expected type.
+ * Returning the fallback in both cases lets the project load rather than
+ * aborting the entire workspace parse.
  * @param xmlElement The mutation element that may contain the attribute.
  * @param name The specific mutation attribute to retrieve and parse.
  * @param parse Validates and narrows the parsed JSON value.
- * @param fallback Value to return when the attribute is absent.
+ * @param fallback Value to return when the attribute is absent or malformed.
  * @returns Parsed and validated mutation attribute value, or `fallback`.
  */
 function parseOptionalMutationJson<T>(
@@ -120,13 +100,12 @@ function parseOptionalMutationJson<T>(
   if (rawValue === null) {
     return fallback
   }
-  let parsedValue: unknown
   try {
-    parsedValue = JSON.parse(rawValue)
+    return parse(JSON.parse(rawValue), name)
   } catch {
-    throw new Error(`Invalid JSON in mutation attribute: ${name}`)
+    console.warn(`Invalid or unexpected mutation attribute "${name}", using default. Raw value: ${rawValue}`)
+    return fallback
   }
-  return parse(parsedValue, name)
 }
 
 /**
@@ -155,6 +134,26 @@ function parseStringArrayMutationValue(value: unknown, name: string): string[] {
   return value
 }
 
+/**
+ * Validate a parsed mutation value as an array of primitives. Argument
+ * defaults from older projects (especially Scratch 2.0 conversions) contain
+ * mixed types — strings for text arguments, numbers for numeric arguments,
+ * and booleans for boolean arguments (e.g. `["",1,1,1,1]`). The old fork
+ * stored these via untyped `JSON.parse` with no validation, and the VM
+ * passes them through as-is.
+ * @param value Parsed mutation value.
+ * @param name Attribute name used in error messages.
+ * @returns Validated array value.
+ */
+function parsePrimitiveArrayMutationValue(value: unknown, name: string): ArgumentDefault[] {
+  const isArgumentDefault = (entry: unknown): entry is ArgumentDefault =>
+    entry === null || typeof entry === 'string' || typeof entry === 'number' || typeof entry === 'boolean'
+  if (!Array.isArray(value) || !value.every(isArgumentDefault)) {
+    throw new Error(`Expected ArgumentDefault[] JSON value in mutation attribute: ${name}`)
+  }
+  return value
+}
+
 /**
  * A drag strategy for the procedures_prototype block that delegates all drag
  * operations to its parent (the procedures_definition block). This lets the
@@ -308,7 +307,7 @@ function callerDomToMutation(this: ProcedureCallBlock, xmlElement: Element) {
   this.procCode_ = getRequiredMutationAttribute(xmlElement, 'proccode')
   const generateshadows = xmlElement.getAttribute('generateshadows')
   this.generateShadows_ = generateshadows !== null ? JSON.parse(generateshadows) === true : false
-  this.argumentIds_ = parseRequiredMutationJson(xmlElement, 'argumentids', parseStringArrayMutationValue)
+  this.argumentIds_ = parseOptionalMutationJson(xmlElement, 'argumentids', parseStringArrayMutationValue, [])
   this.warp_ = parseOptionalMutationJson(xmlElement, 'warp', parseBooleanMutationValue, false)
   this.updateDisplay_()
 }
@@ -349,9 +348,14 @@ function definitionDomToMutation(this: ProcedurePrototypeBlock | ProcedureDeclar
   const prevArgIds = this.argumentIds_
   const prevDisplayNames = this.displayNames_
 
-  this.argumentIds_ = parseRequiredMutationJson(xmlElement, 'argumentids', parseStringArrayMutationValue)
-  this.displayNames_ = parseRequiredMutationJson(xmlElement, 'argumentnames', parseStringArrayMutationValue)
-  this.argumentDefaults_ = parseRequiredMutationJson(xmlElement, 'argumentdefaults', parseStringArrayMutationValue)
+  this.argumentIds_ = parseOptionalMutationJson(xmlElement, 'argumentids', parseStringArrayMutationValue, [])
+  this.displayNames_ = parseOptionalMutationJson(xmlElement, 'argumentnames', parseStringArrayMutationValue, [])
+  this.argumentDefaults_ = parseOptionalMutationJson(
+    xmlElement,
+    'argumentdefaults',
+    parsePrimitiveArrayMutationValue,
+    [],
+  )
 
   // During full XML deserialization (Blockly.Xml.domToWorkspace), the mutation element
   // is part of the parsed XML tree and its parent element also contains <value> children
@@ -1341,7 +1345,7 @@ interface ProcedureBlock extends Blockly.BlockSvg {
 
 export interface ProcedureDeclarationBlock extends ProcedureBlock {
   displayNames_: string[]
-  argumentDefaults_: string[]
+  argumentDefaults_: ArgumentDefault[]
   removeFieldCallback: (field: Blockly.Field) => void
   createArgumentEditor_: (argumentType: ArgumentType, displayName: string) => Blockly.BlockSvg
   focusLastEditor_: () => void
@@ -1361,7 +1365,7 @@ interface ProcedureCallBlock extends ProcedureBlock {
 
 interface ProcedurePrototypeBlock extends ProcedureBlock {
   displayNames_: string[]
-  argumentDefaults_: string[]
+  argumentDefaults_: ArgumentDefault[]
   skipArgumentReporters_: boolean
   createArgumentReporter_: (argumentType: ArgumentType, displayName: string) => Blockly.BlockSvg
   updateArgumentReporterNames_: (prevArgIds: string[], prevDisplayNames: string[]) => void

tests/browser/procedures.test.ts

@@ -272,68 +272,65 @@ describe('legacy mutation tolerance', () => {
     ).toThrow(/proccode/)
   })
 
-  // argumentids/argumentnames/argumentdefaults have a structural invariant:
-  // their lengths must equal the %s/%n/%b token count in proccode. Silently
-  // defaulting any of them to [] would turn an N-arg procedure into a 0-arg
-  // block, so keep them required and surface the problem loudly.
-  it('procedures_prototype mutation without argumentids still throws', () => {
+  // argumentdefaults in older projects can contain numbers alongside strings
+  // (e.g. [1] or ["",1,1,1,1]). This regression case verifies those
+  // mixed primitive values are preserved when loading legacy mutations.
+  it('procedures_prototype mutation with numeric argumentdefaults preserves types', () => {
     expect(() =>
       loadXml(`
         <xml>
           <block type="procedures_definition">
             <statement name="custom_block">
               <block type="procedures_prototype">
-                <mutation proccode="test %s" argumentnames='["x"]' argumentdefaults='[""]' warp="false"></mutation>
+                <mutation proccode="test %n" argumentids='["arg1"]' argumentnames='["x"]' argumentdefaults='[1]' warp="false"></mutation>
               </block>
             </statement>
           </block>
         </xml>
       `),
-    ).toThrow(/argumentids/)
-  })
+    ).not.toThrow()
 
-  it('procedures_prototype mutation without argumentnames still throws', () => {
-    expect(() =>
-      loadXml(`
-        <xml>
-          <block type="procedures_definition">
-            <statement name="custom_block">
-              <block type="procedures_prototype">
-                <mutation proccode="test %s" argumentids='["arg1"]' argumentdefaults='[""]' warp="false"></mutation>
-              </block>
-            </statement>
-          </block>
-        </xml>
-      `),
-    ).toThrow(/argumentnames/)
+    const proto = workspace.getAllBlocks(false).find((b) => b.type === 'procedures_prototype')
+    assert(proto, 'Expected procedures_prototype block')
+    expect((proto as any).argumentDefaults_).toEqual([1])
   })
 
-  it('procedures_prototype mutation without argumentdefaults still throws', () => {
+  it('procedures_prototype mutation with mixed string/number argumentdefaults preserves types', () => {
     expect(() =>
       loadXml(`
         <xml>
           <block type="procedures_definition">
             <statement name="custom_block">
               <block type="procedures_prototype">
-                <mutation proccode="test %s" argumentids='["arg1"]' argumentnames='["x"]' warp="false"></mutation>
+                <mutation proccode="test %s %n %n %n %n" argumentids='["a","b","c","d","e"]' argumentnames='["typ","x","y","sx","sy"]' argumentdefaults='["",1,1,1,1]' warp="true"></mutation>
               </block>
             </statement>
           </block>
         </xml>
       `),
-    ).toThrow(/argumentdefaults/)
+    ).not.toThrow()
+
+    const proto = workspace.getAllBlocks(false).find((b) => b.type === 'procedures_prototype')
+    assert(proto, 'Expected procedures_prototype block')
+    expect((proto as any).argumentDefaults_).toEqual(['', 1, 1, 1, 1])
   })
 
-  it('procedures_call mutation without argumentids still throws', () => {
+  it('procedures_call mutation with warp="null" loads as false', () => {
+    // Project 10118230 has warp='null' on some call blocks.
+    // JSON.parse("null") → null, which is not boolean. Should default to false.
     expect(() =>
       loadXml(`
         <xml>
           <block type="procedures_call">
-            <mutation proccode="test %s" warp="false"></mutation>
+            <mutation proccode="test %s" argumentids='["arg1"]' warp='null'></mutation>
           </block>
         </xml>
       `),
-    ).toThrow(/argumentids/)
+    ).not.toThrow()
+
+    const callBlock = workspace.getAllBlocks(false).find((b) => b.type === 'procedures_call')
+    assert(callBlock, 'Expected procedures_call block')
+    expect((callBlock as any).warp_).toBe(false)
   })
 })