[Bug]: Linking Overshadowed Built-Ins in Loops (#957)

src/dataflow/environments/built-in.ts

@@ -174,24 +174,24 @@ registerSimpleFunctions(
 	'do.call', 'rbind', 'nrow', 'ncol', 'tryCatch', 'expression', 'factor',
 	'missing', 'as.data.frame', 'data.frame', 'na.omit', 'rownames', 'names', 'order', 'length', 'any', 'dim', 'matrix', 'cbind', 'nchar', 't'
 )
-registerBuiltInFunctions(true,  ['lapply', 'sapply', 'vapply', 'mapply'],          processApply,              { indexOfFunction: 1, nameOfFunctionArgument: 'FUN' }                        )
+registerBuiltInFunctions(false, ['lapply', 'sapply', 'vapply', 'mapply'],          processApply,              { indexOfFunction: 1, nameOfFunctionArgument: 'FUN' }                        )
 /* functool wrappers */
-registerBuiltInFunctions(true,  ['Lapply', 'Sapply', 'Vapply', 'Mapply'],          processApply,              { indexOfFunction: 1, nameOfFunctionArgument: 'FUN' }                        )
-registerBuiltInFunctions(true,  ['apply', 'tapply', 'Tapply'],                     processApply,              { indexOfFunction: 2, nameOfFunctionArgument: 'FUN' }                        )
-registerBuiltInFunctions(true,  ['print'],                                         defaultBuiltInProcessor,   { returnsNthArgument: 0, forceArgs: 'all' as const }                         )
+registerBuiltInFunctions(false, ['Lapply', 'Sapply', 'Vapply', 'Mapply'],          processApply,              { indexOfFunction: 1, nameOfFunctionArgument: 'FUN' }                        )
+registerBuiltInFunctions(false, ['apply', 'tapply', 'Tapply'],                     processApply,              { indexOfFunction: 2, nameOfFunctionArgument: 'FUN' }                        )
+registerBuiltInFunctions(false, ['print'],                                         defaultBuiltInProcessor,   { returnsNthArgument: 0, forceArgs: 'all' as const }                         )
 registerBuiltInFunctions(true,  ['('],                                             defaultBuiltInProcessor,   { returnsNthArgument: 0 }                                                    )
-registerBuiltInFunctions(true,  ['load', 'load_all', 'setwd', 'set.seed'],         defaultBuiltInProcessor,   { hasUnknownSideEffects: true, forceArgs: [true] }                           )
+registerBuiltInFunctions(false, ['load', 'load_all', 'setwd', 'set.seed'],         defaultBuiltInProcessor,   { hasUnknownSideEffects: true, forceArgs: [true] }                           )
 registerBuiltInFunctions(false, ['cat'],                                           defaultBuiltInProcessor,   { forceArgs: 'all' as const }                                                ) /* returns null */
 registerBuiltInFunctions(false, ['switch'],                                        defaultBuiltInProcessor,   {}                                                                           ) /* returns null */
 registerBuiltInFunctions(true,  ['return'],                                        defaultBuiltInProcessor,   { returnsNthArgument: 0, cfg: ExitPointType.Return }                         )
 registerBuiltInFunctions(true,  ['break'],                                         defaultBuiltInProcessor,   { cfg: ExitPointType.Break }                                                 )
 registerBuiltInFunctions(true,  ['next'],                                          defaultBuiltInProcessor,   { cfg: ExitPointType.Next }                                                  )
 registerBuiltInFunctions(true,  ['{'],                                             processExpressionList,     {}                                                                           )
-registerBuiltInFunctions(true,  ['source'],                                        processSourceCall,         { includeFunctionCall: true, forceFollow: false }                            )
+registerBuiltInFunctions(false, ['source'],                                        processSourceCall,         { includeFunctionCall: true, forceFollow: false }                            )
 registerBuiltInFunctions(true,  ['[', '[['],                                       processAccess,             { treatIndicesAsString: false }                                              )
 registerBuiltInFunctions(true,  ['$', '@'],                                        processAccess,             { treatIndicesAsString: true }                                               )
 registerBuiltInFunctions(true,  ['if', 'ifelse'],                                  processIfThenElse,         {}                                                                           )
-registerBuiltInFunctions(true,  ['get'],                                           processGet,                {}                                                                           )
+registerBuiltInFunctions(false, ['get'],                                           processGet,                {}                                                                           )
 registerBuiltInFunctions(false, ['library', 'require'],                            processLibrary,            {}                                                                           )
 registerBuiltInFunctions(true,  ['<-', '='],                                       processAssignment,         { canBeReplacement: true }                                                   )
 registerBuiltInFunctions(true,  [':=', 'assign'],                                  processAssignment,         {}                                                                           )
@@ -207,14 +207,14 @@ registerBuiltInFunctions(true,  ['quote', 'substitute', 'bquote'],
 registerBuiltInFunctions(true,  ['for'],                                           processForLoop,            {}                                                                           )
 registerBuiltInFunctions(true,  ['repeat'],                                        processRepeatLoop,         {}                                                                           )
 registerBuiltInFunctions(true,  ['while'],                                         processWhileLoop,          {}                                                                           )
-registerBuiltInFunctions(true,  ['options'],                                       defaultBuiltInProcessor,   { hasUnknownSideEffects: true, forceArgs: 'all' as const }                   )
-registerBuiltInFunctions(true,  ['on.exit', 'sys.on.exit'],                        defaultBuiltInProcessor,   { hasUnknownSideEffects: true }                                              )
+registerBuiltInFunctions(false, ['options'],                                       defaultBuiltInProcessor,   { hasUnknownSideEffects: true, forceArgs: 'all' as const }                   )
+registerBuiltInFunctions(false, ['on.exit', 'sys.on.exit'],                        defaultBuiltInProcessor,   { hasUnknownSideEffects: true }                                              )
 /* library and require is handled above */
-registerBuiltInFunctions(true,  ['requireNamespace', 'loadNamespace', 'attachNamespace', 'asNamespace'], defaultBuiltInProcessor, { hasUnknownSideEffects: true }                          )
+registerBuiltInFunctions(false, ['requireNamespace', 'loadNamespace', 'attachNamespace', 'asNamespace'], defaultBuiltInProcessor, { hasUnknownSideEffects: true }                          )
 /* downloader and installer functions (R, devtools, BiocManager) */
-registerBuiltInFunctions(true,  ['library.dynam', 'install.packages','install', 'install_github', 'install_gitlab', 'install_bitbucket', 'install_url', 'install_git', 'install_svn', 'install_local', 'install_version', 'update_packages'], defaultBuiltInProcessor, { hasUnknownSideEffects: true }                               )
+registerBuiltInFunctions(false, ['library.dynam', 'install.packages','install', 'install_github', 'install_gitlab', 'install_bitbucket', 'install_url', 'install_git', 'install_svn', 'install_local', 'install_version', 'update_packages'], defaultBuiltInProcessor, { hasUnknownSideEffects: true }                               )
 /* weird env attachments */
-registerBuiltInFunctions(true,  ['attach'],                                        defaultBuiltInProcessor,   { hasUnknownSideEffects: true }                                              )
+registerBuiltInFunctions(false, ['attach'],                                        defaultBuiltInProcessor,   { hasUnknownSideEffects: true }                                              )
 
 /* they are all mapped to `<-` but we separate super assignments */
 registerReplacementFunctions({ makeMaybe: true },  ['<-', '<<-'], '[', '[[', '$', '@', 'names', 'dimnames', 'attributes', 'attr', 'class', 'levels', 'rownames', 'colnames')

src/dataflow/internal/linker.ts

@@ -25,6 +25,36 @@ import type { REnvironmentInformation } from '../environments/environment'
 
 export type NameIdMap = DefaultMap<string, IdentifierReference[]>
 
+export function findNonLocalReads(graph: DataflowGraph): IdentifierReference[] {
+	const ids = new Set(
+		[...graph.vertices(true)]
+			.filter(([_, info]) => info.tag === VertexType.Use || info.tag === VertexType.FunctionCall)
+			.map(([id, _]) => id)
+	)
+	/* find all variable use ids which do not link to a given id */
+	const nonLocalReads: IdentifierReference[] = []
+	for(const id of ids) {
+		const outgoing = graph.outgoingEdges(id)
+		if(outgoing === undefined) {
+			continue
+		}
+		for(const [target, { types }] of outgoing) {
+			if(edgeIncludesType(types, EdgeType.Reads) && !ids.has(target)) {
+				const name = recoverName(id, graph.idMap)
+				if(!name) {
+					dataflowLogger.warn('found non-local read without name for id ' + id)
+				}
+				nonLocalReads.push({
+					name:                recoverName(id, graph.idMap),
+					nodeId:              id,
+					controlDependencies: undefined
+				})
+			}
+		}
+	}
+	return nonLocalReads
+}
+
 export function produceNameSharedIdMap(references: IdentifierReference[]): NameIdMap {
 	const nameIdShares = new DefaultMap<string, IdentifierReference[]>(() => [])
 	for(const reference of references) {
@@ -270,7 +300,7 @@ export function linkInputs(referencesToLinkAgainstEnvironment: readonly Identifi
  * `x_2` must get a read marker to `x_1` as `x_1` is the active redefinition in the second loop iteration.
  */
 export function linkCircularRedefinitionsWithinALoop(graph: DataflowGraph, openIns: NameIdMap, outgoing: readonly IdentifierReference[]): void {
-	// first we preprocess out so that only the last definition of a given identifier survives
+	// first, we preprocess out so that only the last definition of a given identifier survives
 	// this implicitly assumes that the outgoing references are ordered
 	const lastOutgoing = new Map<string, IdentifierReference>()
 	for(const out of outgoing) {

src/dataflow/internal/process/functions/call/built-in/built-in-expression-list.ts

@@ -210,7 +210,7 @@ export function processExpressionList<OtherInfo>(
 		out,
 		environment:       environment,
 		graph:             nextGraph,
-		/* if we have no group we take the last evaluated expr */
+		/* if we have no group, we take the last evaluated expr */
 		entryPoint:        meId,
 		exitPoints:        withGroup ? [{ nodeId: rootId, type: ExitPointType.Default, controlDependencies: data.controlDependencies }]
 			: exitPoints

src/dataflow/internal/process/functions/call/built-in/built-in-for-loop.ts

@@ -3,6 +3,7 @@ import { processDataflowFor } from '../../../../../processor'
 import type { DataflowInformation } from '../../../../../info'
 import { filterOutLoopExitPoints, alwaysExits } from '../../../../../info'
 import {
+	findNonLocalReads,
 	linkCircularRedefinitionsWithinALoop,
 	produceNameSharedIdMap
 } from '../../../../linker'
@@ -17,10 +18,9 @@ import type { NodeId } from '../../../../../../r-bridge/lang-4.x/ast/model/proce
 import { overwriteEnvironment } from '../../../../../environments/overwrite'
 import { define } from '../../../../../environments/define'
 import { appendEnvironment } from '../../../../../environments/append'
-import { initializeCleanEnvironments, makeAllMaybe } from '../../../../../environments/environment'
+import { makeAllMaybe } from '../../../../../environments/environment'
 import { EdgeType } from '../../../../../graph/edge'
 import type { RSymbol } from '../../../../../../r-bridge/lang-4.x/ast/model/nodes/r-symbol'
-import { pushLocalEnvironment } from '../../../../../environments/scoping'
 
 export function processForLoop<OtherInfo>(
 	name: RSymbol<OtherInfo & ParentInformation>,
@@ -56,41 +56,19 @@ export function processForLoop<OtherInfo>(
 		headEnvironments = define({ ...write, definedAt: name.info.id, kind: 'variable' }, false, headEnvironments)
 	}
 	data = { ...data, environment: headEnvironments }
-	/* process the body without any environment first, to retrieve all open references */
-	let environment = initializeCleanEnvironments(false)
-	while(headEnvironments.level > environment.level) {
-		environment = pushLocalEnvironment(environment)
-	}
-	const body = processDataflowFor(bodyArg, { ...data, environment })
+
+	const body = processDataflowFor(bodyArg, data)
 
 	const nextGraph = headGraph.mergeWith(body.graph)
 	const outEnvironment = appendEnvironment(headEnvironments, body.environment)
 
-	// again within an if-then-else we consider all actives to be read
-	// currently I add it at the end, but is this correct?
-	const ingoing = [
-		...vector.in,
-		...makeAllMaybe(body.in, nextGraph, outEnvironment, false),
-		...vector.unknownReferences,
-		...makeAllMaybe(body.unknownReferences, nextGraph, outEnvironment, false)
-	]
-
-	// now we have to bind all open reads with the given name to the locally defined writtenVariable!
-	const nameIdShares = produceNameSharedIdMap(ingoing)
+	// now we have to identify all reads that may be effected by a circular redefinition
+	// for this, we search for all reads with a non-local read resolve!
+	const nameIdShares = produceNameSharedIdMap(findNonLocalReads(nextGraph))
 
 	for(const write of writtenVariable) {
 		nextGraph.addEdge(write.nodeId, vector.entryPoint, { type: EdgeType.DefinedBy })
-
-		const name = write.name
-		if(name) {
-			const readIdsToLink = nameIdShares.get(name)
-			for(const readId of readIdsToLink) {
-				nextGraph.addEdge(readId.nodeId, write.nodeId, { type: EdgeType.Reads })
-			}
-			// now, we remove the name from the id shares as they are no longer necessary
-			nameIdShares.delete(name)
-			nextGraph.setDefinitionOfVertex(write)
-		}
+		nextGraph.setDefinitionOfVertex(write)
 	}
 
 	const outgoing = [...variable.out, ...writtenVariable, ...makeAllMaybe(body.out, nextGraph, outEnvironment, true)]

src/dataflow/internal/process/functions/call/built-in/built-in-function-definition.ts

@@ -38,7 +38,7 @@ export function processFunctionDefinition<OtherInfo>(
 	/* we remove the last argument, as it is the body */
 	const parameters = args.slice(0, -1)
 	const bodyArg = unpackArgument(args[args.length - 1])
-	guard(bodyArg !== undefined, () => `Function Definition ${JSON.stringify(args)} has missing body! Bad!`)
+	guard(bodyArg !== undefined, () => `Function Definition ${JSON.stringify(args)} has missing body! This is bad!`)
 
 	const originalEnvironment = data.environment
 	// within a function def we do not pass on the outer binds as they could be overwritten when called

src/dataflow/internal/process/functions/call/built-in/built-in-repeat-loop.ts

@@ -2,6 +2,7 @@ import type { DataflowProcessorInformation } from '../../../../../processor'
 import type { DataflowInformation } from '../../../../../info'
 import { filterOutLoopExitPoints } from '../../../../../info'
 import {
+	findNonLocalReads,
 	linkCircularRedefinitionsWithinALoop,
 	produceNameSharedIdMap
 } from '../../../../linker'
@@ -32,14 +33,19 @@ export function processRepeatLoop<OtherInfo>(
 		args:      unpacked ? [unpacked] : args,
 		rootId,
 		data,
+		patchData: (d, i) => {
+			if(i === 0) {
+				return { ...d, controlDependencies: [...d.controlDependencies ?? [], { id: name.info.id }] }
+			}
+			return d
+		},
 		markAsNSE: [0]
 	})
 
 	const body = processedArguments[0]
 	guard(body !== undefined, () => `Repeat-Loop ${name.content} has no body, impossible!`)
 
-	const namedIdShares = produceNameSharedIdMap([...body.in, ...body.unknownReferences])
-	linkCircularRedefinitionsWithinALoop(information.graph, namedIdShares, body.out)
+	linkCircularRedefinitionsWithinALoop(information.graph, produceNameSharedIdMap(findNonLocalReads(information.graph)), body.out)
 
 	information.exitPoints = filterOutLoopExitPoints(information.exitPoints)
 

src/dataflow/internal/process/functions/call/built-in/built-in-while-loop.ts

@@ -2,6 +2,7 @@ import type { DataflowProcessorInformation } from '../../../../../processor'
 import type { DataflowInformation } from '../../../../../info'
 import { alwaysExits , filterOutLoopExitPoints } from '../../../../../info'
 import {
+	findNonLocalReads,
 	linkCircularRedefinitionsWithinALoop, linkInputs,
 	produceNameSharedIdMap
 } from '../../../../linker'
@@ -63,7 +64,7 @@ export function processWhileLoop<OtherInfo>(
 		...makeAllMaybe(body.unknownReferences, information.graph, information.environment, false),
 		...makeAllMaybe(body.in, information.graph, information.environment, false)
 	], information.environment, [...condition.in, ...condition.unknownReferences], information.graph, true)
-	linkCircularRedefinitionsWithinALoop(information.graph, produceNameSharedIdMap(remainingInputs), body.out)
+	linkCircularRedefinitionsWithinALoop(information.graph, produceNameSharedIdMap(findNonLocalReads(information.graph)), body.out)
 
 	// as the while-loop always evaluates its condition
 	information.graph.addEdge(name.info.id, condition.entryPoint, { type: EdgeType.Reads })

src/dataflow/internal/process/functions/call/common.ts

@@ -103,7 +103,6 @@ export function processAllArguments<OtherInfo>(
 		// resolve reads within argument, we resolve before adding the `processed.environment` to avoid cyclic dependencies
 		for(const ingoing of [...processed.in, ...processed.unknownReferences]) {
 			const tryToResolve = ingoing.name ? resolveByName(ingoing.name, argEnv) : undefined
-
 			if(tryToResolve === undefined) {
 				remainingReadInArgs.push(ingoing)
 			} else {