Fix adding nodes in collapsed functions (#10009)

Insert new nodes before the block's terminal expression-statement, if present.

Fixes #9963.

# Important Notes
- Fix a bug that caused any empty lines at the beginning of a module not to be printed.
- Remove a redundant data-property from `GraphNode`.

app/gui2/shared/ast/parse.ts

@@ -507,11 +507,11 @@ export function printBlock(
 ): string {
   let blockIndent: string | undefined
   let code = ''
-  for (const line of block.fields.get('lines')) {
+  block.fields.get('lines').forEach((line, index) => {
     code += line.newline.whitespace ?? ''
     const newlineCode = block.module.getToken(line.newline.node).code()
     // Only print a newline if this isn't the first line in the output, or it's a comment.
-    if (offset || code || newlineCode.startsWith('#')) {
+    if (offset || index || newlineCode.startsWith('#')) {
       // If this isn't the first line in the output, but there is a concrete newline token:
       // if it's a zero-length newline, ignore it and print a normal newline.
       code += newlineCode || '\n'
@@ -533,7 +533,7 @@ export function printBlock(
       assertEqual(parentId(lineNode), block.id)
       code += lineNode.printSubtree(info, offset + code.length, blockIndent, verbatim)
     }
-  }
+  })
   const span = nodeKey(offset, code.length)
   map.setIfUndefined(info.nodes, span, (): Ast[] => []).unshift(block)
   return code

app/gui2/shared/ast/tree.ts

@@ -137,6 +137,15 @@ export abstract class Ast {
     return this.wrappingExpression()?.documentingAncestor()
   }
 
+  get isBindingStatement(): boolean {
+    const inner = this.wrappedExpression()
+    if (inner) {
+      return inner.isBindingStatement
+    } else {
+      return false
+    }
+  }
+
   code(): string {
     return print(this).code
   }
@@ -1904,6 +1913,10 @@ export class Function extends Ast {
     }
   }
 
+  get isBindingStatement(): boolean {
+    return true
+  }
+
   *concreteChildren(_verbatim?: boolean): IterableIterator<RawNodeChild> {
     const { name, argumentDefinitions, equals, body } = getAll(this.fields)
     yield name
@@ -1992,6 +2005,10 @@ export class Assignment extends Ast {
     return this.module.get(this.fields.get('expression').node)
   }
 
+  get isBindingStatement(): boolean {
+    return true
+  }
+
   *concreteChildren(verbatim?: boolean): IterableIterator<RawNodeChild> {
     const { pattern, equals, expression } = getAll(this.fields)
     yield ensureUnspaced(pattern, verbatim)

app/gui2/src/components/GraphEditor/GraphNodes.vue

@@ -48,7 +48,6 @@ const uploadingFiles = computed<[FileName, File][]>(() => {
     :node="node"
     :edited="id === graphStore.editedNodeInfo?.id"
     :graphNodeSelections="props.graphNodeSelections"
-    :data-node="id"
     @delete="graphStore.deleteNodes([id])"
     @dragging="nodeIsDragged(id, $event)"
     @draggingCommited="dragging.finishDrag()"

app/gui2/src/composables/__tests__/nodeCreation.test.ts

@@ -0,0 +1,43 @@
+import { insertNodeStatements } from '@/composables/nodeCreation'
+import { Ast } from '@/util/ast'
+import { identifier } from 'shared/ast'
+import { initializeFFI } from 'shared/ast/ffi'
+import { expect, test } from 'vitest'
+
+await initializeFFI()
+
+test.each([
+  ['node1 = 123', '*'],
+  ['node1 = 123', '*', 'node1'],
+  ['node1 = 123', '', '*', 'node1'],
+  ['*', 'node1'],
+  ['', '*', 'node1'],
+  ['*', '## Return value', 'node1'],
+  ['*', '## Return value', '', 'node1'],
+  ['*', '## Block ends in documentation?!'],
+])('New node location in block', (...linesWithInsertionPoint: string[]) => {
+  const inputLines = linesWithInsertionPoint.filter((line) => line !== '*')
+  const bodyBlock = Ast.parseBlock(inputLines.join('\n'))
+  insertNodeStatements(bodyBlock, [Ast.parse('newNodePositionMarker')])
+  const lines = bodyBlock
+    .code()
+    .split('\n')
+    .map((line) => (line === 'newNodePositionMarker' ? '*' : line))
+  expect(lines).toEqual(linesWithInsertionPoint)
+})
+
+// This is a special case because when a block is empty, adding a line requires adding *two* linebreaks.
+test('Adding node to empty block', () => {
+  const module = Ast.MutableModule.Transient()
+  const func = Ast.Function.fromStatements(module, identifier('f')!, [], [])
+  const rootBlock = Ast.BodyBlock.new([], module)
+  rootBlock.push(func)
+  expect(rootBlock.code().trimEnd()).toBe('f =')
+  insertNodeStatements(func.bodyAsBlock(), [Ast.parse('newNode')])
+  expect(
+    rootBlock
+      .code()
+      .split('\n')
+      .map((line) => line.trimEnd()),
+  ).toEqual(['f =', '    newNode'])
+})

app/gui2/src/composables/nodeCreation.ts

@@ -115,14 +115,15 @@ export function useNodeCreation(
     }
     const created = new Set<NodeId>()
     graphStore.edit((edit) => {
-      const bodyBlock = edit.getVersion(methodAst).bodyAsBlock()
+      const statements = new Array<Ast.Owned>()
       for (const options of placedNodes) {
         const { rootExpression, id } = newAssignmentNode(edit, options)
-        bodyBlock.push(rootExpression)
+        statements.push(rootExpression)
         created.add(id)
         assert(options.metadata?.position != null, 'Node should already be placed')
         graphStore.nodeRects.set(id, new Rect(Vec2.FromXY(options.metadata.position), Vec2.Zero))
       }
+      insertNodeStatements(edit.getVersion(methodAst).bodyAsBlock(), statements)
     })
     onCreated(created)
   }
@@ -186,3 +187,17 @@ function typeToPrefix(type: Typename): string {
     return type.toLowerCase()
   }
 }
+
+/** Insert the given statements into the given block, at a location appropriate for new nodes.
+ *
+ * The location will be after any statements in the block that bind identifiers; if the block ends in an expression
+ * statement, the location will be before it so that the value of the block will not be affected.
+ */
+export function insertNodeStatements(bodyBlock: Ast.MutableBodyBlock, statements: Ast.Owned[]) {
+  const lines = bodyBlock.lines
+  const index =
+    lines[lines.length - 1]?.expression?.node.isBindingStatement !== false ?
+      lines.length
+    : lines.length - 1
+  bodyBlock.insert(index, ...statements)
+}

app/gui2/src/composables/selection.ts

@@ -179,7 +179,7 @@ export function useGraphHover(isPortEnabled: (port: PortId) => boolean) {
   const hoveredNode = computed<NodeId | undefined>(() => {
     const element = hoveredElement.value?.closest('.GraphNode')
     if (!element) return undefined
-    return dataAttribute<NodeId>(element, 'node')
+    return dataAttribute<NodeId>(element, 'node-id')
   })
 
   return { hoveredNode, hoveredPort }

app/gui2/src/util/ast/__tests__/abstract.test.ts

@@ -369,6 +369,10 @@ const cases = [
   ['value = foo', '    bar'].join('\n'),
   ['value = foo', '    +x', '    bar'].join('\n'),
   ['###', ' x'].join('\n'),
+  '\n',
+  '\n\n',
+  '\na',
+  '\n\na',
 ]
 test.each(cases)('parse/print round trip: %s', (code) => {
   // Get an AST.